| Message ID | YIE90PSXsMTa2Y8n@mwanda |
|---|---|
| State | New |
| Headers | show |
| Series | soc: aspeed: fix a ternary sign expansion bug | expand |
On Thu, Apr 22, 2021 at 12:11:44PM +0300, Dan Carpenter wrote: > 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.
On Thu, Apr 22, 2021 at 09:24:59AM +0000, Al Viro wrote: > On Thu, Apr 22, 2021 at 12:11:44PM +0300, Dan Carpenter wrote: > > 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. Nevermind, you've already caught and posted that bunch. Sorry for noise...
On Thu, Apr 22, 2021 at 2:12 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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 <dan.carpenter@oracle.com> Reviewed-by: Patrick Venture <venture@google.com> > --- > drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c > index 210455efb321..eceeaf8dfbeb 100644 > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c > @@ -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 > 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 <dan.carpenter@oracle.com> > --- > drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c > index 210455efb321..eceeaf8dfbeb 100644 > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c > @@ -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)
On Thu, 22 Apr 2021 at 16:21, David Laight <David.Laight@aculab.com> wrote: > > From: Dan Carpenter > > 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 <dan.carpenter@oracle.com> > > --- > > drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c > > index 210455efb321..eceeaf8dfbeb 100644 > > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c > > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c > > @@ -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 think the change that Dan posted is clear. Thanks Dan! I'll get it queued up. Cheers, Joel
On Thu, Apr 22, 2021 at 04:21:40PM +0000, David Laight wrote: > From: Dan Carpenter > > 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 <dan.carpenter@oracle.com> > > --- > > drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c > > index 210455efb321..eceeaf8dfbeb 100644 > > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c > > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c > > @@ -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'. 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
David Laight <David.Laight@ACULAB.COM> writes: > From: Dan Carpenter >> 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 <dan.carpenter@oracle.com> >> --- >> drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c >> index 210455efb321..eceeaf8dfbeb 100644 >> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c >> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c >> @@ -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'. 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 > Sent: 23 April 2021 11:46 > > David Laight <David.Laight@ACULAB.COM> writes: > > > From: Dan Carpenter > >> 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 <dan.carpenter@oracle.com> > >> --- > >> drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c > >> index 210455efb321..eceeaf8dfbeb 100644 > >> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c > >> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c > >> @@ -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'. > > 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; 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
On Fri, Apr 23, 2021 at 01:45:40PM +0300, Sergey Organov wrote: > David Laight <David.Laight@ACULAB.COM> writes: > > > From: Dan Carpenter > >> 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 <dan.carpenter@oracle.com> > >> --- > >> drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c > >> index 210455efb321..eceeaf8dfbeb 100644 > >> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c > >> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c > >> @@ -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'. > > 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; 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
Walter Harms <wharms@bfs.de> writes: > 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, 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 > Sent: 23 April 2021 15:40 > > Walter Harms <wharms@bfs.de> writes: > > > 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, > > 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)
On Fri, Apr 23, 2021 at 05:40:19PM +0300, Sergey Organov wrote: > Walter Harms <wharms@bfs.de> writes: > > > 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, > > 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
diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c index 210455efb321..eceeaf8dfbeb 100644 --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c @@ -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,
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 <dan.carpenter@oracle.com> --- drivers/soc/aspeed/aspeed-lpc-snoop.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)