Messages in this thread |  | | From | Michal Nazarewicz <> | Subject | Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts | Date | Thu, 31 Oct 2013 10:11:53 +0100 |
| |
On Wed, Oct 30 2013, Andrew Morton wrote: > On Sat, 26 Oct 2013 12:56:11 +0100 Michal Nazarewicz <mpn@google.com> wrote: > >> From: Michal Nazarewicz <mina86@mina86.com> >> >> Changing flags field of the w1_slave to unsigned long may on >> some architectures increase the size of the structure, but >> otherwise makes the code more kosher as casting is avoided >> and *_bit family of calls do not attempt to operate on an >> entity of bigger size than realy is available. >> >> The current behaviour does not introduce any bugs (since any >> bytes past flags field are preserved) > > hm, what does this mean.... > >> --- a/drivers/w1/w1.c >> +++ b/drivers/w1/w1.c >> @@ -709,7 +709,7 @@ static int w1_attach_slave_device(struct w1_master *dev, struct w1_reg_num *rn) >> >> sl->owner = THIS_MODULE; >> sl->master = dev; >> - set_bit(W1_SLAVE_ACTIVE, (long *)&sl->flags); >> + set_bit(W1_SLAVE_ACTIVE, &sl->flags); > > ... I'd have though that running this code on little-endian 64-bit > would result in a scribble over ... > >> --- a/drivers/w1/w1.h >> +++ b/drivers/w1/w1.h >> @@ -67,8 +67,8 @@ struct w1_slave >> struct w1_reg_num reg_num; >> atomic_t refcnt; >> u8 rom[9]; >> - u32 flags; >> int ttl; > > ... w1_slave.ttl?
Now that I look at documentation, I think you are correct, but the problem is on big-endian 64-bit architectures. The fix is still valid, but the commit message not so much. Something along the lines of the following would be better:
-------- >8 -------------------------------------------------------- drivers: w1: make w1_slave::flags long to avoid memory corruption
On architectures where long is more then 32 bits, modifying a 32-bit field with set_bit (and other atomic bit operations) may cause bytes following the field to by modified.
Because the endianness of the bits within a field is the native endianness of the CPU[1], on big-endian machines, bit number zero is in the last byte of the field.
Therefore, “set_bit(0, ptr)” on a 64-bit big-endian machine is roughly equivalent to “((char *)ptr)[7] |= 1”, and since w1 driver uses a 32-bit field for holding the flags, this causes bytes beyond the field to be modified.
[1] From Documentation/atomic_ops.txt:
Native atomic bit operations are defined to operate on objects aligned to the size of an "unsigned long" C data type, and are least of that size. The endianness of the bits within each "unsigned long" are the native endianness of the cpu. -------- >8 --------------------------------------------------------
>> + unsigned long flags; >> >> struct w1_master *master; >> struct w1_family *family;
-- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz (o o) ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo-- [unhandled content-type:application/pgp-signature] |  |