fusefs: Take mnt_ref when mounting

[arrowd]

Summary

Still not sure if this is correct. Putting printf("===> mnt_ref: %d\n", mp->mnt_ref);
right after the MNT_REL() call in fuse_vfsop_unmount() prints "3" for a successfull unmounting case.

If I remove the vfs_ref() call from the destructor, the unmounting after KILL hangs with mnt_ref=2.
Which makes me think that all this change is pointless as there should be no difference between 2 and 3
as "good" value for mnt_ref.

Test Plan

Unmounting with umount works, auto-unmounting after KILL works.

[kib]

No, this is not what I suggested. You do not need to change fuse_vfsop_unmount(), instead in fdata_destroy()

	if (fdata->mp != NULL) {
              if fdata->dataflags & FSESS_AUTO_UNMOUNT)
		dounmount(fdata->mp, MNT_FORCE, curthread);
	      else
                 mnt_rel(fdata-mp);
       }

modulo small details that you should figure out.

[arrowd]

You do not need to change fuse_vfsop_unmount()

Not sure how is this possible.

I put debugging printfs into the destructor and fuse_vfsop_unmount(). When I kill -KILL the daemon I see:

===> fdata_dtor
===> fdata_dtor, mnt_ref: 4
===> fuse_vfsop_unmount, ref: 3

The control first enters fdata_dtor, then reaches the code you suggested in the last comment, then enters dounmount and eventually reaches fuse_vfsop_unmount with mnt_ref: 3. This scenario works.

Now if I do proper umount /foo I end up with

===> fuse_vfsop_unmount, ref: 4
===> fdata_dtor

The dounmount -> fuse_vfsop_unmount path happens before the destructor and the refcount is too high, which makes fuse_vfsop_unmount to sleep in [mntref]. When fdata_dtor gets executed the fdata variable is already NULL, so it exits early.

I guess, your intentition was to make the destructor dereference that last reference so that fuse_vfsop_unmount would be able to proceed?

[kib]

In D55056#1258736, @arrowd wrote:

You do not need to change fuse_vfsop_unmount()

Not sure how is this possible.

I put debugging printfs into the destructor and fuse_vfsop_unmount(). When I kill -KILL the daemon I see:

===> fdata_dtor
===> fdata_dtor, mnt_ref: 4
===> fuse_vfsop_unmount, ref: 3

The control first enters fdata_dtor, then reaches the code you suggested in the last comment, then enters dounmount and eventually reaches fuse_vfsop_unmount with mnt_ref: 3. This scenario works.

Now if I do proper umount /foo I end up with

===> fuse_vfsop_unmount, ref: 4
===> fdata_dtor

The dounmount -> fuse_vfsop_unmount path happens before the destructor and the refcount is too high, which makes fuse_vfsop_unmount to sleep in [mntref]. When fdata_dtor gets executed the fdata variable is already NULL, so it exits early.

I guess, your intentition was to make the destructor dereference that last reference so that fuse_vfsop_unmount would be able to proceed?

My intent is to ensure that fdata destructor references the right mp. If it does not own the reference of fdata->mp, then what prevents mp from being unmounted and then reused for something else?

[asomers]

I'm not very confident in this area, but I think arrowd's latest patch should work. But to check for double-releases, he should add this patch to the test suite:

diff --git a/tests/sys/fs/fusefs/destroy.cc b/tests/sys/fs/fusefs/destroy.cc
index 0c8e2fd22a50..b3e7f8bc5fff 100644
--- a/tests/sys/fs/fusefs/destroy.cc
+++ b/tests/sys/fs/fusefs/destroy.cc
@@ -136,6 +136,17 @@ TEST_F(AutoUnmount, dup)
        assert_unmounted();
 }
 
+/* 
+ * When the -o auto_unmount option is in use the daemon can still be unmounted
+ * normally, too.  Nothing bad should happen.
+ */
+TEST_F(AutoUnmount, unmount)
+{
+       expect_destroy(0);
+
+       m_mock->unmount();
+}
+
 /*
  * The server dies with unsent operations still on the message queue.
  * Check for any memory leaks like this:

[kib]

In D55056#1260915, @asomers wrote:

I'm not very confident in this area, but I think arrowd's latest patch should work. But to check for double-releases, he should add this patch to the test suite:

I have no idea what is double-release. My concern is that the pointer to mp, stored somewhere in fusefs data, gets reused for other mount.
[struct mounts are type-safe, they are never freed to work around some complicated parts of VFS, one of which is VOP_GETWRITEMOUNT()].

[arrowd]

My concern is that the pointer to mp, stored somewhere in fusefs data, gets reused for other mount.

How could this happen? My understanding is that it can't happen until we unmount and the unmounting may happen for two reasons:

• User called umount
• A FUSE process terminates abnormally, but has passed -o auto_unmount.

In both cases we perform the unmounting and then set fdata = NULL. So, for example, if the daemon first performs a proper umount and then dies due to SIGKILL, we won't touch fdata->mp in the destructor.

[asomers]

I have no idea what is double-release.

Calling vfs_rel twice on the same struct mp, or something like that.

In D55056#1261107, @arrowd wrote:

My concern is that the pointer to mp, stored somewhere in fusefs data, gets reused for other mount.

How could this happen? My understanding is that it can't happen until we unmount and the unmounting may happen for two reasons:

• User called umount
• A FUSE process terminates abnormally, but has passed -o auto_unmount.

In both cases we perform the unmounting and then set fdata = NULL. So, for example, if the daemon first performs a proper umount and then dies due to SIGKILL, we won't touch fdata->mp in the destructor.

I too can't find a way for Konstantin's fear to happen. But keeping the mountpoint reference is still a good defensive coding practice.

[kib]

In D55056#1261167, @asomers wrote:

I have no idea what is double-release.

Calling vfs_rel twice on the same struct mp, or something like that.

In D55056#1261107, @arrowd wrote:

My concern is that the pointer to mp, stored somewhere in fusefs data, gets reused for other mount.

How could this happen? My understanding is that it can't happen until we unmount and the unmounting may happen for two reasons:

• User called umount
• A FUSE process terminates abnormally, but has passed -o auto_unmount.

In both cases we perform the unmounting and then set fdata = NULL. So, for example, if the daemon first performs a proper umount and then dies due to SIGKILL, we won't touch fdata->mp in the destructor.

I too can't find a way for Konstantin's fear to happen. But keeping the mountpoint reference is still a good defensive coding practice.

Lets consider several simpler cases first. Suppose that the fuse-owned fd is being closed, and the parallel unmount occurs. What e.g. prevents fuse_vfsop_unmount() to set fdata->mp to NULL, while fdata_dtor() to race with it, read non-NULL mp first and fall into the FSESS_AUTO_UNMOUNT path, then read fdata->mp once more and see it NULL? Then you are calling vfs_ref() or dounmount() on NULL.

Since I was forced to read fusefs code, I think there is another big issue, esp. if fuse mounts are allowed for non-root. fuse_vfsop_mount() calls devfs_get_cdevpriv() on the specified fd, but it does not check that the file returned from fget() belongs to fusefs. If that fd is not fuse, but happens to have some cdevpriv, fuse_vfsop_mount() operates on random memory.

That said, returning to the original question, and assuming that the proper synchronization between fuse_vfsop_unmount() and fdata_dtor() is provided (which is not currently). I suspect that the answer is that fuse_vfsop_unmount() is supposed to destroy cdevpriv data for this mount, and there is at most one cdevpriv data for the mount (is this right?). If all of that is true, then I would agree that at worst dounmount() is called by dtor on already unmounted mp, but this mp cannot be released and reused until unmount finished.