public inbox for linux-fsdevel@vger.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: NeilBrown <neil@brown.name>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
	Jeff Layton <jlayton@kernel.org>,
	Trond Myklebust <trondmy@kernel.org>,
	Anna Schumaker <anna@kernel.org>,
	Miklos Szeredi <miklos@szeredi.hu>,
	Amir Goldstein <amir73il@gmail.com>, Jeremy Kerr <jk@ozlabs.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	linux-efi@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-nfs@vger.kernel.org, linux-unionfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 03/19] VFS: allow d_alloc_name() to be used with ->d_hash
Date: Tue, 28 Apr 2026 03:10:56 +0100	[thread overview]
Message-ID: <20260428021056.GT3518998@ZenIV> (raw)
In-Reply-To: <20260427040517.828226-4-neilb@ownmail.net>

On Mon, Apr 27, 2026 at 02:01:21PM +1000, NeilBrown wrote:

> +/**
> + * d_alloc_name: allocate a dentry for use in a dcache-based filesystem.
> + * @parent: dentry of the parent for the dentry
> + * @name: name of the dentry
> + *
> + * d_alloc_name() allocates a dentry without any protection against
> + * races.  It should only be used in directories that do not support
> + * create/rename/link inode operations and so is particularly suited for

Contemplate

const struct inode_operations efivarfs_dir_inode_operations = {
        .lookup = simple_lookup,
	.unlink = efivarfs_unlink,
	.create = efivarfs_create,
};

in fs/efivarfs/inode.c, please.

The only reason efivarfs is still playing with d_alloc() rather than
using simple_start_creating()/simple_done_creating() (and believe me,
I had been very tempted to kill that weirdness) is that exclusion in
there is deeply weird.

It *has* ->create() (and ->unlink(), while we are at it).  Both are
using efivar_lock().  And efivarfs_create_dentry() is called by
their iterator callbacks - called under efivar_lock().  So we can't
use anything like the regular exclusion in that case or we would
deadlock... or, at least, scare the living hell out of lockdep.

In reality there is an exclusion of very irregular sort - and it's
not entirely correct, at that.  It's based upon the fs freeze levels,
of all things - calls of ->create() and ->unlink() can't overlap with
calls of ->unfreeze_fs(), which is where that shite comes from after
the filesystem had been mounted.

Only it's not quite enough - O_RDONLY open() can bloody well race with
->unfreeze_fs() and pick dentry before we hit
        inode_lock(inode);
	inode->i_private = entry;
	i_size_write(inode, size + sizeof(__u32)); /* attributes + data */
	inode_unlock(inode);
in efivarfs_create_dentry(), oopsing on
static int efivarfs_file_open(struct inode *inode, struct file *file)
{
        struct efivar_entry *entry = inode->i_private;
 
        file->private_data = entry;
 
        inode_lock(inode);
        entry->open_count++;
this.

When the only reason for your extension is something that flat-out violates
the conditions of use you are putting into comment in the same commit...

That crap needs to be straightened out, but I would rather not mix that
into your series.

  reply	other threads:[~2026-04-28  2:10 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-27  4:01 [PATCH v3 00/19] Prepare to lift lookup out of exclusive lock for directory ops NeilBrown
2026-04-27  4:01 ` [PATCH v3 01/19] VFS: fix various typos in documentation for start_creating start_removing etc NeilBrown
2026-04-27  4:01 ` [PATCH v3 02/19] VFS: enhance d_splice_alias() to handle in-lookup dentries NeilBrown
2026-04-27  4:01 ` [PATCH v3 03/19] VFS: allow d_alloc_name() to be used with ->d_hash NeilBrown
2026-04-28  2:10   ` Al Viro [this message]
2026-04-27  4:01 ` [PATCH v3 04/19] VFS: use wait_var_event for waiting in d_alloc_parallel() NeilBrown
2026-04-28  3:37   ` Al Viro
2026-04-27  4:01 ` [PATCH v3 05/19] VFS: introduce d_alloc_noblock() NeilBrown
2026-04-28  2:22   ` Al Viro
2026-04-27  4:01 ` [PATCH v3 06/19] VFS: add d_duplicate() NeilBrown
2026-04-27  4:01 ` [PATCH v3 07/19] VFS: Add LOOKUP_SHARED flag NeilBrown
2026-04-27  7:43   ` Amir Goldstein
2026-04-27  8:47     ` NeilBrown
2026-04-27  9:05       ` Amir Goldstein
2026-04-27 23:51         ` NeilBrown
2026-04-27  4:01 ` [PATCH v3 08/19] VFS/xfs/ntfs: drop parent lock across d_alloc_parallel() in d_add_ci() NeilBrown
2026-04-27  7:49   ` Amir Goldstein
2026-04-27  8:48     ` NeilBrown
2026-04-27  4:01 ` [PATCH v3 09/19] ovl: stop using lookup_one() in iterate_shared() handling NeilBrown
2026-04-27 10:10   ` Amir Goldstein
2026-04-28  0:24     ` NeilBrown
2026-04-27  4:01 ` [PATCH v3 10/19] VFS/ovl: add d_alloc_noblock_return() NeilBrown
2026-04-27  9:40   ` Amir Goldstein
2026-04-28  0:34     ` NeilBrown
2026-04-28  4:35       ` Al Viro
2026-04-27  4:01 ` [PATCH v3 11/19] efivarfs: use d_alloc_name() NeilBrown
2026-04-27  4:01 ` [PATCH v3 12/19] shmem: use d_duplicate() NeilBrown
2026-04-27  4:01 ` [PATCH v3 13/19] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open() NeilBrown
2026-04-27  4:01 ` [PATCH v3 14/19] nfs: use d_splice_alias() in nfs_link() NeilBrown
2026-04-27  4:01 ` [PATCH v3 15/19] nfs: don't d_drop() before d_splice_alias() NeilBrown
2026-04-27  4:01 ` [PATCH v3 16/19] nfs: don't d_drop() before d_splice_alias() in atomic_create NeilBrown
2026-04-27  4:01 ` [PATCH v3 17/19] nfs: Use d_alloc_noblock() in nfs_prime_dcache() NeilBrown
2026-04-27  4:01 ` [PATCH v3 18/19] nfs: use d_alloc_noblock() in silly-rename NeilBrown
2026-04-27  4:01 ` [PATCH v3 19/19] nfs: use d_duplicate() NeilBrown
2026-04-27  8:42 ` [syzbot ci] Re: Prepare to lift lookup out of exclusive lock for directory ops syzbot ci

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260428021056.GT3518998@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=amir73il@gmail.com \
    --cc=anna@kernel.org \
    --cc=ardb@kernel.org \
    --cc=brauner@kernel.org \
    --cc=jack@suse.cz \
    --cc=jk@ozlabs.org \
    --cc=jlayton@kernel.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    --cc=neil@brown.name \
    --cc=torvalds@linux-foundation.org \
    --cc=trondmy@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox