Re: [PATCH] xfs: pin inodes that would otherwise overflow link count

[<cheng.lin130@zte.com.cn>]

> On Thu, Oct 12, 2023 at 08:08:20AM +1100, Dave Chinner wrote:
> > On Wed, Oct 11, 2023 at 01:33:50PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > The VFS inc_nlink function does not explicitly check for integer
> > > overflows in the i_nlink field.  Instead, it checks the link count
> > > against s_max_links in the vfs_{link,create,rename} functions.  XFS
> > > sets the maximum link count to 2.1 billion, so integer overflows should
> > > not be a problem.
> > >
> > > However.  It's possible that online repair could find that a file has
> > > more than four billion links, particularly if the link count got
> > > corrupted while creating hardlinks to the file.  The di_nlinkv2 field is
> > > not large enough to store a value larger than 2^32, so we ought to
> > > define a magic pin value of ~0U which means that the inode never gets
> > > deleted.  This will prevent a UAF error if the repair finds this
> > > situation and users begin deleting links to the file.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/libxfs/xfs_format.h |    6 ++++++
> > >  fs/xfs/scrub/nlinks.c      |    8 ++++----
> > >  fs/xfs/scrub/repair.c      |   12 ++++++------
> > >  fs/xfs/xfs_inode.c         |   28 +++++++++++++++++++++++-----
> > >  4 files changed, 39 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > > index 6409dd22530f2..320522b887bb3 100644
> > > --- a/fs/xfs/libxfs/xfs_format.h
> > > +++ b/fs/xfs/libxfs/xfs_format.h
> > > @@ -896,6 +896,12 @@ static inline uint xfs_dinode_size(int version)
> > >   */
> > >  #define    XFS_MAXLINK        ((1U << 31) - 1U)
> > >
> > > +/*
> > > + * Any file that hits the maximum ondisk link count should be pinned to avoid
> > > + * a use-after-free situation.
> > > + */
> > > +#define XFS_NLINK_PINNED    (~0U)
> > > +
> > >  /*
> > >   * Values for di_format
> > >   *
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 4db2c2a6538d6..30604e11182c4 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -910,15 +910,25 @@ xfs_init_new_inode(
> > >   */
> > >  static int            /* error */
> > >  xfs_droplink(
> > > -    xfs_trans_t *tp,
> > > -    xfs_inode_t *ip)
> > > +    struct xfs_trans    *tp,
> > > +    struct xfs_inode    *ip)
> > >  {
> > > +    struct inode        *inode = VFS_I(ip);
> > > +
> > >      xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
> > >
> > > -    drop_nlink(VFS_I(ip));
> > > +    if (inode->i_nlink == 0) {
> > > +        xfs_info_ratelimited(tp->t_mountp,
> > > + "Inode 0x%llx link count dropped below zero.  Pinning link count.",
> > > +                ip->i_ino);
> > > +        set_nlink(inode, XFS_NLINK_PINNED);
> > > +    }
> > > +    if (inode->i_nlink != XFS_NLINK_PINNED)
> > > +        drop_nlink(inode);
> > > +
> > >      xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >
> > I think the di_nlink field now needs to be checked by verifiers to
> > ensure the value is in the range of:
> >
> >     (0 <= di_nlink <= XFS_MAXLINKS || di_nlink == XFS_NLINK_PINNED)
> >
> > And we need to ensure that in xfs_bumplink() - or earlier (top avoid
> > dirty xaction cancle shutdowns) - that adding a count to di_nlink is
> > not going to exceed XFS_MAXLINKS....
> I think the VFS needs to check that unlinking a nondirectory won't
> underflow its link count, and that rmdiring an (empty) subdirectory
> won't underflow the link counts of the parent or child.
> Cheng Lin, would you please fix all the filesystems at once instead of
> just XFS?
As FS infrastructure, its change may have a significant impact.
> --D