linux-ext4 - Re: [PATCH] vfs: don't decrement i_nlink in d

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]

Message-ID: <20190215155604.GL32253@magnolia>
Date:   Fri, 15 Feb 2019 07:56:04 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
        dsterba@...e.com, Al Viro <viro@...iv.linux.org.uk>,
        Jan Kara <jack@...e.com>, Theodore Tso <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Jaegeuk Kim <jaegeuk@...nel.org>, yuchao0@...wei.com,
        Hugh Dickins <hughd@...gle.com>,
        Christoph Hellwig <hch@...radead.org>,
        Richard Weinberger <richard@....at>,
        Artem Bityutskiy <dedekind1@...il.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Linux Btrfs <linux-btrfs@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Ext4 <linux-ext4@...r.kernel.org>,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-mtd@...ts.infradead.org, Linux MM <linux-mm@...ck.org>
Subject: Re: [PATCH] vfs: don't decrement i_nlink in d_tmpfile

On Fri, Feb 15, 2019 at 10:04:12AM +0200, Amir Goldstein wrote:
> On Fri, Feb 15, 2019 at 4:23 AM Darrick J. Wong <darrick.wong@...cle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@...cle.com>
> >
> > d_tmpfile was introduced to instantiate an inode in the dentry cache as
> > a temporary file.  This helper decrements the inode's nlink count and
> > dirties the inode, presumably so that filesystems could call new_inode
> > to create a new inode with nlink == 1 and then call d_tmpfile which will
> > decrement nlink.
> >
> > However, this doesn't play well with XFS, which needs to allocate,
> > initialize, and insert a tempfile inode on its unlinked list in a single
> > transaction.  In order to maintain referential integrity of the XFS
> > metadata, we cannot have an inode on the unlinked list with nlink >= 1.
> >
> > XFS and btrfs hack around d_tmpfile's behavior by creating the inode
> > with nlink == 0 and then incrementing it just prior to calling
> > d_tmpfile, anticipating that it will be reset to 0.
> >
> > Everywhere else outside of d_tmpfile, it appears that nlink updates and
> > persistence is the responsibility of individual filesystems.  Therefore,
> > move the nlink decrement out of d_tmpfile into the callers, and require
> > that callers only pass in inodes with nlink already set to 0.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@...cle.com>
> > ---
> >  fs/btrfs/inode.c  |    8 --------
> >  fs/dcache.c       |    8 ++++++--
> >  fs/ext2/namei.c   |    2 +-
> >  fs/ext4/namei.c   |    1 +
> >  fs/f2fs/namei.c   |    1 +
> >  fs/minix/namei.c  |    2 +-
> >  fs/ubifs/dir.c    |    1 +
> >  fs/udf/namei.c    |    2 +-
> >  fs/xfs/xfs_iops.c |   13 ++-----------
> >  mm/shmem.c        |    1 +
> >  10 files changed, 15 insertions(+), 24 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 5c349667c761..bd189fc50f83 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -10382,14 +10382,6 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
> >         if (ret)
> >                 goto out;
> >
> > -       /*
> > -        * We set number of links to 0 in btrfs_new_inode(), and here we set
> > -        * it to 1 because d_tmpfile() will issue a warning if the count is 0,
> > -        * through:
> > -        *
> > -        *    d_tmpfile() -> inode_dec_link_count() -> drop_nlink()
> > -        */
> > -       set_nlink(inode, 1);
> >         d_tmpfile(dentry, inode);
> >         unlock_new_inode(inode);
> >         mark_inode_dirty(inode);
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index aac41adf4743..5fb4ecce2589 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -3042,12 +3042,16 @@ void d_genocide(struct dentry *parent)
> >
> >  EXPORT_SYMBOL(d_genocide);
> >
> > +/*
> > + * Instantiate an inode in the dentry cache as a temporary file.  Callers must
> > + * ensure that @inode has a zero link count.
> > + */
> >  void d_tmpfile(struct dentry *dentry, struct inode *inode)
> >  {
> > -       inode_dec_link_count(inode);
> >         BUG_ON(dentry->d_name.name != dentry->d_iname ||
> >                 !hlist_unhashed(&dentry->d_u.d_alias) ||
> > -               !d_unlinked(dentry));
> > +               !d_unlinked(dentry) ||
> > +               inode->i_nlink != 0);
> 
> You've just promoted i_nlink filesystem accounting error (which
> are not that rare) from WARN_ON() to BUG_ON(), not to mention
> Linus' objection to any use of BUG_ON() at all.
> 
> !hlist_unhashed is anyway checked again in d_instantiate().
> !d_unlinked is not a reason to break the machine.
> The name check is really not a reason to break the machine.
> Can probably make tmp name code conditional to WARN_ON().

Fair enough, I'll remove the redundant checks and downgrade that to a
WARN_ON, if nobody else objects....

--D

> Thanks,
> Amir.