| Message ID | 1386239818-7408-1-git-send-email-nikolay@redhat.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Dec 05, 2013 at 11:36:58AM +0100, Nikolay Aleksandrov wrote: >There's an issue when showing the value of packets_per_slave due to >using signed integer. The value may be < 0 and thus not put through >reciprocal_value() before showing. This patch makes it use unsigned >integer when showing it. I was already checking my basic algebra knowledge here, reciprocal_value(reciprocal_value(0..USHRT_MAX)) can become negative?!? :) If anyone's also wondering... packets_per_slave is reciprocal_value(0..USHRT_MAX), and thus can indeed be negative, and then the code if (packets_per_slave > 1) packets_per_slave = reciprocal_value(packets_per_slave); would fail to recognise that it's a reciprocal_divide() value, and not a standard 0/1 option (in bond_rr_gen_slave_id() we verify it via a switch, so we're safe there) - and thus output nonsense. Acked-by: Veaceslav Falico <vfalico@redhat.com> > >CC: Andy Gospodarek <andy@greyhouse.net> >CC: Jay Vosburgh <fubar@us.ibm.com> >CC: Veaceslav Falico <vfalico@redhat.com> >CC: David S. Miller <davem@davemloft.net> >Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com> >--- > drivers/net/bonding/bond_sysfs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c >index abf5e10..0ae580b 100644 >--- a/drivers/net/bonding/bond_sysfs.c >+++ b/drivers/net/bonding/bond_sysfs.c >@@ -1635,12 +1635,12 @@ static ssize_t bonding_show_packets_per_slave(struct device *d, > char *buf) > { > struct bonding *bond = to_bond(d); >- int packets_per_slave = bond->params.packets_per_slave; >+ unsigned int packets_per_slave = bond->params.packets_per_slave; > > if (packets_per_slave > 1) > packets_per_slave = reciprocal_value(packets_per_slave); > >- return sprintf(buf, "%d\n", packets_per_slave); >+ return sprintf(buf, "%u\n", packets_per_slave); > } > > static ssize_t bonding_store_packets_per_slave(struct device *d, >-- >1.8.1.4 > -- 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 12/05/2013 12:08 PM, Veaceslav Falico wrote: > On Thu, Dec 05, 2013 at 11:36:58AM +0100, Nikolay Aleksandrov wrote: >> There's an issue when showing the value of packets_per_slave due to >> using signed integer. The value may be < 0 and thus not put through >> reciprocal_value() before showing. This patch makes it use unsigned >> integer when showing it. > > I was already checking my basic algebra knowledge here, > reciprocal_value(reciprocal_value(0..USHRT_MAX)) can become negative?!? :) > > If anyone's also wondering... > > packets_per_slave is reciprocal_value(0..USHRT_MAX), and thus can indeed be > negative, and then the code > > if (packets_per_slave > 1) > packets_per_slave = reciprocal_value(packets_per_slave); > > would fail to recognise that it's a reciprocal_divide() value, and not a > standard 0/1 option (in bond_rr_gen_slave_id() we verify it via a switch, > so we're safe there) - and thus output nonsense. > > Acked-by: Veaceslav Falico <vfalico@redhat.com> > Thanks for the review. Indeed, I should've made it unsigned from the beginning but thought to be consistent with the other options (just don't ask why :) ). Anyhow, once net-next opens up I can convert it to unsigned completely as it's supposed to be. Cheers, Nik -- 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, 2013-12-05 at 12:08 +0100, Veaceslav Falico wrote: > On Thu, Dec 05, 2013 at 11:36:58AM +0100, Nikolay Aleksandrov wrote: > >There's an issue when showing the value of packets_per_slave due to > >using signed integer. The value may be < 0 and thus not put through > >reciprocal_value() before showing. This patch makes it use unsigned > >integer when showing it. > > I was already checking my basic algebra knowledge here, > reciprocal_value(reciprocal_value(0..USHRT_MAX)) can become negative?!? :) > > If anyone's also wondering... > > packets_per_slave is reciprocal_value(0..USHRT_MAX), and thus can indeed be > negative, and then the code > > if (packets_per_slave > 1) > packets_per_slave = reciprocal_value(packets_per_slave); > > would fail to recognise that it's a reciprocal_divide() value, and not a > standard 0/1 option (in bond_rr_gen_slave_id() we verify it via a switch, > so we're safe there) - and thus output nonsense. > > Acked-by: Veaceslav Falico <vfalico@redhat.com> This code is very confusing. Please rename bond->params.packets_per_slave to bond->params.reciprocal_packets_per_slave To make clear that its the reciprocal value. Also the module parameter is named packets_per_slave, it would be nice if same name was not reused as local variable in bond_rr_gen_slave_id() bond_check_params() reads the sys value several times. This is racy with /sys access. You should use ACCESS_ONCE() to make sure nothing bad happens. -- 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 12/05/2013 01:33 PM, Eric Dumazet wrote: > On Thu, 2013-12-05 at 12:08 +0100, Veaceslav Falico wrote: >> On Thu, Dec 05, 2013 at 11:36:58AM +0100, Nikolay Aleksandrov wrote: >>> There's an issue when showing the value of packets_per_slave due to >>> using signed integer. The value may be < 0 and thus not put through >>> reciprocal_value() before showing. This patch makes it use unsigned >>> integer when showing it. >> >> I was already checking my basic algebra knowledge here, >> reciprocal_value(reciprocal_value(0..USHRT_MAX)) can become negative?!? :) >> >> If anyone's also wondering... >> >> packets_per_slave is reciprocal_value(0..USHRT_MAX), and thus can indeed be >> negative, and then the code >> >> if (packets_per_slave > 1) >> packets_per_slave = reciprocal_value(packets_per_slave); >> >> would fail to recognise that it's a reciprocal_divide() value, and not a >> standard 0/1 option (in bond_rr_gen_slave_id() we verify it via a switch, >> so we're safe there) - and thus output nonsense. >> >> Acked-by: Veaceslav Falico <vfalico@redhat.com> > > This code is very confusing. > > Please rename bond->params.packets_per_slave > to bond->params.reciprocal_packets_per_slave > > To make clear that its the reciprocal value. > > Also the module parameter is named packets_per_slave, it would be nice > if same name was not reused as local variable in bond_rr_gen_slave_id() > > bond_check_params() reads the sys value several times. > > This is racy with /sys access. > IIRC bond_check_params() runs before sysfs is initialized for the bond device. > You should use ACCESS_ONCE() to make sure nothing bad happens. > Actually I think ACCESS_ONCE() should be added to bond_show_packets_per_slave and to bond_rr_gen_slave_id() just as a precaution. It'll also serve the purpose to show what's intended. What do you think ? > > Thanks for the feedback, I'll address the ACCESS_ONCE() in a separate patch for net and leave the renaming to my net-next patch which will take care of the types as well. Cheers, Nik -- 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, Dec 05, 2013 at 04:33:55AM -0800, Eric Dumazet wrote: >On Thu, 2013-12-05 at 12:08 +0100, Veaceslav Falico wrote: >> On Thu, Dec 05, 2013 at 11:36:58AM +0100, Nikolay Aleksandrov wrote: >> >There's an issue when showing the value of packets_per_slave due to >> >using signed integer. The value may be < 0 and thus not put through >> >reciprocal_value() before showing. This patch makes it use unsigned >> >integer when showing it. >> >> I was already checking my basic algebra knowledge here, >> reciprocal_value(reciprocal_value(0..USHRT_MAX)) can become negative?!? :) >> >> If anyone's also wondering... >> >> packets_per_slave is reciprocal_value(0..USHRT_MAX), and thus can indeed be >> negative, and then the code >> >> if (packets_per_slave > 1) >> packets_per_slave = reciprocal_value(packets_per_slave); >> >> would fail to recognise that it's a reciprocal_divide() value, and not a >> standard 0/1 option (in bond_rr_gen_slave_id() we verify it via a switch, >> so we're safe there) - and thus output nonsense. >> >> Acked-by: Veaceslav Falico <vfalico@redhat.com> > >This code is very confusing. > >Please rename bond->params.packets_per_slave >to bond->params.reciprocal_packets_per_slave > >To make clear that its the reciprocal value. > >Also the module parameter is named packets_per_slave, it would be nice >if same name was not reused as local variable in bond_rr_gen_slave_id() Agreed, but I think it'd be rather net-next material. Here we have a clear bug... > >bond_check_params() reads the sys value several times. > >This is racy with /sys access. > >You should use ACCESS_ONCE() to make sure nothing bad happens. Hrm? bond_check_params() isn't involved in sysfs at all :-/, it's called only via bonding_init(). And bond->params.packets_per_slave isn't read there at all, only assigned. Or, given the naming confusions, I'm again missing something? -- 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, 2013-12-05 at 13:42 +0100, Veaceslav Falico wrote: > . > > Hrm? bond_check_params() isn't involved in sysfs at all :-/, it's called > only via bonding_init(). And bond->params.packets_per_slave isn't read > there at all, only assigned. > > Or, given the naming confusions, I'm again missing something? This is so confusing. If the check was done properly, we could expose the value in /sys/module/bonding/parameters/packets_per_slave So that you can change the setting without reloading the module. -- 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
From: Nikolay Aleksandrov <nikolay@redhat.com> Date: Thu, 5 Dec 2013 11:36:58 +0100 > There's an issue when showing the value of packets_per_slave due to > using signed integer. The value may be < 0 and thus not put through > reciprocal_value() before showing. This patch makes it use unsigned > integer when showing it. > > CC: Andy Gospodarek <andy@greyhouse.net> > CC: Jay Vosburgh <fubar@us.ibm.com> > CC: Veaceslav Falico <vfalico@redhat.com> > CC: David S. Miller <davem@davemloft.net> > Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com> Applied, thanks. -- 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 abf5e10..0ae580b 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -1635,12 +1635,12 @@ static ssize_t bonding_show_packets_per_slave(struct device *d, char *buf) { struct bonding *bond = to_bond(d); - int packets_per_slave = bond->params.packets_per_slave; + unsigned int packets_per_slave = bond->params.packets_per_slave; if (packets_per_slave > 1) packets_per_slave = reciprocal_value(packets_per_slave); - return sprintf(buf, "%d\n", packets_per_slave); + return sprintf(buf, "%u\n", packets_per_slave); } static ssize_t bonding_store_packets_per_slave(struct device *d,
There's an issue when showing the value of packets_per_slave due to using signed integer. The value may be < 0 and thus not put through reciprocal_value() before showing. This patch makes it use unsigned integer when showing it. CC: Andy Gospodarek <andy@greyhouse.net> CC: Jay Vosburgh <fubar@us.ibm.com> CC: Veaceslav Falico <vfalico@redhat.com> CC: David S. Miller <davem@davemloft.net> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com> --- drivers/net/bonding/bond_sysfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)