lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Dec 2013 13:42:59 +0100 From: Veaceslav Falico <vfalico@...hat.com> To: Eric Dumazet <eric.dumazet@...il.com> Cc: Nikolay Aleksandrov <nikolay@...hat.com>, netdev@...r.kernel.org, Andy Gospodarek <andy@...yhouse.net>, Jay Vosburgh <fubar@...ibm.com>, "David S. Miller" <davem@...emloft.net> Subject: Re: [PATCH net] bonding: fix packets_per_slave showing 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@...hat.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@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists