[patch] w1: off by one in w1_f29_remove_slave()

[patch] w1: off by one in w1_f29_remove_slave()

[Dan Carpenter]

This reads past the end of the w1_f29_sysfs_bin_files[] array.  I
don't have this hardware, but I assume it crashes every time the
function is called.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
index c377818..0da4e97 100644
--- a/drivers/w1/slaves/w1_ds2408.c
+++ b/drivers/w1/slaves/w1_ds2408.c
@@ -373,7 +373,7 @@ static int w1_f29_add_slave(struct w1_slave *sl)
 static void w1_f29_remove_slave(struct w1_slave *sl)
 {
 	int i;
-	for (i = NB_SYSFS_BIN_FILES; i <= 0; --i)
+	for (i = NB_SYSFS_BIN_FILES - 1; i <= 0; --i)
 		sysfs_remove_bin_file(&sl->dev.kobj,
 			&(w1_f29_sysfs_bin_files[i]));
 }

Re: [patch] w1: off by one in w1_f29_remove_slave()

[Jean-François Dagenais]

Yup, confirmed, weird that I did not notice a crash, probably lucky shot in the environment I was using it.

On 2011-07-27, at 05:55, Dan Carpenter wrote:

> This reads past the end of the w1_f29_sysfs_bin_files[] array.  I
> don't have this hardware, but I assume it crashes every time the
> function is called.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Acked-by: Jean-François Dagenais <dagenaisj@sonatest.com>
> 
> diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
> index c377818..0da4e97 100644
> --- a/drivers/w1/slaves/w1_ds2408.c
> +++ b/drivers/w1/slaves/w1_ds2408.c
> @@ -373,7 +373,7 @@ static int w1_f29_add_slave(struct w1_slave *sl)
> static void w1_f29_remove_slave(struct w1_slave *sl)
> {
> 	int i;
> -	for (i = NB_SYSFS_BIN_FILES; i <= 0; --i)
> +	for (i = NB_SYSFS_BIN_FILES - 1; i <= 0; --i)
> 		sysfs_remove_bin_file(&sl->dev.kobj,
> 			&(w1_f29_sysfs_bin_files[i]));
> }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] w1: off by one in w1_f29_remove_slave()

[Dan Carpenter]

On Sat, Jul 30, 2011 at 12:46:10AM +0200, bojan prtvar wrote:
> 
> Shouldn't it be i>=0 ?
> 

Yup...  Good eye.  Will send a v2 tomorrow.

regards,
dan carpenter

[patch v2] w1: fix for loop in w1_f29_remove_slave()

[Dan Carpenter]

The for loop was looking for i <= 0 instead of i >= 0 so this
function never did anything.  Also we started with i NB_SYSFS_BIN_FILES instead of "NB_SYSFS_BIN_FILES - 1" which is an
off by one bug.

Reported-by: Bojan Prtvar <prtvar.b@gmail.com>
Signed-off-by: Dan Carpenter <error27@gmail.com>
---
v2:  fix >= vs <
diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
index c377818..7c8cdb8 100644
--- a/drivers/w1/slaves/w1_ds2408.c
+++ b/drivers/w1/slaves/w1_ds2408.c
@@ -373,7 +373,7 @@ static int w1_f29_add_slave(struct w1_slave *sl)
 static void w1_f29_remove_slave(struct w1_slave *sl)
 {
 	int i;
-	for (i = NB_SYSFS_BIN_FILES; i <= 0; --i)
+	for (i = NB_SYSFS_BIN_FILES - 1; i >= 0; --i)
 		sysfs_remove_bin_file(&sl->dev.kobj,
 			&(w1_f29_sysfs_bin_files[i]));
 }

Re: [patch v2] w1: fix for loop in w1_f29_remove_slave()

[Jean-Francois Dagenais]

> The for loop was looking for i <= 0 instead of i >= 0 so this
> function never did anything.  Also we started with i > NB_SYSFS_BIN_FILES instead of "NB_SYSFS_BIN_FILES - 1" which is an
> off by one bug.
> 
> Reported-by: Bojan Prtvar <prtvar.b@gmail.com>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Acked-by: Jean-François Dagenais <dagenaisj@sonatest.com>
> ---
> v2:  fix >= vs <> 
> diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
> index c377818..7c8cdb8 100644
> --- a/drivers/w1/slaves/w1_ds2408.c
> +++ b/drivers/w1/slaves/w1_ds2408.c
> @@ -373,7 +373,7 @@ static int w1_f29_add_slave(struct w1_slave *sl)
> static void w1_f29_remove_slave(struct w1_slave *sl)
> {
> 	int i;
> -	for (i = NB_SYSFS_BIN_FILES; i <= 0; --i)
> +	for (i = NB_SYSFS_BIN_FILES - 1; i >= 0; --i)
> 		sysfs_remove_bin_file(&sl->dev.kobj,
> 			&(w1_f29_sysfs_bin_files[i]));
> }

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] w1: off by one in w1_f29_remove_slave()

[walter harms]

Am 29.07.2011 22:28, schrieb Jean-François Dagenais:
> Yup, confirmed, weird that I did not notice a crash, probably lucky shot in the environment I was using it.
> 
> On 2011-07-27, at 05:55, Dan Carpenter wrote:
> 
>> This reads past the end of the w1_f29_sysfs_bin_files[] array.  I
>> don't have this hardware, but I assume it crashes every time the
>> function is called.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
> Acked-by: Jean-François Dagenais <dagenaisj@sonatest.com>
>>
>> diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
>> index c377818..0da4e97 100644
>> --- a/drivers/w1/slaves/w1_ds2408.c
>> +++ b/drivers/w1/slaves/w1_ds2408.c
>> @@ -373,7 +373,7 @@ static int w1_f29_add_slave(struct w1_slave *sl)
>> static void w1_f29_remove_slave(struct w1_slave *sl)
>> {
>> 	int i;
>> -	for (i = NB_SYSFS_BIN_FILES; i <= 0; --i)
>> +	for (i = NB_SYSFS_BIN_FILES - 1; i <= 0; --i)
>> 		sysfs_remove_bin_file(&sl->dev.kobj,
>> 			&(w1_f29_sysfs_bin_files[i]));
>> }


Is there any reason to release the data "backwards" ?
The default way is to go upwards. that looks more readable.

for (i=0; i < NB_SYSFS_BIN_FILES ; i++)
  sysfs_remove_bin_file(&sl->dev.kobj, w1_f29_sysfs_bin_files+i);

re,
 wh
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] w1: off by one in w1_f29_remove_slave()

[Dan Carpenter]

On Sat, Jul 30, 2011 at 10:42:29AM +0200, walter harms wrote:
> Is there any reason to release the data "backwards" ?
> The default way is to go upwards. that looks more readable.

I considered doing it that way.  It would work.  I throught maybe
they were going to add something where it needed to be unwound LIFO
order.

I'll wait for Jean-Françoisto say which way is prefered.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] w1: off by one in w1_f29_remove_slave()

[Jean-Francois Dagenais]

On Jul 30, 2011, at 4:51, Dan Carpenter wrote:

> On Sat, Jul 30, 2011 at 10:42:29AM +0200, walter harms wrote:
>> Is there any reason to release the data "backwards" ?
>> The default way is to go upwards. that looks more readable.
> 
> I considered doing it that way.  It would work.  I throught maybe
> they were going to add something where it needed to be unwound LIFO
> order.
> 
> I'll wait for Jean-Françoisto say which way is prefered.
> 
Sorry for the delay. Sorry also for the trouble this dummy copy/paste mistake is causing. It's a bit embarrassing even if the intentions are good ;) This second mistake explains why I wasn't getting ill effects from the remove.

The sysfs files were removed backwards as a programmer reflex. In this case it is not really necessary since there are no real dependencies between the function implementations... but there may be in the future. This is why it's a safe habit to do it this way.

I would keep the removal in reverse order,  so I will ack the the v2 patch

Cheers!
/jfd
> regards,
> dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html