* Re: [PATCH] NFS: Fix directory delegation verifier checks
2026-04-04 18:32 ` [PATCH] NFS: Fix directory delegation verifier checks Al Viro
@ 2026-04-04 19:07 ` Al Viro
2026-04-05 2:39 ` Al Viro
1 sibling, 0 replies; 3+ messages in thread
From: Al Viro @ 2026-04-04 19:07 UTC (permalink / raw)
To: Trond Myklebust
Cc: anna@kernel.org, hch@infradead.org, linux-nfs@vger.kernel.org,
linux-fsdevel
On Sat, Apr 04, 2026 at 07:32:47PM +0100, Al Viro wrote:
> [cc to fsdevel added]
>
> On Wed, Dec 31, 2025 at 09:52:35PM +0000, Trond Myklebust wrote:
>
> > +static void nfs_clear_verifier_directory(struct inode *dir)
> > +{
> > + struct dentry *this_parent;
> > + struct dentry *dentry;
> > + struct inode *inode;
> > +
> > + if (hlist_empty(&dir->i_dentry))
> > + return;
> > + this_parent =
> > + hlist_entry(dir->i_dentry.first, struct dentry, d_u.d_alias);
> > +
> > + spin_lock(&this_parent->d_lock);
> > + nfs_unset_verifier_delegated(&this_parent->d_time);
> > + dentry = d_first_child(this_parent);
> > + hlist_for_each_entry_from(dentry, d_sib) {
> > + if (unlikely(dentry->d_flags & DCACHE_DENTRY_CURSOR))
> > + continue;
> > + inode = d_inode_rcu(dentry);
> > + if (inode &&
> > + NFS_PROTO(inode)->have_delegation(inode, FMODE_READ, 0))
> > + continue;
>
> What's to stop the inode from being freed right under you? You are *not*
> guaranteed to be holding rcu_read_lock(), unless I'm missing something
> in the call chain, so... what's going on here?
While we are at it, what are cursors doing on NFS directories? AFAICS,
you are not using simple_dir_operations there or anything that would
call dcache_dir_open(), for that matter. Confused...
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] NFS: Fix directory delegation verifier checks
2026-04-04 18:32 ` [PATCH] NFS: Fix directory delegation verifier checks Al Viro
2026-04-04 19:07 ` Al Viro
@ 2026-04-05 2:39 ` Al Viro
1 sibling, 0 replies; 3+ messages in thread
From: Al Viro @ 2026-04-05 2:39 UTC (permalink / raw)
To: Trond Myklebust
Cc: anna@kernel.org, hch@infradead.org, linux-nfs@vger.kernel.org,
linux-fsdevel
On Sat, Apr 04, 2026 at 07:32:47PM +0100, Al Viro wrote:
> [cc to fsdevel added]
>
> On Wed, Dec 31, 2025 at 09:52:35PM +0000, Trond Myklebust wrote:
>
> > +static void nfs_clear_verifier_directory(struct inode *dir)
> > +{
> > + struct dentry *this_parent;
> > + struct dentry *dentry;
> > + struct inode *inode;
> > +
> > + if (hlist_empty(&dir->i_dentry))
> > + return;
> > + this_parent =
> > + hlist_entry(dir->i_dentry.first, struct dentry, d_u.d_alias);
> > +
> > + spin_lock(&this_parent->d_lock);
> > + nfs_unset_verifier_delegated(&this_parent->d_time);
> > + dentry = d_first_child(this_parent);
> > + hlist_for_each_entry_from(dentry, d_sib) {
> > + if (unlikely(dentry->d_flags & DCACHE_DENTRY_CURSOR))
> > + continue;
> > + inode = d_inode_rcu(dentry);
> > + if (inode &&
> > + NFS_PROTO(inode)->have_delegation(inode, FMODE_READ, 0))
> > + continue;
>
> What's to stop the inode from being freed right under you? You are *not*
> guaranteed to be holding rcu_read_lock(), unless I'm missing something
> in the call chain, so... what's going on here?
Incidentally, nfs_clear_verifier_file() doesn't need to bother with
checking for NULL dir - alias is found in ->i_dentry of inode with
->i_lock held, which means it can't have progressed through
__dentry_kill() to dropping the reference to parent. So holding
alias->d_lock (which stabilizes ->d_parent) is enough - ->d_parent
is still pinned by alias and it can't have become negative.
"Can't have progressed through __dentry_kill()" is critical here -
that's the difference from nfs_set_verifier_locked() case.
Folks, this stuff can get seriously subtle; _please_ ask around on
fsdevel if you are doing something non-trivial with it.
^ permalink raw reply [flat|nested] 3+ messages in thread