Page MenuHomeFreeBSD

unionfs: reference the underlying FS' mount objects when using them
AbandonedPublic

Authored by jah on May 6 2021, 9:54 PM.

Details

Reviewers
markj
mjg
kib
Summary

Unionfs operations such as statfs() sometimes require access to
mount objects for the upper/lower filesystem layers. While the
upper/lower mount point vnodes will not be freed because the unionfs
mount object holds references to them, their v_mount fields will be
cleared if either underlying FS is forcibly unmounted. This will
result in a panic on any subsequent attempt to access the underlying
mount through a unionfs operation.

Instead, use vfs_ref_from_vp() to access the underlying mounts,
failing gracefully if vfs_ref_from_vp() returns NULL.

Found by running the unionfs stress2 test and then attempting to
execute 'df' after terminating the test.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 39183
Build 36072: arc lint + arc unit

Event Timeline

jah requested review of this revision.May 6 2021, 9:54 PM

A simpler approach would be to directly store the underlying mount objects in struct unionfs_mount, vfs_ref() them on mount and vfs_rel() them on unmount.
But that would have the effect of blocking forcible unmount of the underlying filesystems in vfs_mount_destroy(). Meanwhile, the underlying FS' vnodes would still be flushed before this happens so the underlying filesystems would be no more usable than they are with this approach.

Taking a reference on the mount point does not prevent unmount. For instance, calling VFS_QUOTACTL() requires busying mp, which, if successful, prevents unmount until unbusy. In fact, busying fs is currently enforced by VFS_QUOTACTL protocol since UFS (the only actual quotactl provider) in some cases unbusies and then busies the mp.

sys/fs/unionfs/union_vnops.c
722

mnt_flag is uint64_t, and reading it without mnt_ilock on 32bit arches is not guaranteed to be atomic.

799

I think there you can get away with reading uppervp->v_mount and storing it in local var. Then check the pointer for != NULL, and you can freely dereference it without taking a reference. struct mount is type-stable, i.e. it can be reused, but only for another mount. There I do not believe that this is critical.

In D30152#676937, @jah wrote:

A simpler approach would be to directly store the underlying mount objects in struct unionfs_mount, vfs_ref() them on mount and vfs_rel() them on unmount.
But that would have the effect of blocking forcible unmount of the underlying filesystems in vfs_mount_destroy(). Meanwhile, the underlying FS' vnodes would still be flushed before this happens so the underlying filesystems would be no more usable than they are with this approach.

In D30152#676980, @kib wrote:

Taking a reference on the mount point does not prevent unmount. For instance, calling VFS_QUOTACTL() requires busying mp, which, if successful, prevents unmount until unbusy. In fact, busying fs is currently enforced by VFS_QUOTACTL protocol since UFS (the only actual quotactl provider) in some cases unbusies and then busies the mp.

It certainly doesn't prevent the unmount operations (vflush, etc) from happening, but doesn't it prevent the unmount system call from returning? That's what I observed in testing, and is corroborated by reading vfs_mount_destroy().

sys/fs/unionfs/union_vnops.c
722

good point, will fix while here.

In D30152#677007, @jah wrote:
In D30152#676937, @jah wrote:

A simpler approach would be to directly store the underlying mount objects in struct unionfs_mount, vfs_ref() them on mount and vfs_rel() them on unmount.
But that would have the effect of blocking forcible unmount of the underlying filesystems in vfs_mount_destroy(). Meanwhile, the underlying FS' vnodes would still be flushed before this happens so the underlying filesystems would be no more usable than they are with this approach.

In D30152#676980, @kib wrote:

Taking a reference on the mount point does not prevent unmount. For instance, calling VFS_QUOTACTL() requires busying mp, which, if successful, prevents unmount until unbusy. In fact, busying fs is currently enforced by VFS_QUOTACTL protocol since UFS (the only actual quotactl provider) in some cases unbusies and then busies the mp.

It certainly doesn't prevent the unmount operations (vflush, etc) from happening, but doesn't it prevent the unmount system call from returning? That's what I observed in testing, and is corroborated by reading vfs_mount_destroy().

Why 'preventing syscall from returning' matters? What matters is that unmount destroys the private mount data pointed to by mnt_data. This data is usually vital for VFS_XXX() methods, and destroying/freeing it while these methods operate causes undefined behavior, a crash in the best case.

In D30152#677009, @kib wrote:
In D30152#677007, @jah wrote:
In D30152#676937, @jah wrote:

A simpler approach would be to directly store the underlying mount objects in struct unionfs_mount, vfs_ref() them on mount and vfs_rel() them on unmount.
But that would have the effect of blocking forcible unmount of the underlying filesystems in vfs_mount_destroy(). Meanwhile, the underlying FS' vnodes would still be flushed before this happens so the underlying filesystems would be no more usable than they are with this approach.

In D30152#676980, @kib wrote:

Taking a reference on the mount point does not prevent unmount. For instance, calling VFS_QUOTACTL() requires busying mp, which, if successful, prevents unmount until unbusy. In fact, busying fs is currently enforced by VFS_QUOTACTL protocol since UFS (the only actual quotactl provider) in some cases unbusies and then busies the mp.

It certainly doesn't prevent the unmount operations (vflush, etc) from happening, but doesn't it prevent the unmount system call from returning? That's what I observed in testing, and is corroborated by reading vfs_mount_destroy().

Why 'preventing syscall from returning' matters? What matters is that unmount destroys the private mount data pointed to by mnt_data. This data is usually vital for VFS_XXX() methods, and destroying/freeing it while these methods operate causes undefined behavior, a crash in the best case.

I only meant that a user would probably be surprised to see their 'umount -f' command hang here. But you're completely right, VFS_UNMOUNT is called well before vfs_mount_destroy(), so vfsops will require busying the mount.

sys/fs/unionfs/union_vnops.c
722

On second reading, I'm not sure that non-atomicity of mnt_flag matters here.
We only care about MNT_RDONLY here, so even a torn 64-bit load will either see the previous value or the updated value of the flag. Even without a torn load, the VOP_ACCESS check here seems inherently racy in the face of a concurrent update of the underlying mount.

If we don't care about MNT_ILOCK here, then we can rely on type-stability of struct mount to simplify this path as you note below for VOP_GETATTR. Otherwise, I don't think we can safely use the ilock mutex across mount recycling, so we'll need to both ref the mount and lock the ilock.

Apply vfs_busy, minor refactoring

Use atomic_load_ptr to enforce single load of vp->v_mount

kib added inline comments.
sys/fs/unionfs/union_vfsops.c
297

!= 0 there and next line

This revision is now accepted and ready to land.May 10 2021, 1:58 AM
This revision now requires review to proceed.May 10 2021, 4:51 PM
markj added inline comments.
sys/fs/unionfs/union_vfsops.c
410

Looking at ufs_quotactl(), there are some specific cases where it unbusies the mount point itself. See also sys_quotactl(). Don't we need similar special casing here?

sys/fs/unionfs/union_vnops.c
727
804
This revision is now accepted and ready to land.May 10 2021, 6:58 PM
sys/fs/unionfs/union_vfsops.c
410

Oh yeah, I suspect you're right. I made a mental note to dig into that and then promptly forgot; I'll figure out what I need to do here when I get some time tomorrow.

Follow the unbusy requirements for vfs_quotactl()

This revision now requires review to proceed.May 11 2021, 7:00 PM

Use correct variable when passing through VFS_QUOTACTL

Do you plan to revise this change after changing the VFS_QUOTACTL interface? I'm just not sure whether to wait until that's done before re-reading this.

Do you plan to revise this change after changing the VFS_QUOTACTL interface? I'm just not sure whether to wait until that's done before re-reading this.

Yes, sorry...I'm about to update D30218 and this PR together.

sys/fs/unionfs/union_subr.c
1295

Is it actually necessary to do the ref step here?
vfs_busy() appears to perform a superset of what vfs_ref() does, so it seems we'd be able to just use a simple atomic load of vp->v_mount followed by a NULL check and vfs_busy().

sys/fs/unionfs/union_vnops.c
804

Replace vfs_ref() with simple atomic load of v_mount