| Message ID | 20181001164458.GB29197@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 014704e6f54189a203cc14c7c0bb411b940241bc |
| Headers | show |
| Series | powerpc: signedness bug in update_flash_db() | expand |
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
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
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
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>
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
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
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;
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>