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

[Dave Chinner <david@fromorbit.com>]

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