* [PATCH] powerpc: signedness bug in update_flash_db()
@ 2018-10-01 16:44 Dan Carpenter
2018-10-01 18:22 ` christophe leroy
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Dan Carpenter @ 2018-10-01 16:44 UTC (permalink / raw)
To: Geoff Levand, Geert Uytterhoeven
Cc: kernel-janitors, Paul Mackerras, linuxppc-dev
The "count < sizeof(struct os_area_db)" comparison is type promoted to
size_t so negative values of "count" are treated as very high values and
we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker
warning.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/arch/powerpc/platforms/ps3/os-area.c b/arch/powerpc/platforms/ps3/os-area.c
index cdbfc5cfd6f3..f5387ad82279 100644
--- a/arch/powerpc/platforms/ps3/os-area.c
+++ b/arch/powerpc/platforms/ps3/os-area.c
@@ -664,7 +664,7 @@ static int update_flash_db(void)
db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
- if (count < sizeof(struct os_area_db)) {
+ if (count < 0 || count < sizeof(struct os_area_db)) {
pr_debug("%s: os_area_flash_write failed %zd\n", __func__,
count);
error = count < 0 ? count : -EIO;
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] powerpc: signedness bug in update_flash_db()
2018-10-01 16:44 [PATCH] powerpc: signedness bug in update_flash_db() Dan Carpenter
@ 2018-10-01 18:22 ` christophe leroy
2018-10-01 19:02 ` Dan Carpenter
2018-10-09 11:54 ` Michael Ellerman
2018-10-01 22:45 ` Geoff Levand
2018-10-15 4:01 ` Michael Ellerman
2 siblings, 2 replies; 7+ messages in thread
From: christophe leroy @ 2018-10-01 18:22 UTC (permalink / raw)
To: Dan Carpenter, Geoff Levand, Geert Uytterhoeven
Cc: Paul Mackerras, kernel-janitors, linuxppc-dev
Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
> The "count < sizeof(struct os_area_db)" comparison is type promoted to
> size_t so negative values of "count" are treated as very high values and
> we accidentally return success instead of a negative error code.
>
> This doesn't really change runtime much but it fixes a static checker
> warning.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/arch/powerpc/platforms/ps3/os-area.c b/arch/powerpc/platforms/ps3/os-area.c
> index cdbfc5cfd6f3..f5387ad82279 100644
> --- a/arch/powerpc/platforms/ps3/os-area.c
> +++ b/arch/powerpc/platforms/ps3/os-area.c
> @@ -664,7 +664,7 @@ static int update_flash_db(void)
> db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
>
> count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
> - if (count < sizeof(struct os_area_db)) {
> + if (count < 0 || count < sizeof(struct os_area_db)) {
Why not simply add a cast ? :
if (count < (ssize_t)sizeof(struct os_area_db)) {
Christophe
> pr_debug("%s: os_area_flash_write failed %zd\n", __func__,
> count);
> error = count < 0 ? count : -EIO;
>
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] powerpc: signedness bug in update_flash_db()
2018-10-01 18:22 ` christophe leroy
@ 2018-10-01 19:02 ` Dan Carpenter
2018-10-01 19:06 ` Dan Carpenter
2018-10-09 11:54 ` Michael Ellerman
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2018-10-01 19:02 UTC (permalink / raw)
To: christophe leroy
Cc: Geoff Levand, Geert Uytterhoeven, kernel-janitors, linuxppc-dev,
Paul Mackerras
On Mon, Oct 01, 2018 at 08:22:01PM +0200, christophe leroy wrote:
>
>
> Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
> > The "count < sizeof(struct os_area_db)" comparison is type promoted to
> > size_t so negative values of "count" are treated as very high values and
> > we accidentally return success instead of a negative error code.
> >
> > This doesn't really change runtime much but it fixes a static checker
> > warning.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/arch/powerpc/platforms/ps3/os-area.c b/arch/powerpc/platforms/ps3/os-area.c
> > index cdbfc5cfd6f3..f5387ad82279 100644
> > --- a/arch/powerpc/platforms/ps3/os-area.c
> > +++ b/arch/powerpc/platforms/ps3/os-area.c
> > @@ -664,7 +664,7 @@ static int update_flash_db(void)
> > db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
> > count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
> > - if (count < sizeof(struct os_area_db)) {
> > + if (count < 0 || count < sizeof(struct os_area_db)) {
>
> Why not simply add a cast ? :
>
> if (count < (ssize_t)sizeof(struct os_area_db)) {
>
There are so many ways to solve these and no accounting for taste. Do
you need me to resend or can you redo it yourself?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] powerpc: signedness bug in update_flash_db()
2018-10-01 19:02 ` Dan Carpenter
@ 2018-10-01 19:06 ` Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2018-10-01 19:06 UTC (permalink / raw)
To: christophe leroy
Cc: Geoff Levand, Geert Uytterhoeven, kernel-janitors, linuxppc-dev,
Paul Mackerras
On Mon, Oct 01, 2018 at 10:02:54PM +0300, Dan Carpenter wrote:
> On Mon, Oct 01, 2018 at 08:22:01PM +0200, christophe leroy wrote:
> >
> >
> > Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
> > > The "count < sizeof(struct os_area_db)" comparison is type promoted to
> > > size_t so negative values of "count" are treated as very high values and
> > > we accidentally return success instead of a negative error code.
> > >
> > > This doesn't really change runtime much but it fixes a static checker
> > > warning.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > diff --git a/arch/powerpc/platforms/ps3/os-area.c b/arch/powerpc/platforms/ps3/os-area.c
> > > index cdbfc5cfd6f3..f5387ad82279 100644
> > > --- a/arch/powerpc/platforms/ps3/os-area.c
> > > +++ b/arch/powerpc/platforms/ps3/os-area.c
> > > @@ -664,7 +664,7 @@ static int update_flash_db(void)
> > > db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
> > > count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
> > > - if (count < sizeof(struct os_area_db)) {
> > > + if (count < 0 || count < sizeof(struct os_area_db)) {
> >
> > Why not simply add a cast ? :
> >
> > if (count < (ssize_t)sizeof(struct os_area_db)) {
> >
>
> There are so many ways to solve these and no accounting for taste. Do
> you need me to resend or can you redo it yourself?
>
Btw, I just went on vacation, and I'm not going to be back until next
week.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: signedness bug in update_flash_db()
2018-10-01 18:22 ` christophe leroy
2018-10-01 19:02 ` Dan Carpenter
@ 2018-10-09 11:54 ` Michael Ellerman
1 sibling, 0 replies; 7+ messages in thread
From: Michael Ellerman @ 2018-10-09 11:54 UTC (permalink / raw)
To: christophe leroy, Dan Carpenter, Geoff Levand, Geert Uytterhoeven
Cc: kernel-janitors, Paul Mackerras, linuxppc-dev
christophe leroy <christophe.leroy@c-s.fr> writes:
> Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
>> The "count < sizeof(struct os_area_db)" comparison is type promoted to
>> size_t so negative values of "count" are treated as very high values and
>> we accidentally return success instead of a negative error code.
>>
>> This doesn't really change runtime much but it fixes a static checker
>> warning.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/arch/powerpc/platforms/ps3/os-area.c b/arch/powerpc/platforms/ps3/os-area.c
>> index cdbfc5cfd6f3..f5387ad82279 100644
>> --- a/arch/powerpc/platforms/ps3/os-area.c
>> +++ b/arch/powerpc/platforms/ps3/os-area.c
>> @@ -664,7 +664,7 @@ static int update_flash_db(void)
>> db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
>>
>> count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
>> - if (count < sizeof(struct os_area_db)) {
>> + if (count < 0 || count < sizeof(struct os_area_db)) {
>
> Why not simply add a cast ? :
>
> if (count < (ssize_t)sizeof(struct os_area_db)) {
The explicit check against 0 is much clearer IMO.
The original author and all reviewers since obviously didn't realise
that count was being implicitly cast, so fixing that with another cast
seems likely to just confuse people even more :)
cheers
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc: signedness bug in update_flash_db()
2018-10-01 16:44 [PATCH] powerpc: signedness bug in update_flash_db() Dan Carpenter
2018-10-01 18:22 ` christophe leroy
@ 2018-10-01 22:45 ` Geoff Levand
2018-10-15 4:01 ` Michael Ellerman
2 siblings, 0 replies; 7+ messages in thread
From: Geoff Levand @ 2018-10-01 22:45 UTC (permalink / raw)
To: Dan Carpenter, Geert Uytterhoeven
Cc: kernel-janitors, Paul Mackerras, linuxppc-dev
On 10/01/2018 09:44 AM, Dan Carpenter wrote:
> The "count < sizeof(struct os_area_db)" comparison is type promoted to
> size_t so negative values of "count" are treated as very high values and
> we accidentally return success instead of a negative error code.
>
> This doesn't really change runtime much but it fixes a static checker
> warning.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/arch/powerpc/platforms/ps3/os-area.c b/arch/powerpc/platforms/ps3/os-area.c
> index cdbfc5cfd6f3..f5387ad82279 100644
> --- a/arch/powerpc/platforms/ps3/os-area.c
> +++ b/arch/powerpc/platforms/ps3/os-area.c
> @@ -664,7 +664,7 @@ static int update_flash_db(void)
> db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
>
> count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
> - if (count < sizeof(struct os_area_db)) {
> + if (count < 0 || count < sizeof(struct os_area_db)) {
> pr_debug("%s: os_area_flash_write failed %zd\n", __func__,
> count);
> error = count < 0 ? count : -EIO;
>
Seems OK.
Acked-by: Geoff Levand <geoff@infradead.org>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: powerpc: signedness bug in update_flash_db()
2018-10-01 16:44 [PATCH] powerpc: signedness bug in update_flash_db() Dan Carpenter
2018-10-01 18:22 ` christophe leroy
2018-10-01 22:45 ` Geoff Levand
@ 2018-10-15 4:01 ` Michael Ellerman
2 siblings, 0 replies; 7+ messages in thread
From: Michael Ellerman @ 2018-10-15 4:01 UTC (permalink / raw)
To: Dan Carpenter, Geoff Levand, Geert Uytterhoeven
Cc: Paul Mackerras, kernel-janitors, linuxppc-dev
On Mon, 2018-10-01 at 16:44:58 UTC, Dan Carpenter wrote:
> The "count < sizeof(struct os_area_db)" comparison is type promoted to
> size_t so negative values of "count" are treated as very high values and
> we accidentally return success instead of a negative error code.
>
> This doesn't really change runtime much but it fixes a static checker
> warning.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Acked-by: Geoff Levand <geoff@infradead.org>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/014704e6f54189a203cc14c7c0bb41
cheers
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-15 4:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-10-01 16:44 [PATCH] powerpc: signedness bug in update_flash_db() Dan Carpenter
2018-10-01 18:22 ` christophe leroy
2018-10-01 19:02 ` Dan Carpenter
2018-10-01 19:06 ` Dan Carpenter
2018-10-09 11:54 ` Michael Ellerman
2018-10-01 22:45 ` Geoff Levand
2018-10-15 4:01 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).