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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 15 Feb 2019 10:04:12 +0200 From: Amir Goldstein <amir73il@...il.com> To: "Darrick J. Wong" <darrick.wong@...cle.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 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(). Thanks, Amir.
Powered by blists - more mailing lists