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(-)
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
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 <[email protected]> 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 <[email protected]>
Reviewed-by: Patrick Venture <[email protected]>
> ---
> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
________________________________________
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; [email protected]; [email protected]; [email protected]
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
> Sent: 23 April 2021 11:46
>
> David Laight <[email protected]> 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 <[email protected]>
> >> ---
> >> 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)
On Fri, Apr 23, 2021 at 01:45:40PM +0300, Sergey Organov wrote:
> David Laight <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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