| Message ID | 1310608665-12216-1-git-send-email-andy@greyhouse.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thursday 14 July 2011 04:57:45 Andy Gospodarek wrote: > - if (strnicmp > - (slave->dev->name, buf, > - strlen(slave->dev->name)) == 0) { > + int max_len = max(strlen(slave->dev->name), > + strlen(buf) - 1); > + if (strnicmp(slave->dev->name, buf, max_len) == 0) { As for me there is no sense in preventing "address out of range" errors in strnicmp by calculating length with strlen first. If there is missing \0 at the end of the string you just shift failure point from stricmp to the strlen function call. IMHO "maximum length" argument in strnicmp should be some appropriate constant instead. Alternatively we can use count: if (strnicmp(slave->dev->name, buf, count) == 0) { > - if (strnicmp > - (slave->dev->name, buf, > - strlen(slave->dev->name)) == 0) { > + int max_len = max(strlen(slave->dev->name), > + strlen(buf) - 1); > + if (strnicmp(slave->dev->name, buf, max_len) == 0) { Same here.
Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote: >On Thursday 14 July 2011 04:57:45 Andy Gospodarek wrote: >> - if (strnicmp >> - (slave->dev->name, buf, >> - strlen(slave->dev->name)) == 0) { >> + int max_len = max(strlen(slave->dev->name), >> + strlen(buf) - 1); >> + if (strnicmp(slave->dev->name, buf, max_len) == 0) { > >As for me there is no sense in preventing "address out of range" errors in >strnicmp by calculating length with strlen first. If there is missing \0 at >the end of the string you just shift failure point from stricmp to the strlen >function call. >IMHO "maximum length" argument in strnicmp should be some appropriate constant >instead. Alternatively we can use count: I agree about using a constant, and I nominate IFNAMSIZ for that constant. Also, should we really be using strnicmp? I.e., case insensitive? Aren't interface names case sensitive? -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 14, 2011 at 09:02:06AM -0700, Jay Vosburgh wrote: > Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote: > > >On Thursday 14 July 2011 04:57:45 Andy Gospodarek wrote: > >> - if (strnicmp > >> - (slave->dev->name, buf, > >> - strlen(slave->dev->name)) == 0) { > >> + int max_len = max(strlen(slave->dev->name), > >> + strlen(buf) - 1); > >> + if (strnicmp(slave->dev->name, buf, max_len) == 0) { > > > >As for me there is no sense in preventing "address out of range" errors in > >strnicmp by calculating length with strlen first. If there is missing \0 at > >the end of the string you just shift failure point from stricmp to the strlen > >function call. > >IMHO "maximum length" argument in strnicmp should be some appropriate constant > >instead. Alternatively we can use count: > > I agree about using a constant, and I nominate IFNAMSIZ for that > constant. > A constant like IFNAMSIZ can work, but only if buf has the '\n' removed from the string that is added by echo or other command first. > Also, should we really be using strnicmp? I.e., case > insensitive? Aren't interface names case sensitive? Probably not. I'll roll a patch next week that drops the newline and uses IFNAMSIZ if that is the preference. I didn't think it was worth the trouble initially, so I didn't do it that way the first time. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index b60835f..50e859c 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -1037,9 +1037,9 @@ static ssize_t bonding_store_primary(struct device *d, bond->dev->name, bond->dev->name, bond->params.mode); } else { bond_for_each_slave(bond, slave, i) { - if (strnicmp - (slave->dev->name, buf, - strlen(slave->dev->name)) == 0) { + int max_len = max(strlen(slave->dev->name), + strlen(buf) - 1); + if (strnicmp(slave->dev->name, buf, max_len) == 0) { pr_info("%s: Setting %s as primary slave.\n", bond->dev->name, slave->dev->name); bond->primary_slave = slave; @@ -1208,9 +1208,9 @@ static ssize_t bonding_store_active_slave(struct device *d, bond->dev->name, bond->dev->name, bond->params.mode); else { bond_for_each_slave(bond, slave, i) { - if (strnicmp - (slave->dev->name, buf, - strlen(slave->dev->name)) == 0) { + int max_len = max(strlen(slave->dev->name), + strlen(buf) - 1); + if (strnicmp(slave->dev->name, buf, max_len) == 0) { old_active = bond->curr_active_slave; new_active = slave; if (new_active == old_active) {
When a bond contains a device where one name is the subset of another (eth1 and eth10, for example), one cannot properly set the primary device or the currently active device. This was reported and based on work by Takuma Umeya. I also verified the problem and tested that this fix resolves it. Signed-off-by: Andy Gospodarek <andy@greyhouse.net> Reported-by: Takuma Umeya <tumeya@redhat.com> --- drivers/net/bonding/bond_sysfs.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)