linux-kernel - Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAKUZ0zKWDVocdSa60ZZPjq9u24wEW+EaUsXoUrrCF=Z+pacGHQ@mail.gmail.com>
Date: Tue, 8 Apr 2025 03:15:00 -0400
From: Gabriel Shahrouzi <gshahrouzi@...il.com>
To: Coly Li <colyli@...nel.org>
Cc: Kent Overstreet <kent.overstreet@...ux.dev>, linux-bcache@...r.kernel.org, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, 
	kernelmentees@...ts.linuxfoundation.org
Subject: Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

On Tue, Apr 8, 2025 at 12:58 AM Coly Li <colyli@...nel.org> wrote:
>
> On Mon, Apr 07, 2025 at 11:33:22PM +0800, Gabriel Shahrouzi wrote:
> > Remove unnecessary cpu_to_le16() and cpu_to_le32() conversions when
> > assigning values (priorities, timestamps) to native integer type
> > members. Prevent incorrect byte ordering for big-endian systems.
> >
>
> Hmm, why do you feel the conversions are unncessary? Please explain
> with details.
I used Sparse for static analysis on bcache and it gave incorrect type
in assignment warnings.

For example:

u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());

ktime_get_real_seconds() returns back u64 and gets casted down to a
u32. u is of type struct uuid_entry whose member fields are either u8,
u32, or u64. A conversion here contradicts the type it should be
assigned.

>From my understanding, this would not produce an unexpected result if
the value were to be read from or written to some location which seems
to be the case here. I believe it would only cause issues on
big-endian systems if the value were to be modified in some way.

Looking at the commit history for when the code for this specific
example was first introduced (12 years ago), it seems like this was
the author’s intent. It looks like the intention was to store the
value as little endian in uint32_t. Doing this, the author saves space
/ time. If the type was le32 instead, the conversion would have to be
applied each time it’s used. Alternatively, if another member variable
was defined but for the le32 version, then extra space is used up.

In the unlikely event that these specific files change drastically,
making sure the types are the same serves as a preventative measure
to make sure it’s not misused. On the other hand, making the change
most likely goes against the author’s original intent and could cause
something unintended.
>
> I don't mean the modification is correct or incorrect, just want to
> see detailed analysis and help me understand in correct why as you
> are.
>
> BTW, did you have chance to test your patch on big-endian machine?
I only analyzed the compilation warnings so far. I’ll look into trying
to test this on a big-endian machine.


>
> Thanks.
>
>
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@...il.com>
> > ---
> >  drivers/md/bcache/super.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index e42f1400cea9d..c4c5ca17fb600 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -648,7 +648,7 @@ int bch_prio_write(struct cache *ca, bool wait)
> >               for (b = ca->buckets + i * prios_per_bucket(ca);
> >                    b < ca->buckets + ca->sb.nbuckets && d < end;
> >                    b++, d++) {
> > -                     d->prio = cpu_to_le16(b->prio);
> > +                     d->prio = b->prio;
> >                       d->gen = b->gen;
> >               }
> >
> > @@ -721,7 +721,7 @@ static int prio_read(struct cache *ca, uint64_t bucket)
> >                       d = p->data;
> >               }
> >
> > -             b->prio = le16_to_cpu(d->prio);
> > +             b->prio = d->prio;
> >               b->gen = b->last_gc = d->gen;
> >       }
> >
> > @@ -832,7 +832,7 @@ static void bcache_device_detach(struct bcache_device *d)
> >
> >               SET_UUID_FLASH_ONLY(u, 0);
> >               memcpy(u->uuid, invalid_uuid, 16);
> > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > +             u->invalidated = (u32)ktime_get_real_seconds();
> >               bch_uuid_write(d->c);
> >       }
> >
> > @@ -1188,7 +1188,7 @@ void bch_cached_dev_detach(struct cached_dev *dc)
> >  int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> >                         uint8_t *set_uuid)
> >  {
> > -     uint32_t rtime = cpu_to_le32((u32)ktime_get_real_seconds());
> > +     uint32_t rtime = (u32)ktime_get_real_seconds();
> >       struct uuid_entry *u;
> >       struct cached_dev *exist_dc, *t;
> >       int ret = 0;
> > @@ -1230,7 +1230,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> >           (BDEV_STATE(&dc->sb) == BDEV_STATE_STALE ||
> >            BDEV_STATE(&dc->sb) == BDEV_STATE_NONE)) {
> >               memcpy(u->uuid, invalid_uuid, 16);
> > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > +             u->invalidated = (u32)ktime_get_real_seconds();
> >               u = NULL;
> >       }
> >
> > @@ -1591,7 +1591,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size)
> >
> >       get_random_bytes(u->uuid, 16);
> >       memset(u->label, 0, 32);
> > -     u->first_reg = u->last_reg = cpu_to_le32((u32)ktime_get_real_seconds());
> > +     u->first_reg = u->last_reg = (u32)ktime_get_real_seconds();
> >
> >       SET_UUID_FLASH_ONLY(u, 1);
> >       u->sectors = size >> 9;
> > --
> > 2.43.0
> >
>
> --
> Coly Li