[bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[Dan Carpenter]

Hello Darrick J. Wong,

The patch c809d7e948a1: "xfs: pass the goal of the incore inode walk
to xfs_inode_walk()" from Jun 1, 2021, leads to the following
Smatch static checker warning:

	fs/xfs/xfs_icache.c:52 xfs_icwalk_tag()
	warn: unsigned 'goal' is never less than zero.

fs/xfs/xfs_icache.c
    49 static inline unsigned int
    50 xfs_icwalk_tag(enum xfs_icwalk_goal goal)
    51 {
--> 52 	return goal < 0 ? XFS_ICWALK_NULL_TAG : goal;

This enum will be unsigned in GCC, so "goal" can't be negative.  Plus
we only pass 0-1 for goal (as far as Smatch can tell).

    53 }

regards,
dan carpenter

Re: [bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[Dave Chinner]

On Thu, Aug 12, 2021 at 09:42:22AM +0300, Dan Carpenter wrote:
> Hello Darrick J. Wong,
> 
> The patch c809d7e948a1: "xfs: pass the goal of the incore inode walk
> to xfs_inode_walk()" from Jun 1, 2021, leads to the following
> Smatch static checker warning:
> 
> 	fs/xfs/xfs_icache.c:52 xfs_icwalk_tag()
> 	warn: unsigned 'goal' is never less than zero.
> 
> fs/xfs/xfs_icache.c
>     49 static inline unsigned int
>     50 xfs_icwalk_tag(enum xfs_icwalk_goal goal)
>     51 {
> --> 52 	return goal < 0 ? XFS_ICWALK_NULL_TAG : goal;
> 
> This enum will be unsigned in GCC, so "goal" can't be negative.

I think this is incorrect. The original C standard defines enums as
signed integers, not unsigned. And according to the GCC manual
(section 4.9 Structures, Unions, Enumerations, and Bit-Fields)
indicates that C90 first defines the enum type to be compatible with
the declared values. IOWs, for a build using C89 like the kernel
does, enums should always be signed.

This enum is defined as:

enum xfs_icwalk_goal {
        /* Goals that are not related to tags; these must be < 0. */
        XFS_ICWALK_DQRELE       = -1,

        /* Goals directly associated with tagged inodes. */
        XFS_ICWALK_BLOCKGC      = XFS_ICI_BLOCKGC_TAG,
        XFS_ICWALK_RECLAIM      = XFS_ICI_RECLAIM_TAG,
};

i.e. the enum is defined to clearly contain negative values and so
GCC should be defining it as a signed integer regardless of the
version of C being used...

> Plus
> we only pass 0-1 for goal (as far as Smatch can tell).

Yup, smatch has definitely got that one wrong:

xfs_dqrele_all_inodes()
  xfs_icwalk(mp, XFS_ICWALK_DQRELE, &icw);
    xfs_icwalk_get_perag(.... XFS_ICWALK_DQRELE)
      xfs_icwalk_tag(... XFS_ICWALK_DQRELE, ...)

So this warning looks like an issue with smatch, not a bug in the
code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[Darrick J. Wong]

On Fri, Aug 13, 2021 at 07:40:48AM +1000, Dave Chinner wrote:
> On Thu, Aug 12, 2021 at 09:42:22AM +0300, Dan Carpenter wrote:
> > Hello Darrick J. Wong,
> > 
> > The patch c809d7e948a1: "xfs: pass the goal of the incore inode walk
> > to xfs_inode_walk()" from Jun 1, 2021, leads to the following
> > Smatch static checker warning:
> > 
> > 	fs/xfs/xfs_icache.c:52 xfs_icwalk_tag()
> > 	warn: unsigned 'goal' is never less than zero.
> > 
> > fs/xfs/xfs_icache.c
> >     49 static inline unsigned int
> >     50 xfs_icwalk_tag(enum xfs_icwalk_goal goal)
> >     51 {
> > --> 52 	return goal < 0 ? XFS_ICWALK_NULL_TAG : goal;
> > 
> > This enum will be unsigned in GCC, so "goal" can't be negative.
> 
> I think this is incorrect. The original C standard defines enums as
> signed integers, not unsigned. And according to the GCC manual
> (section 4.9 Structures, Unions, Enumerations, and Bit-Fields)
> indicates that C90 first defines the enum type to be compatible with
> the declared values. IOWs, for a build using C89 like the kernel
> does, enums should always be signed.
> 
> This enum is defined as:
> 
> enum xfs_icwalk_goal {
>         /* Goals that are not related to tags; these must be < 0. */
>         XFS_ICWALK_DQRELE       = -1,
> 
>         /* Goals directly associated with tagged inodes. */
>         XFS_ICWALK_BLOCKGC      = XFS_ICI_BLOCKGC_TAG,
>         XFS_ICWALK_RECLAIM      = XFS_ICI_RECLAIM_TAG,
> };
> 
> i.e. the enum is defined to clearly contain negative values and so
> GCC should be defining it as a signed integer regardless of the
> version of C being used...
> 
> > Plus
> > we only pass 0-1 for goal (as far as Smatch can tell).
> 
> Yup, smatch has definitely got that one wrong:
> 
> xfs_dqrele_all_inodes()
>   xfs_icwalk(mp, XFS_ICWALK_DQRELE, &icw);
>     xfs_icwalk_get_perag(.... XFS_ICWALK_DQRELE)
>       xfs_icwalk_tag(... XFS_ICWALK_DQRELE, ...)
> 
> So this warning looks like an issue with smatch, not a bug in the
> code...

...unless Dan is running smatch against for-next, which removes
XFS_ICWALK_DQRELE and thus allows for an unsigned type to back the enum?

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[Dave Chinner]

On Thu, Aug 12, 2021 at 03:41:33PM -0700, Darrick J. Wong wrote:
> On Fri, Aug 13, 2021 at 07:40:48AM +1000, Dave Chinner wrote:
> > On Thu, Aug 12, 2021 at 09:42:22AM +0300, Dan Carpenter wrote:
> > > Hello Darrick J. Wong,
> > > 
> > > The patch c809d7e948a1: "xfs: pass the goal of the incore inode walk
> > > to xfs_inode_walk()" from Jun 1, 2021, leads to the following
> > > Smatch static checker warning:
> > > 
> > > 	fs/xfs/xfs_icache.c:52 xfs_icwalk_tag()
> > > 	warn: unsigned 'goal' is never less than zero.
> > > 
> > > fs/xfs/xfs_icache.c
> > >     49 static inline unsigned int
> > >     50 xfs_icwalk_tag(enum xfs_icwalk_goal goal)
> > >     51 {
> > > --> 52 	return goal < 0 ? XFS_ICWALK_NULL_TAG : goal;
> > > 
> > > This enum will be unsigned in GCC, so "goal" can't be negative.
> > 
> > I think this is incorrect. The original C standard defines enums as
> > signed integers, not unsigned. And according to the GCC manual
> > (section 4.9 Structures, Unions, Enumerations, and Bit-Fields)
> > indicates that C90 first defines the enum type to be compatible with
> > the declared values. IOWs, for a build using C89 like the kernel
> > does, enums should always be signed.
> > 
> > This enum is defined as:
> > 
> > enum xfs_icwalk_goal {
> >         /* Goals that are not related to tags; these must be < 0. */
> >         XFS_ICWALK_DQRELE       = -1,
> > 
> >         /* Goals directly associated with tagged inodes. */
> >         XFS_ICWALK_BLOCKGC      = XFS_ICI_BLOCKGC_TAG,
> >         XFS_ICWALK_RECLAIM      = XFS_ICI_RECLAIM_TAG,
> > };
> > 
> > i.e. the enum is defined to clearly contain negative values and so
> > GCC should be defining it as a signed integer regardless of the
> > version of C being used...
> > 
> > > Plus
> > > we only pass 0-1 for goal (as far as Smatch can tell).
> > 
> > Yup, smatch has definitely got that one wrong:
> > 
> > xfs_dqrele_all_inodes()
> >   xfs_icwalk(mp, XFS_ICWALK_DQRELE, &icw);
> >     xfs_icwalk_get_perag(.... XFS_ICWALK_DQRELE)
> >       xfs_icwalk_tag(... XFS_ICWALK_DQRELE, ...)
> > 
> > So this warning looks like an issue with smatch, not a bug in the
> > code...
> 
> ...unless Dan is running smatch against for-next, which removes
> XFS_ICWALK_DQRELE and thus allows for an unsigned type to back the enum?

Ah, I didn't realise that had gone away in the quotaoff removal -
I've kinda had my head stuck in fixing the journal/log recovery
problems recently.  Thanks for pointing out something I missed.

FWIW, I just assumed it was a current TOT being checked because
c809d7e948a1 was introduced in 5.14-rc1 and that's the commit smatch
is, IMO, incorrectly blaming.  Commit 777eb1fa857e ("xfs: remove
xfs_dqrele_all_inodes") which is the one in for-next that removed
the XFS_ICWALK_DQRELE definition from the enum and so, under C90,
gcc will turn the enum from from signed to unsigned. But we still
build the kernel under C89, so it's not clear to me that the smatch
assertion is correct...

Perhaps there might be some improvements that can be made to smatch
to handle this better. Knowing what tree was being checked would
also help us here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[Dan Carpenter]

On Fri, Aug 13, 2021 at 07:40:48AM +1000, Dave Chinner wrote:
> On Thu, Aug 12, 2021 at 09:42:22AM +0300, Dan Carpenter wrote:
> > Hello Darrick J. Wong,
> > 
> > The patch c809d7e948a1: "xfs: pass the goal of the incore inode walk
> > to xfs_inode_walk()" from Jun 1, 2021, leads to the following
> > Smatch static checker warning:
> > 
> > 	fs/xfs/xfs_icache.c:52 xfs_icwalk_tag()
> > 	warn: unsigned 'goal' is never less than zero.
> > 
> > fs/xfs/xfs_icache.c
> >     49 static inline unsigned int
> >     50 xfs_icwalk_tag(enum xfs_icwalk_goal goal)
> >     51 {
> > --> 52 	return goal < 0 ? XFS_ICWALK_NULL_TAG : goal;
> > 
> > This enum will be unsigned in GCC, so "goal" can't be negative.
> 
> I think this is incorrect. The original C standard defines enums as
> signed integers, not unsigned. And according to the GCC manual
> (section 4.9 Structures, Unions, Enumerations, and Bit-Fields)
> indicates that C90 first defines the enum type to be compatible with
> the declared values. IOWs, for a build using C89 like the kernel
> does, enums should always be signed.
> 
> This enum is defined as:
> 
> enum xfs_icwalk_goal {
>         /* Goals that are not related to tags; these must be < 0. */
>         XFS_ICWALK_DQRELE       = -1,
> 
>         /* Goals directly associated with tagged inodes. */
>         XFS_ICWALK_BLOCKGC      = XFS_ICI_BLOCKGC_TAG,
>         XFS_ICWALK_RECLAIM      = XFS_ICI_RECLAIM_TAG,
> };
> 
> i.e. the enum is defined to clearly contain negative values and so
> GCC should be defining it as a signed integer regardless of the
> version of C being used...

You're analysis is correct, but I'm looking at a newer version of the
code and I blamed the wrong commit.  It should be commit 777eb1fa857e
("xfs: remove xfs_dqrele_all_inodes")
https://lore.kernel.org/linux-xfs/20210809065938.1199181-3-hch@lst.de/
That commit removes the "XFS_ICWALK_DQRELE       = -1," line which
changes the enum type from int to unsigned int.

So this suggests that we should just remove the check for negative
values.

regards,
dan carpenter


> 
> > Plus
> > we only pass 0-1 for goal (as far as Smatch can tell).
> 
> Yup, smatch has definitely got that one wrong:
> 
> xfs_dqrele_all_inodes()
>   xfs_icwalk(mp, XFS_ICWALK_DQRELE, &icw);
>     xfs_icwalk_get_perag(.... XFS_ICWALK_DQRELE)
>       xfs_icwalk_tag(... XFS_ICWALK_DQRELE, ...)
> 
> So this warning looks like an issue with smatch, not a bug in the
> code...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[Dan Carpenter]

On Fri, Aug 13, 2021 at 09:57:14AM +1000, Dave Chinner wrote:
> FWIW, I just assumed it was a current TOT being checked because
> c809d7e948a1 was introduced in 5.14-rc1 and that's the commit smatch
> is, IMO, incorrectly blaming.  Commit 777eb1fa857e ("xfs: remove
> xfs_dqrele_all_inodes") which is the one in for-next that removed
> the XFS_ICWALK_DQRELE definition from the enum and so, under C90,
> gcc will turn the enum from from signed to unsigned. But we still
> build the kernel under C89, so it's not clear to me that the smatch
> assertion is correct...

No, it's still unsigned with -std=gnu89.

#include <stdio.h>

enum num { ONE, TWO };

int main(void)
{
	enum num x;

	x = -1;
	if (x < 0)
		printf("signed\n");
	else
		printf("unsigned\n");

	return 0;
}

$ gcc -std=gnu89 test.c
$ ./a.out
unsigned
$

> 
> Perhaps there might be some improvements that can be made to smatch
> to handle this better. Knowing what tree was being checked would
> also help us here.

Yep.  My bad.

regards,
dan carpenter

Re: [bug report] xfs: pass the goal of the incore inode walk to xfs_inode_walk()

[Christoph Hellwig]

On Fri, Aug 13, 2021 at 10:38:12AM +0300, Dan Carpenter wrote:
> > 
> > i.e. the enum is defined to clearly contain negative values and so
> > GCC should be defining it as a signed integer regardless of the
> > version of C being used...
> 
> You're analysis is correct, but I'm looking at a newer version of the
> code and I blamed the wrong commit.  It should be commit 777eb1fa857e
> ("xfs: remove xfs_dqrele_all_inodes")
> https://lore.kernel.org/linux-xfs/20210809065938.1199181-3-hch@lst.de/
> That commit removes the "XFS_ICWALK_DQRELE       = -1," line which
> changes the enum type from int to unsigned int.
> 
> So this suggests that we should just remove the check for negative
> values.

Remove the check as in removing the XFS code: yes.  I just prepared a
patch for that.  As in remove the check in smach:  As usual these
kind of checks tend to find something fishy.  Be that real bugs,
dead code or just the need to document weirdness better.