usb: f_fs: off by one bug in _ffs_func_bind()

PATCH Superseded Dan Carpenter 3 participants 12 messages May 27, 2016 – May 28, 2016

Subsystems (2)

USB Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2016-05-27 11:23 UTC #0 lore raw

PATCH 1/1 usb: f_fs: off by one bug in _ffs_func_bind()

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 <[email protected]>

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

drivers/usb/gadget/function/f_fs.c +2 -2

index 73515d5..7fff81a 100644
@@ -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

walter harms

2016-05-27 11:45 UTC #1 lore raw

Am 27.05.2016 13:23, schrieb Dan Carpenter:

Quoted 18 lines

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 <[email protected]>
---
I just spotted this reviewing the code, I have not tested it. Please review carefully, the vla_ptr() macro is difficult to understand.
drivers/usb/gadget/function/f_fs.c +0 -0
index 73515d5..7fff81a 100644
@@ -2777,11 +2777,11 @@ static int _ffs_func_bind(struct usb_configuration *c,
 	       ffs->raw_descs_length);
 

drivers/usb/gadget/function/f_fs.c static int _ffs_func_bind(struct usb_configuration *c, · line 2786

Quoted 10 lines

drivers/usb/gadget/function/f_fs.c +2 -2

 	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

Quoted 6 lines

drivers/usb/gadget/function/f_fs.c +0 -1
 	/* Save pointers
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michal Nazarewicz

Acked-by on v1 1/1

2016-05-27 12:23 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 7 lines · 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 <[email protected]>

Acked-by Michal Nazarewicz <[email protected]>

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â€¦

Quoted 3 lines

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

drivers/usb/gadget/function/f_fs.c static int _ffs_func_bind(struct usb_configuration *c, · line 2783

Quoted 13 lines

drivers/usb/gadget/function/f_fs.c +1 -1

index 73515d5..7fff81a 100644
@@ -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;

Quoted 5 lines

drivers/usb/gadget/function/f_fs.c +1 -1

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

Signature · 7 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

walter harms

2016-05-27 17:25 UTC #3 lore raw

In reply to Dan Carpenter

Am 27.05.2016 14:23, schrieb Michal Nazarewicz:

Quoted 34 lines · Dan Carpenter

Quoted 7 lines
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 <[email protected]>
Acked-by Michal Nazarewicz <[email protected]>
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…
Quoted 3 lines
---
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!
Quoted 12 lines
drivers/usb/gadget/function/f_fs.c +1 -1
index 73515d5..7fff81a 100644
@@ -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;
 

Quoted 8 lines

drivers/usb/gadget/function/f_fs.c static int _ffs_func_bind(struct usb_configuration *c, · line 2783
Quoted 1 line
drivers/usb/gadget/function/f_fs.c +0 -0
 		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

Quoted 6 lines

Quoted 5 lines
drivers/usb/gadget/function/f_fs.c +1 -1
-		ptr[ret].num = -1;
+		ptr[i].num = -1;
 	}
 
 	/* Save pointers

Dan Carpenter

author

2016-05-28 04:46 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 20 lines · walter harms

Am 27.05.2016 14:23, schrieb Michal Nazarewicz:
Quoted 17 lines · Dan Carpenter
drivers/usb/gadget/function/f_fs.c static int _ffs_func_bind(struct usb_configuration *c, · line 2783
Quoted 13 lines
drivers/usb/gadget/function/f_fs.c +1 -1
index 73515d5..7fff81a 100644
@@ -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

Quoted 7 lines

Quoted 4 lines
	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

Dan Carpenter

author

2016-05-28 04:48 UTC #5 lore raw

PATCH v2 1/1 usb: f_fs: off by one bug in _ffs_func_bind()

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 <[email protected]>

---
v2: move the eps_ptr assignment outside the loop.

drivers/usb/gadget/function/f_fs.c +4 -6

index 73515d5..d26eb64 100644
@@ -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

walter harms

2016-05-28 09:05 UTC #6 lore raw

much better readable than the original.
nice work

re,
wh

Am 28.05.2016 06:48, schrieb Dan Carpenter:

Quoted 44 lines

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 <[email protected]>
---
v2: move the eps_ptr assignment outside the loop.
drivers/usb/gadget/function/f_fs.c +4 -7
index 73515d5..d26eb64 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michal Nazarewicz

Acked-by on v2 1/1

2016-05-28 10:16 UTC #7 lore raw

In reply to Dan Carpenter

Quoted 8 lines · 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 <[email protected]>

Acked-by Michal Nazarewicz <[email protected]>

Quoted 31 lines

---
v2: move the eps_ptr assignment outside the loop.

drivers/usb/gadget/function/f_fs.c +4 -6

index 73515d5..d26eb64 100644
@@ -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

Signature · 7 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Michal Nazarewicz

2016-05-28 10:15 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 2 lines · Dan Carpenter

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.

Signature · 7 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2016-05-28 10:53 UTC #9 lore raw

In reply to Dan Carpenter

Quoted 6 lines · Michal Nazarewicz

Quoted 2 lines · Dan Carpenter
Also in the kernel we have to declare variables at the start of the block.
/me shrugs

Quoted 7 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2016-05-28 11:05 UTC #10 lore raw

In reply to Dan Carpenter

Quoted 6 lines · Michal Nazarewicz

Quoted 2 lines · Dan Carpenter
Also in the kernel we have to declare variables at the start of the block.
/me shrugs

Quoted 7 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Michal Nazarewicz

2016-05-28 12:01 UTC #11 lore raw

In reply to Dan Carpenter

Quoted 1 line · Dan Carpenter

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.

Signature · 7 lines

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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html