Re: [PATCH] btrfs: use unsigned types for constants defined as bit shifts

[Boris Burkov <boris@bur.io>]

On Wed, May 07, 2025 at 07:43:28PM +0200, David Sterba wrote:
> On Wed, May 07, 2025 at 03:50:51PM +0200, Daniel Vacek wrote:
> > > --- a/fs/btrfs/raid56.c
> > > +++ b/fs/btrfs/raid56.c
> > > @@ -203,8 +203,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> > >         struct btrfs_stripe_hash_table *x;
> > >         struct btrfs_stripe_hash *cur;
> > >         struct btrfs_stripe_hash *h;
> > > -       int num_entries = 1 << BTRFS_STRIPE_HASH_TABLE_BITS;
> > > -       int i;
> > > +       unsigned int num_entries = 1U << BTRFS_STRIPE_HASH_TABLE_BITS;
> > 
> > This one does not really make much sense. It is an isolated local thing.
> > 
> > >         if (info->stripe_hash_table)
> > >                 return 0;
> > > @@ -225,7 +224,7 @@ int btrfs_alloc_stripe_hash_table(struct btrfs_fs_info *info)
> > >
> > >         h = table->table;
> > >
> > > -       for (i = 0; i < num_entries; i++) {
> > > +       for (unsigned int i = 0; i < num_entries; i++) {
> > 
> > I'd just do:
> > 
> > for (int i = 0; i < 1 << BTRFS_STRIPE_HASH_TABLE_BITS; i++) {
> > 
> > The compiler will resolve the shift and the loop will compare to the
> > immediate constant value.
> 
> Yes, it's a compile time constant. It's in num_entries because it's used
> twice in the function, for that we usually have a local variable so we
> don't have to open code the value everywhere.
> 
> > ---
> > 
> > Quite honestly the whole patch is questionable. The recommendations
> > are about right shifts. Left shifts are not prone to any sinedness
> > issues.
> > 
> > What is more important is where the constants are being used. They
> > should honor the type they are compared with or assigned to. Like for
> > example 0x80ULL for flags as these are usually declared unsigned long
> > long, and so on...
> 
> Agreed, flags and masks should be unsigned.
> 
> > For example the LINK_LOWER is converted to int when used as
> > btrfs_backref_link_edge(..., LINK_LOWER) parameter and then another
> > LINK_LOWER is and-ed to that int argument. I know, the logical
> > operations are not really influenced by the signedness, but still.
> 
> Well, it's more a matter of consistency and coding style. We did have a
> real bug with signed bit defined on 32bit int that got promoted to u64
> not as a single bit but 0xffffffff80000000 and this even got propagated
> to on-disk data. We were lucky it never had any real impact but since
> then I'm on the side of making bit shifts unconditionally on unsigned
> types. 77eea05e7851d910b7992c8c237a6b5d462050da
> 

For what it's worth, my support for moving to unsigned types for all
shifts is based on that same investigation.

I think a sign extension on s32->u64 is way more surprising and hard to
debug than a "whoops I thought -1 was less than 1 but actually it's
bigger", which is the first thing you would think of for weird
arithmetic errors.

> > 
> > And btw, the LINK_UPPER is not used at all anywhere in the code, if I
> > see correctly.
> 
> $ git grep LINK_UPPER
> backref.c:      if (link_which & LINK_UPPER)
> backref.h:#define               LINK_UPPER      (1U << 1)
> 
> > ---
> > 
> > In theory the situation could be even worse at some places as
> > incorrectly using an unsigned constant may force a signed variable to
> > get promoted to unsigned one to match. This may result in an int
> > variable with the value of -1 ending up as UINT_MAX instead. And now
> > imagine if(i < (1U << 4)) where int i = -1;
> > 
> > I did not check all the places where the constants you are changing
> > are being used, but it looks scary.
> 
> This is touching the core problem, mixing the signed and unsigned types
> in the wrong and very unobvious way. But we maybe disagree that it
> should be int, while I'd rather unify that to unsigned.
> 
> If you find a scary and potentially wrong use please send a RFC patch so
> we can see how much we can avoid that by changing that to a safer
> pattern.

I suppose the MOST convincing form of this patch would thoroughly audit
all the users/arithmetic involved to make sure they aren't doing
anything silly. I immediately noticed that LINK_UPPER/LINK_LOWER do get
passed as ints to btrfs_backref_link_edge() in a way that is fine to my
eye, but it wouldn't hurt to change that unsigned int too, the way you
did in a few other places?

I am still comfortable with this as-is.