soc: aspeed: fix a ternary sign expansion bug
Subsystems (2)
PATCH 1/1 soc: aspeed: fix a ternary sign expansion bug
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the snoop_file_read() function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.
Fix this by removing the ternary so that "ret" is type promoted directly to long.
--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/soc/aspeed/aspeed-lpc-snoop.c +3 -1
@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret; -return ret ? ret : copied; +return copied; } static __poll_t snoop_file_poll(struct file *file,
Signature · 1 line
2.30.2
Quoted 10 lines · Dan Carpenter
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Hmm... Let's grep for kfifo_to_user() - smells like a possible recurring bug... Yup -
samples/kfifo/bytestream-example.c:138: ret = kfifo_to_user(&test, buf, count, &copied);samples/kfifo/inttype-example.c:131: ret = kfifo_to_user(&test, buf, count, &copied);samples/kfifo/record-example.c:145: ret = kfifo_to_user(&test, buf, count, &copied);
All three are exactly like that one.
Quoted 12 lines · Al Viro
Quoted 10 lines · Dan Carpenter
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Quoted 8 lines
Hmm... Let's grep for
kfifo_to_user()- smells like a possible recurring bug... Yup -
samples/kfifo/bytestream-example.c:138: ret =kfifo_to_user(&test, buf, count, &copied);samples/kfifo/inttype-example.c:131: ret =kfifo_to_user(&test, buf, count, &copied);samples/kfifo/record-example.c:145: ret =kfifo_to_user(&test, buf, count, &copied);All three are exactly like that one.
Nevermind, you've already caught and posted that bunch. Sorry for noise...
Quoted 9 lines · Dan Carpenter
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.
Quoted 5 lines
Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>
Quoted 23 lines
--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +3
-2@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret; -return ret ? ret : copied; +return copied; } static __poll_t snoop_file_poll(struct file *file, --2.30.2
From: Dan Carpenter
Quoted 24 lines
Sent: 22 April 2021 10:12
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +0
-0
drivers/soc/aspeed/aspeed-lpc-snoop.c static ssize_t snoop_file_read(struct file *file, char __user *buffer, · line 100
Quoted 8 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c +3
-1@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret; -return ret ? ret : copied; +return copied;
I wonder if changing it to:
return ret ? ret + 0L : copied;
Might make people think in the future and not convert it back as an 'optimisation'.
I much prefer adding 0 to a cast to fix integer types. In can go less wrong!
IMHO there are far too many casts in the kernel sources. Especially the ones that are only there to appease sparse. A functional notation for those would remove some of the potential problems.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Joel Stanley
maintainerQuoted 32 lines · David Laight
From: Dan Carpenter
Quoted 30 lines
Sent: 22 April 2021 10:12
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +2
-0@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret;
Quoted 8 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c static ssize_t snoop_file_read(struct file *file, char __user *buffer, · line 100
Quoted 2 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c +1
-1-return ret ? ret : copied; +return copied;I wonder if changing it to:
return ret ? ret + 0L : copied;Might make people think in the future and not convert it back as an 'optimisation'.
I think the change that Dan posted is clear.
Thanks Dan! I'll get it queued up.
Cheers,
Joel
Quoted 31 lines · David Laight
From: Dan Carpenter
Quoted 30 lines
Sent: 22 April 2021 10:12
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +2
-0@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret;
Quoted 8 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c static ssize_t snoop_file_read(struct file *file, char __user *buffer, · line 100
Quoted 2 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c +1
-1-return ret ? ret : copied; +return copied;I wonder if changing it to:
return ret ? ret + 0L : copied;Might make people think in the future and not convert it back as an 'optimisation'.
This is from a Smatch test that Aur?lien Aptel requested. The test is pretty good quality with few false positives so I will push it soon.
If someone converts it back then I expect the checker will catch it.
regards,
dan carepnter
Quoted 31 lines · David Laight
From: Dan Carpenter
Quoted 30 lines
Sent: 22 April 2021 10:12
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +2
-0@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret;
Quoted 8 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c static ssize_t snoop_file_read(struct file *file, char __user *buffer, · line 100
Quoted 2 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c +1
-1-return ret ? ret : copied; +return copied;I wonder if changing it to:
return ret ? ret + 0L : copied;Might make people think in the future and not convert it back as an 'optimisation'.
It rather made me think: "what the heck is going on here?!"
Shouldn't it better be:
return ret ? ret : (long)copied;
or even:
return ret ?: (long)copied;
?
-- Sergey Organov
From: Sergey Organov
Quoted 46 lines
Sent: 23 April 2021 11:46
Quoted 31 lines · David Laight
From: Dan Carpenter
Quoted 30 lines
Sent: 22 April 2021 10:12
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +2
-0@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret;Quoted 8 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c static ssize_t snoop_file_read(struct file *file, char __user *buffer, · line 100
Quoted 2 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c +1
-1-return ret ? ret : copied; +return copied;I wonder if changing it to:
return ret ? ret + 0L : copied;Might make people think in the future and not convert it back as an 'optimisation'.
It rather made me think: "what the heck is going on here?!"
Quoted 7 lines
Shouldn't it better be:
return ret ? ret : (long)copied;
or even:
return ret ?: (long)copied;
Or change the return type to int ?
The problem is that ?: doesn't behave the way most people expect. The two possible values have to be converted to the same type.
Together with the broken decision of the original ANSI C committee to change from K&R's 'sign preserving' to 'value preserving' integer promotions causes grief here and elsewhere. (Not no mention breaking existing code!)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
as indepentent observer,
i would go for Dans solution:
ret = kfifo_to_user();
/* if an error occurs just return */
if (ret)
return ret;
/* otherwise return the copied number of bytes */
return copied;
there is no need for any deeper language knowledge,
jm2c
re,
wh
________________________________________
Von: David Laight <[email protected]>
Gesendet: Freitag, 23. April 2021 12:54:59
An: 'Sergey Organov'
Cc: 'Dan Carpenter'; Joel Stanley; Andrew Jeffery; Chia-Wei, Wang; Jae Hyun Yoo; John Wang; Brad Bishop; Patrick Venture; Benjamin Fair; Greg Kroah-Hartman; Robert Lippert; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org; kernel-janitors at vger.kernel.org
Betreff: RE: [PATCH] soc: aspeed: fix a ternary sign expansion bug
WARNUNG: Diese E-Mail kam von au?erhalb der Organisation. Klicken Sie nicht auf Links oder ?ffnen Sie keine Anh?nge, es sei denn, Sie kennen den/die Absender*in und wissen, dass der Inhalt sicher ist.
From: Sergey Organov
Quoted 46 lines
Sent: 23 April 2021 11:46
Quoted 31 lines · David Laight
From: Dan Carpenter
Quoted 30 lines
Sent: 22 April 2021 10:12
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +2
-0@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret;Quoted 8 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c static ssize_t snoop_file_read(struct file *file, char __user *buffer, · line 100
Quoted 2 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c +1
-1-return ret ? ret : copied; +return copied;I wonder if changing it to:
return ret ? ret + 0L : copied;Might make people think in the future and not convert it back as an 'optimisation'.
It rather made me think: "what the heck is going on here?!"
Quoted 7 lines
Shouldn't it better be:
return ret ? ret : (long)copied;or even:
return ret ?: (long)copied;
Or change the return type to int ?
The problem is that ?: doesn't behave the way most people expect. The two possible values have to be converted to the same type.
Together with the broken decision of the original ANSI C committee to change from K&R's 'sign preserving' to 'value preserving' integer promotions causes grief here and elsewhere. (Not no mention breaking existing code!)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Quoted 8 lines · Walter Harms
as indepentent observer,
i would go for Dans solution:ret =
kfifo_to_user();
/* if an error occurs just return */
if (ret)
return ret;
Quoted 5 lines
/* otherwise return the copied number of bytes */
return copied;
there is no need for any deeper language knowledge,
Yep, but this is not idiomatic C, so one looking at this code would tend to convert it back to ternary, and the actual problem here is that the type of 'copied' does not match the return type of the function.
-- Sergey Organov
From: Sergey Organov
Quoted 14 lines
Sent: 23 April 2021 15:40
Quoted 10 lines · Walter Harms
as indepentent observer,
i would go for Dans solution:ret =
kfifo_to_user();
/* if an error occurs just return */
if (ret)
return ret;/* otherwise return the copied number of bytes */
Quoted 7 lines
Quoted 3 lines
return copied;
there is no need for any deeper language knowledge,
Yep, but this is not idiomatic C, so one looking at this code would tend to convert it back to ternary, and the actual problem here is that the type of 'copied' does not match the return type of the function.
Actually changing the type of 'ret' to ssize_t is probably the safest change.
That works until someone tries to optimise out 'ret' by doing:
return kfifo_to_user(...) ?: count;
Or rattle through and remove the 'pass by reference' 'count' parameter from kfifo_to_user() in favour of returning the value the callers want.
I need to stop looking at this code :-)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Quoted 12 lines · Sergey Organov
Quoted 10 lines · Walter Harms
as indepentent observer,
i would go for Dans solution:ret =
kfifo_to_user();
/* if an error occurs just return */
if (ret)
return ret;/* otherwise return the copied number of bytes */
Quoted 8 lines
Quoted 3 lines
return copied;
there is no need for any deeper language knowledge,
Yep, but this is not idiomatic C, so one looking at this code would tend to convert it back to ternary, and the actual problem here is that the type of 'copied' does not match the return type of the function.
I help maintain drivers/staging. I would hope that no one would send us a patch like this because it's not a checkpatch or CodingStyle violation. But people have sent us these before and Greg NAKs them because he doesn't like ternaries. I NAK them because I like my success path kept separate from the failure path. I want the success path indented one tab and the failure path indented two tabs. I like when code is written ploddingly, without fanciness, or combining multiple things on one line.
Using a ternary in this context seems to me like it falls under the anti-pattern of "making the last call in a function weird". A lot of times people change from failure handling to success handling for the last function call.
err = one();
if (err)
goto fail;
err = two();
if (err)
goto fail;
err = three();
if (!err)
return 0;
goto fail:
print("failed!\n");It seems crazy, but people do this all the time! It's fine to do:
return three();
There are some maintainers who insist that it should be:
err = three(); if (err) return err; return 0;
I don't go as far as that. But I also do like when I can glance at the function and there is a giant "return 0;" at the bottom.
Anyway, if people change it back to ternary then the kbuild bot will send them a warning message and they'll learn about an odd quirk in C's type promotion rules.
regards,
dan carpenter
Quoted 44 lines · Sergey Organov
Quoted 31 lines · David Laight
From: Dan Carpenter
Quoted 30 lines
Sent: 22 April 2021 10:12
The intent here was to return negative error codes but it actually returns positive values. The problem is that type promotion with ternary operations is quite complicated.
"ret" is an int. "copied" is a u32. And the
snoop_file_read()function returns long. What happens is that "ret" is cast to u32 and becomes positive then it's cast to long and it's still positive.Fix this by removing the ternary so that "ret" is type promoted directly to long.
Fixes 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc chardev")Signed-off-by Dan Carpenter <[email protected]>--- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)drivers/soc/aspeed/aspeed-lpc-snoop.c +2
-0@@ -94,8 +94,10 @@ static ssize_t snoop_file_read(struct file *file, char __user *buffer,return -EINTR ; } ret =kfifo_to_user (&chan->fifo, buffer, count, &copied); +if (ret) +return ret;Quoted 8 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c static ssize_t snoop_file_read(struct file *file, char __user *buffer, · line 100
Quoted 2 lines
drivers/soc/aspeed/aspeed-lpc-snoop.c +1
-1-return ret ? ret : copied; +return copied;I wonder if changing it to:
return ret ? ret + 0L : copied;Might make people think in the future and not convert it back as an 'optimisation'.
It rather made me think: "what the heck is going on here?!"
Quoted 7 lines
Shouldn't it better be:
return ret ? ret : (long)copied;
or even:
return ret ?: (long)copied;
I work with Greg a lot and his bias against ternaries has rubbed off a bit. They're sort of Perl-ish. And I have nothing against Perl. It's a perfectly fine programming language, but when I write Perl I write it in C.
regards,
dan carpenter