linux-kernel - Re: [PATCH v3] xfs: introduce protection for drop nlink

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Sep 2023 08:44:51 +1000
From:   Dave Chinner <david@...morbit.com>
To:     cheng.lin130@....com.cn
Cc:     djwong@...nel.org, linux-xfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, jiang.yong5@....com.cn,
        wang.liang82@....com.cn, liu.dong3@....com.cn
Subject: Re: [PATCH v3] xfs: introduce protection for drop nlink

On Fri, Sep 15, 2023 at 05:50:56PM +0800, cheng.lin130@....com.cn wrote:
> > On Wed, Sep 13, 2023 at 05:44:45PM +0800, cheng.lin130@....com.cn wrote:
> > > From: Cheng Lin <cheng.lin130@....com.cn>
> > >
> > > When abnormal drop_nlink are detected on the inode,
> > > shutdown filesystem, to avoid corruption propagation.
> > >
> > > Signed-off-by: Cheng Lin <cheng.lin130@....com.cn>
> > > ---
> > >  fs/xfs/xfs_inode.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 9e62cc500..40cc106ae 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -919,6 +919,15 @@ xfs_droplink(
> > >      xfs_trans_t *tp,
> > >      xfs_inode_t *ip)
> > >  {
> > > +
> > > +    if (VFS_I(ip)->i_nlink == 0) {
> > > +        xfs_alert(ip->i_mount,
> > > +              "%s: Deleting inode %llu with no links.",
> > > +              __func__, ip->i_ino);
> > > +        tp->t_flags |= XFS_TRANS_DIRTY;
> > Marking the transaction dirty is not necessary.
> > Otherwise this seems fine.
> Another strategy: 
> Set nlink to an invalid value(like XFS_NLINK_PINNED), and
> Complete this transaction before shutdown fs. To make sure
> nlink not be zero. If the nlink of a directory are zero, it may
> be cleaned up.
> Is that appropriate?

No, all I'm asking you to do is drop dirtying of the transaction
from this patch because it is a) unnecessary and b) a layering
violation.

It is unnecessary because the transaction will almost always be
dirty before we get to xfs_droplink(). In the cases where it isn't
dirty (e.g. xfs_remove() on a directory) we explicitly check that
nlink == 2 before proceeding to call xfs_droplink(). Hence we can't
actually get to xfs_droplink() with a clean transaction, and so
marking it dirty here on underrun is unnecessary as returning an
error from xfs_droplink() will result in shutting down the
filesystem when the transaction is cancelled.

-Dave.
-- 
Dave Chinner
david@...morbit.com