nullfs: Clear inotify flags during reclaim
ClosedPublic
Actions

Authored by markj on Sat, Apr 25, 9:21 PM.

Details

Reviewers

Commits

rGa02d794f5acd: nullfs: Clear inotify flags during reclaim

Summary

The inotify flags are copied from the lower vnode into the nullfs vnode
so that the INOTIFY() macro will invoke VOP_INOTIFY on the nullfs vnode;
this is then bypassed to the lower vnode. However, when a nullfs vnode
is reclaimed we should clear these flags, as the vnode is now doomed and
no longer forwards VOPs to the lower vnode.

Alernately we could add a dummy VOP_INOTIFY implementation to
dead_vnodeops, but I prefer this approach.

Add a regression test.

Fixes: f1f230439fa4 ("vfs: Initial revision of inotify")
PR: 292495
MFC after: 1 week

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 72550
Build 69433: arc lint + arc unit

Event Timeline

markj created this revision.Sat, Apr 25, 9:21 PM

Herald added a subscriber: imp. · View Herald TranscriptSat, Apr 25, 9:21 PM

markj requested review of this revision.Sat, Apr 25, 9:21 PM

Harbormaster completed remote builds in B72550: Diff 176514.Sat, Apr 25, 9:21 PM

What are the consequences of not clearing the flag? Does it trigger some undesired code path?

In D56639#1297195, @kib wrote:

What are the consequences of not clearing the flag? Does it trigger some undesired code path?

We call vop_stdinotify() on the nullfs vnode instead of the lower vnode. vop_stdinotify() assumes that the vnode's v_pollinfo is allocated, but in this case it is not.

In D56639#1297203, @markj wrote:

In D56639#1297195, @kib wrote:

What are the consequences of not clearing the flag? Does it trigger some undesired code path?

We call vop_stdinotify() on the nullfs vnode instead of the lower vnode. vop_stdinotify() assumes that the vnode's v_pollinfo is allocated, but in this case it is not.

The check for flag to ensure that pollinfo is allocated is fragile IMO. Why not check for v_pollinfo != NULL in vn_inotify() then, or even in inotify_log()? You do this in vn_inotify_revoke(), and I think that this is a better pattern.

Hm, do we have a bigger issue there? VOP_INOTIFY() does not require vnode lock, so what prevents the flag or vpollinfo check to become invalid right after? You need to deref the memory after the check, which means that it could be a freed memory.
Perhaps pollinfo should use non-freeable zone instead of plain malloc.

In D56639#1297207, @kib wrote:

In D56639#1297203, @markj wrote:

In D56639#1297195, @kib wrote:

What are the consequences of not clearing the flag? Does it trigger some undesired code path?

We call vop_stdinotify() on the nullfs vnode instead of the lower vnode. vop_stdinotify() assumes that the vnode's v_pollinfo is allocated, but in this case it is not.

The check for flag to ensure that pollinfo is allocated is fragile IMO. Why not check for v_pollinfo != NULL in vn_inotify() then, or even in inotify_log()? You do this in vn_inotify_revoke(), and I think that this is a better pattern.

I forgot about vn_inotify_revoke(). I don't really like this approach though: it has some potential to hide a bug where we call VOP_INOTIFY in situations where we shouldn't. And, if v_pollinfo is already allocated, we would lock it for now reason. Right now nullfs sets VIRF_INOTIFY and VIRF_INOTIFY_PARENT on its own vnodes (in null_bypass()), so IMO it should be responsible for clearing it too.

In D56639#1297218, @kib wrote:

Hm, do we have a bigger issue there? VOP_INOTIFY() does not require vnode lock, so what prevents the flag or vpollinfo check to become invalid right after? You need to deref the memory after the check, which means that it could be a freed memory.
Perhaps pollinfo should use non-freeable zone instead of plain malloc.

The inotify descriptor will hold a ref on watched vnodes. nullfs is special, so I prefer to keep the VIRF_* flag management there.

In D56639#1297223, @markj wrote:

In D56639#1297218, @kib wrote:

Hm, do we have a bigger issue there? VOP_INOTIFY() does not require vnode lock, so what prevents the flag or vpollinfo check to become invalid right after? You need to deref the memory after the check, which means that it could be a freed memory.
Perhaps pollinfo should use non-freeable zone instead of plain malloc.

The inotify descriptor will hold a ref on watched vnodes. nullfs is special, so I prefer to keep the VIRF_* flag management there.