[PATCH] soc: aspeed: fix a ternary sign expansion bug

[PATCH] soc: aspeed: fix a ternary sign expansion bug

[Joel Stanley]

From: Dan Carpenter <dan.carpenter@oracle.com>

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>
Link: https://lore.kernel.org/r/YIE90PSXsMTa2Y8n@mwanda
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
Soc maintainers, can you please apply this fix.

 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

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[patchwork-bot+linux-soc]

Hello:

This patch was applied to soc/soc.git (refs/heads/for-next):

On Fri, 23 Apr 2021 09:39:19 +0930 you wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> 
> 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.
> 
> [...]

Here is the summary with links:
  - soc: aspeed: fix a ternary sign expansion bug
    https://git.kernel.org/soc/soc/c/d42805807be7

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[Dan Carpenter]

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

RE: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[David Laight]

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)

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[Sergey Organov]

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

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[Dan Carpenter]

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

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[Joel Stanley]

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

RE: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[David Laight]

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)

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[Patrick Venture]

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
>

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[Al Viro]

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

Re: [PATCH] soc: aspeed: fix a ternary sign expansion bug

[Al Viro]

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.

[PATCH] soc: aspeed: fix a ternary sign expansion bug

[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.

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;
 }
 
 static __poll_t snoop_file_poll(struct file *file,
-- 
2.30.2