From linux-kernel Fri Apr 23 11:03:30 2021 From: Walter Harms Date: Fri, 23 Apr 2021 11:03:30 +0000 To: linux-kernel Subject: AW: [PATCH] soc: aspeed: fix a ternary sign expansion bug Message-Id: X-MARC-Message: https://marc.info/?l=linux-kernel&m=161917627500970 as indepentent observer, i would go for Dans solution: ret =3D 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 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-Ha= rtman; Robert Lippert; linux-aspeed@lists.ozlabs.org; linux-kernel@vger.ker= nel.org; kernel-janitors@vger.kernel.org Betreff: RE: [PATCH] soc: aspeed: fix a ternary sign expansion bug WARNUNG: Diese E-Mail kam von au=DFerhalb der Organisation. Klicken Sie nic= ht auf Links oder =F6ffnen Sie keine Anh=E4nge, 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 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() functi= on > >> 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 direct= ly > >> to long. > >> > >> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc= chardev") > >> Signed-off-by: Dan Carpenter > >> --- > >> 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/aspee= d/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, c= har __user *buffer, > >> return -EINTR; > >> } > >> ret =3D 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 1= PT, UK Registration No: 1397386 (Wales)