[PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Michal Nazarewicz]

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) but make the code
slightly more confusing then it needs to be.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/w1/w1.c | 10 +++++-----
 drivers/w1/w1.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index fa932c2..66efa96 100644
--- 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);
 
 	memset(&msg, 0, sizeof(msg));
 	memcpy(&sl->reg_num, rn, sizeof(sl->reg_num));
@@ -866,7 +866,7 @@ void w1_slave_found(struct w1_master *dev, u64 rn)
 
 	sl = w1_slave_search_device(dev, tmp);
 	if (sl) {
-		set_bit(W1_SLAVE_ACTIVE, (long *)&sl->flags);
+		set_bit(W1_SLAVE_ACTIVE, &sl->flags);
 	} else {
 		if (rn && tmp->crc == w1_calc_crc8((u8 *)&rn_le, 7))
 			w1_attach_slave_device(dev, tmp);
@@ -984,14 +984,14 @@ void w1_search_process_cb(struct w1_master *dev, u8 search_type,
 	struct w1_slave *sl, *sln;
 
 	list_for_each_entry(sl, &dev->slist, w1_slave_entry)
-		clear_bit(W1_SLAVE_ACTIVE, (long *)&sl->flags);
+		clear_bit(W1_SLAVE_ACTIVE, &sl->flags);
 
 	w1_search_devices(dev, search_type, cb);
 
 	list_for_each_entry_safe(sl, sln, &dev->slist, w1_slave_entry) {
-		if (!test_bit(W1_SLAVE_ACTIVE, (unsigned long *)&sl->flags) && !--sl->ttl)
+		if (!test_bit(W1_SLAVE_ACTIVE, &sl->flags) && !--sl->ttl)
 			w1_slave_detach(sl);
-		else if (test_bit(W1_SLAVE_ACTIVE, (unsigned long *)&sl->flags))
+		else if (test_bit(W1_SLAVE_ACTIVE, &sl->flags))
 			sl->ttl = dev->slave_ttl;
 	}
 
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index 45908e5..ca8081a 100644
--- 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;
+	unsigned long		flags;
 
 	struct w1_master	*master;
 	struct w1_family	*family;
-- 
1.8.4

Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Andrew Morton]

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?

> +	unsigned long		flags;
>  
>  	struct w1_master	*master;
>  	struct w1_family	*family;
>
> ...
>

Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

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--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Evgeniy Polyakov]

Hi everyone

31.10.2013, 13:12, "Michal Nazarewicz" <mina86@mina86.com>:

>>>           int ttl;
>>  ... w1_slave.ttl?

For noisy lines this was a 'timeout' after which system considered attached
device as failed, the noisier line is the longer is timeout

I experimented with meters-long w1 wires and it required several
search fails to correctly determine that device disappeared

> 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:

Guys, you so much overcomplicate things - this field is basically a set of in-memory flags
for attached device, there is no need to even think about how it is present in different endianess

Or do I miss something fundamental there? I wrote it gazillions years ago and probably already forgot something

Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Andrew Morton]

On Fri, 01 Nov 2013 20:01:39 +0400 Evgeniy Polyakov <zbr@ioremap.net> wrote:

> > 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:
> 
> Guys, you so much overcomplicate things - this field is basically a set of in-memory flags
> for attached device, there is no need to even think about how it is present in different endianess
> 
> Or do I miss something fundamental there? I wrote it gazillions years ago and probably already forgot something

set_bit() operates on longs.  So if we do

struct foo {
	u32 a;
	u32 b;
} f;

	...
	set_bit(0, (long *)&f.a);
	...

then we'll scribble on f.b on a big-endian 64-bit machine.

Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Рустафа Джамурахметов]

Hi

01.11.2013, 23:30, "Andrew Morton" <akpm@linux-foundation.org>:

> set_bit() operates on longs.  So if we do
>
> struct foo {
>         u32 a;
>         u32 b;
> } f;
>
>         ...
>         set_bit(0, (long *)&f.a);
>         ...
>
> then we'll scribble on f.b on a big-endian 64-bit machine.

Argh, why would we just don't do that? Its in-memory field, it can be anything,
I wouldn't be surprised if it even can be non-atomic because of proper locks already
being held

Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Michal Nazarewicz]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

> 01.11.2013, 23:30, "Andrew Morton" <akpm@linux-foundation.org>:
>> set_bit() operates on longs.  So if we do
>>
>> struct foo { u32 a; u32 b; } f;
>> set_bit(0, (long *)&f.a);
>>
>> then we'll scribble on f.b on a big-endian 64-bit machine.

On Sat, Nov 02 2013, Рустафа Джамурахметов <zbr@ioremap.net> wrote:
> Argh, why would we just don't do that? Its in-memory field, it can be
> anything, I wouldn't be surprised if it even can be non-atomic because
> of proper locks already being held

If the driver does not require an atomic set_bit operation for setting
and testing the flag, feel free to prepare a patch replacing the whole
thing with a plain bool.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

[Рустафа Джамурахметов]

Hi

02.11.2013, 20:59, "Michal Nazarewicz" <mina86@mina86.com>:
>>  Argh, why would we just don't do that? Its in-memory field, it can be
>>  anything, I wouldn't be surprised if it even can be non-atomic because
>>  of proper locks already being held
>
> If the driver does not require an atomic set_bit operation for setting
> and testing the flag, feel free to prepare a patch replacing the whole
> thing with a plain bool.

Please make sure it is properly protected if it is ever required