[patch] usb: f_fs: off by one bug in _ffs_func_bind()

[patch] usb: f_fs: off by one bug in _ffs_func_bind()

[Dan Carpenter]

This loop is supposed to set all the .num values to -1 but it's doesn't
set the first element and it sets one element beyond the end of the
array.  Really there is no reason for it to be done backwards.  And
"ret" is the wrong variable to use for an iterator.

Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I just spotted this reviewing the code, I have not tested it.  Please
review carefully, the vla_ptr() macro is difficult to understand.

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d5..7fff81a 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	       ffs->raw_descs_length);
 
 	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
-	for (ret = ffs->eps_count; ret; --ret) {
+	for (i = 0; i < ffs->eps_count; i++) {
 		struct ffs_ep *ptr;
 
 		ptr = vla_ptr(vlabuf, d, eps);
-		ptr[ret].num = -1;
+		ptr[i].num = -1;
 	}
 
 	/* Save pointers

Re: [patch] usb: f_fs: off by one bug in _ffs_func_bind()

[walter harms]

Am 27.05.2016 13:23, schrieb Dan Carpenter:
> This loop is supposed to set all the .num values to -1 but it's doesn't
> set the first element and it sets one element beyond the end of the
> array.  Really there is no reason for it to be done backwards.  And
> "ret" is the wrong variable to use for an iterator.
> 
> Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I just spotted this reviewing the code, I have not tested it.  Please
> review carefully, the vla_ptr() macro is difficult to understand.
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73515d5..7fff81a 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration *c,
>  	       ffs->raw_descs_length);
>  
>  	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
> -	for (ret = ffs->eps_count; ret; --ret) {
> +	for (i = 0; i < ffs->eps_count; i++) {
>  		struct ffs_ep *ptr;
>  
>  		ptr = vla_ptr(vlabuf, d, eps);
> -		ptr[ret].num = -1;
> +		ptr[i].num = -1;
>  	}
>  

The patch is ok (programmers are bad at counting backward) but the
code got me a bit confused (easy done).

so far i understand:

ptr=vla_ptr(vlabuf, d, eps) expands into  ptr=vlabuf+d_eps__offset
so it is a const.

so why not move the assign out of the loop ?
struct ffs_ep *ptr=vla_ptr(vlabuf, d, eps);
for (i = 0; i < ffs->eps_count; i++)
	ptr[i].num = -1;


just my 2 cents,

re,
 wh


>  	/* Save pointers
> --
> 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] usb: f_fs: off by one bug in _ffs_func_bind()

[Michal Nazarewicz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 2053 bytes --]

On Fri, May 27 2016, Dan Carpenter wrote:
> This loop is supposed to set all the .num values to -1 but it's doesn't
> set the first element and it sets one element beyond the end of the
> array.  Really there is no reason for it to be done backwards.  And
> "ret" is the wrong variable to use for an iterator.
>
> Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

How on Earth could I have made that mistake is beyond my
comprehension. O_o On second thought, things being beyond one’s
comprehension is probably how bugs are introduced…

> ---
> I just spotted this reviewing the code, I have not tested it.  Please
> review carefully, the vla_ptr() macro is difficult to understand.

Yeah.  In retrospect I’m not sure savings in memory those macros bring
offset time engineers spent trying to understand them.  Damn you clang!

> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73515d5..7fff81a 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration *c,
>  	       ffs->raw_descs_length);
>  
>  	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
> -	for (ret = ffs->eps_count; ret; --ret) {
> +	for (i = 0; i < ffs->eps_count; i++) {
>  		struct ffs_ep *ptr;
>  
>  		ptr = vla_ptr(vlabuf, d, eps);

As pointed by Walter, this could be moved outside.  Maybe

	i = ffs->eps_count;
	for (struct ffs_ep *ptr = vla_ptr(vlabuf, d, eps); i; ++ptr, --i)
		ptr->num = -1;

> -		ptr[ret].num = -1;
> +		ptr[i].num = -1;
>  	}
>  
>  	/* Save pointers

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
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] usb: f_fs: off by one bug in _ffs_func_bind()

[walter harms]

Am 27.05.2016 14:23, schrieb Michal Nazarewicz:
> On Fri, May 27 2016, Dan Carpenter wrote:
>> This loop is supposed to set all the .num values to -1 but it's doesn't
>> set the first element and it sets one element beyond the end of the
>> array.  Really there is no reason for it to be done backwards.  And
>> "ret" is the wrong variable to use for an iterator.
>>
>> Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Acked-by: Michal Nazarewicz <mina86@mina86.com>
> 
> How on Earth could I have made that mistake is beyond my
> comprehension. O_o On second thought, things being beyond one’s
> comprehension is probably how bugs are introduced…
> 
>> ---
>> I just spotted this reviewing the code, I have not tested it.  Please
>> review carefully, the vla_ptr() macro is difficult to understand.
> 
> Yeah.  In retrospect I’m not sure savings in memory those macros bring
> offset time engineers spent trying to understand them.  Damn you clang!
> 
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 73515d5..7fff81a 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration *c,
>>  	       ffs->raw_descs_length);
>>  
>>  	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
>> -	for (ret = ffs->eps_count; ret; --ret) {
>> +	for (i = 0; i < ffs->eps_count; i++) {
>>  		struct ffs_ep *ptr;
>>  
>>  		ptr = vla_ptr(vlabuf, d, eps);
> 
> As pointed by Walter, this could be moved outside.  Maybe
> 
> 	i = ffs->eps_count;
> 	for (struct ffs_ep *ptr = vla_ptr(vlabuf, d, eps); i; ++ptr, --i)
> 		ptr->num = -1;
> 

I think staying with an array here improves readability.

re,
 wh

>> -		ptr[ret].num = -1;
>> +		ptr[i].num = -1;
>>  	}
>>  
>>  	/* Save pointers
> 

Re: [patch] usb: f_fs: off by one bug in _ffs_func_bind()

[Dan Carpenter]

On Fri, May 27, 2016 at 07:25:30PM +0200, walter harms wrote:
> 
> 
> Am 27.05.2016 14:23, schrieb Michal Nazarewicz:
> > On Fri, May 27 2016, Dan Carpenter wrote:
> >> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> >> index 73515d5..7fff81a 100644
> >> --- a/drivers/usb/gadget/function/f_fs.c
> >> +++ b/drivers/usb/gadget/function/f_fs.c
> >> @@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration *c,
> >>  	       ffs->raw_descs_length);
> >>  
> >>  	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
> >> -	for (ret = ffs->eps_count; ret; --ret) {
> >> +	for (i = 0; i < ffs->eps_count; i++) {
> >>  		struct ffs_ep *ptr;
> >>  
> >>  		ptr = vla_ptr(vlabuf, d, eps);
> > 
> > As pointed by Walter, this could be moved outside.  Maybe
> > 
> > 	i = ffs->eps_count;
> > 	for (struct ffs_ep *ptr = vla_ptr(vlabuf, d, eps); i; ++ptr, --i)
> > 		ptr->num = -1;
> > 
> 
> I think staying with an array here improves readability.
> 

I'm surprised you didn't comment on the --i.  I was thinking about you
when I changed it to a ++ loop.  Also in the kernel we have to declare
variables at the start of the block.

Anyway, let me send a v2.

regards,
dan carpenter

[patch v2] usb: f_fs: off by one bug in _ffs_func_bind()

[Dan Carpenter]

This loop is supposed to set all the .num[] values to -1 but it's off by
one so it skips the first element and sets one element past the end of
the array.

I've cleaned up the loop a little as well.

Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: move the eps_ptr assignment outside the loop.

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 73515d5..d26eb64 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2729,6 +2729,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
 		func->ffs->ss_descs_count;
 
 	int fs_len, hs_len, ss_len, ret, i;
+	struct ffs_ep *eps_ptr;
 
 	/* Make it a single chunk, less management later on */
 	vla_group(d);
@@ -2777,12 +2778,9 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	       ffs->raw_descs_length);
 
 	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
-	for (ret = ffs->eps_count; ret; --ret) {
-		struct ffs_ep *ptr;
-
-		ptr = vla_ptr(vlabuf, d, eps);
-		ptr[ret].num = -1;
-	}
+	eps_ptr = vla_ptr(vlabuf, d, eps);
+	for (i = 0; i < ffs->eps_count; i++)
+		eps_ptr[i].num = -1;
 
 	/* Save pointers
 	 * d_eps = vlabuf, func->eps used to kfree vlabuf later

Re: [patch v2] usb: f_fs: off by one bug in _ffs_func_bind()

[walter harms]

much better readable than the original.
nice work

re,
 wh

Am 28.05.2016 06:48, schrieb Dan Carpenter:
> This loop is supposed to set all the .num[] values to -1 but it's off by
> one so it skips the first element and sets one element past the end of
> the array.
> 
> I've cleaned up the loop a little as well.
> 
> Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: move the eps_ptr assignment outside the loop.
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73515d5..d26eb64 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2729,6 +2729,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
>  		func->ffs->ss_descs_count;
>  
>  	int fs_len, hs_len, ss_len, ret, i;
> +	struct ffs_ep *eps_ptr;
>  
>  	/* Make it a single chunk, less management later on */
>  	vla_group(d);
> @@ -2777,12 +2778,9 @@ static int _ffs_func_bind(struct usb_configuration *c,
>  	       ffs->raw_descs_length);
>  
>  	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
> -	for (ret = ffs->eps_count; ret; --ret) {
> -		struct ffs_ep *ptr;
> -
> -		ptr = vla_ptr(vlabuf, d, eps);
> -		ptr[ret].num = -1;
> -	}
> +	eps_ptr = vla_ptr(vlabuf, d, eps);
> +	for (i = 0; i < ffs->eps_count; i++)
> +		eps_ptr[i].num = -1;
>  
>  	/* Save pointers
>  	 * d_eps = vlabuf, func->eps used to kfree vlabuf later
> --
> 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 v2] usb: f_fs: off by one bug in _ffs_func_bind()

[Michal Nazarewicz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1781 bytes --]

On Sat, May 28 2016, Dan Carpenter wrote:
> This loop is supposed to set all the .num[] values to -1 but it's off by
> one so it skips the first element and sets one element past the end of
> the array.
>
> I've cleaned up the loop a little as well.
>
> Fixes: ddf8abd25994 ('USB: f_fs: the FunctionFS driver')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> ---
> v2: move the eps_ptr assignment outside the loop.
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73515d5..d26eb64 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2729,6 +2729,7 @@ static int _ffs_func_bind(struct usb_configuration *c,
>  		func->ffs->ss_descs_count;
>  
>  	int fs_len, hs_len, ss_len, ret, i;
> +	struct ffs_ep *eps_ptr;
>  
>  	/* Make it a single chunk, less management later on */
>  	vla_group(d);
> @@ -2777,12 +2778,9 @@ static int _ffs_func_bind(struct usb_configuration *c,
>  	       ffs->raw_descs_length);
>  
>  	memset(vla_ptr(vlabuf, d, inums), 0xff, d_inums__sz);
> -	for (ret = ffs->eps_count; ret; --ret) {
> -		struct ffs_ep *ptr;
> -
> -		ptr = vla_ptr(vlabuf, d, eps);
> -		ptr[ret].num = -1;
> -	}
> +	eps_ptr = vla_ptr(vlabuf, d, eps);
> +	for (i = 0; i < ffs->eps_count; i++)
> +		eps_ptr[i].num = -1;
>  
>  	/* Save pointers
>  	 * d_eps = vlabuf, func->eps used to kfree vlabuf later

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
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] usb: f_fs: off by one bug in _ffs_func_bind()

[Michal Nazarewicz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 727 bytes --]

On Sat, May 28 2016, Dan Carpenter wrote:
> Also in the kernel we have to declare variables at the start of the
> block.

/me shrugs

I looked at this out of curiosity and there are precedents:

    $ git grep 'for (\(int\|unsigned\|signed\|long\|char\)[[:space:]]' |wc -l
    19

(albeit mostly in tools and scripts), CodingStyle is silent on that
front and checkpatch.pl doesn’t complain about it.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
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] usb: f_fs: off by one bug in _ffs_func_bind()

[Dan Carpenter]

On Sat, May 28, 2016 at 12:15:24PM +0200, Michal Nazarewicz wrote:
> On Sat, May 28 2016, Dan Carpenter wrote:
> > Also in the kernel we have to declare variables at the start of the
> > block.
> 
> /me shrugs
> 
> I looked at this out of curiosity and there are precedents:
> 
>     $ git grep 'for (\(int\|unsigned\|signed\|long\|char\)[[:space:]]' |wc -l
>     19
> 
> (albeit mostly in tools and scripts), CodingStyle is silent on that
> front and checkpatch.pl doesn’t complain about it.

Try compiling the code you suggested.

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] usb: f_fs: off by one bug in _ffs_func_bind()

[Dan Carpenter]

On Sat, May 28, 2016 at 12:15:24PM +0200, Michal Nazarewicz wrote:
> On Sat, May 28 2016, Dan Carpenter wrote:
> > Also in the kernel we have to declare variables at the start of the
> > block.
> 
> /me shrugs
> 
> I looked at this out of curiosity and there are precedents:
> 
>     $ git grep 'for (\(int\|unsigned\|signed\|long\|char\)[[:space:]]' |wc -l
>     19
> 
> (albeit mostly in tools and scripts), CodingStyle is silent on that
> front and checkpatch.pl doesn’t complain about it.

There is one in Documentation, the rest are basically all in tools or
ifdef 0.  Otherwise it causes a GCC warning.

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] usb: f_fs: off by one bug in _ffs_func_bind()

[Michal Nazarewicz]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 662 bytes --]

On Sat, May 28 2016, Dan Carpenter wrote:
> Try compiling the code you suggested.

I find this amusing to be perfectly honest.

Kernel uses designated initialisers, flexible array members, stdint.h,
compound literals and variadic macros, yet still for some reason sticks
to -std=gnu89.

No matter, this is a rather off topic discussion.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
--
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