Fanotify (hsm or erofs) / fuse / nbd / ... + write(mmap) deadlock vector followup

[Gao Xiang <hsiangkao@linux.alibaba.com>]

Hi all,

Amir just suggested that I posted a long off-list discussion
on the list right now, but since it seems to relate to
`sb->s_writers.rw_sem` locking design, I am not quite sure
how deep I could help generally: But here it goes.

The background is that we have discussed a generic deadlock
timing for several months as below:

fsA is a filesystem which supports fsfreeze, such as EXT4/XFS/...

fsB is a filesystem which have some relationship with a
userspace daemon (e.g. a filesystem with fanotify HSM hooks /
   fanotify + EROFS file-backed mounts / a FUSE filesystem or
   a filesystem backed by a virtual block device)

Thread A                           Thread B                                 Userspace deamon
  write(fsA_fd, mmap(fsB_fd))
   file_start_write()
    (take SB_FREEZE_WRITE read lock)

    handle fsB mmap fault read
     -> notify userspace and wait

                                    freeze_super
                                    (try to take SB_FREEZE_WRITE write lock)
                                                                             received/handling fsB mmap read request
                                                                             (do random something...)
                                                                             write(fsA_fd2)
                                                                              (take SB_FREEZE_WRITE read lock)
The problem timing here is thread A does
`write(fsA_fd, mmap(fsB_fd))` => file_start_write() (rwsem read),
then hits page fault and wait for userspace deamon to finish
the request;

Thread B does fsfreeze on fsA so it is waiting a write lock
(sb_wait_write(), rwsem write) and blocked on thread A;

And the userspace deamon is a handler handling page fault,
and trying to write to another file (fsA_fd2) on fsA again
and blocked on thread B (file_start_write(), rwsem read).

because of the specific locking timing is `R->W->R`, at least
the whole workflow won't proceed so the related processes
(and fsA above) will be stuck.


Since the issue is complex, I can only give my own thought
from the perspective of fanotify+EROFS use cases on this:

  - fsfreeze is typically unneeded, especially on typical
    cloud + container environment;

  - Also we could isolate the image backing file into another
    totally different local filesystem so that
    "write(fd, mmap(),..)" + fsfreeze won't be a practical
    issue.

  - In the worst case, user programs can be killed to recover
    the system, and fanotify already supports this way, so
    it shouldn't be too harmful;

  - this vector impacts all userspace approaches.


I also gave my own preliminary idea at LSF to Jan and Amir
(just for reference):

diff --git a/fs/super.c b/fs/super.c
index 378e81efe643..2897d3572e9e 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -2069,6 +2069,7 @@ static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
   */
  int freeze_super(struct super_block *sb, enum freeze_holder who, const void *freeze_owner)
  {
+    bool retried;
      int ret;

      if (!super_lock_excl(sb)) {
@@ -2111,7 +2112,15 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
      sb->s_writers.frozen = SB_FREEZE_WRITE;
      /* Release s_umount to preserve sb_start_write -> s_umount ordering */
      super_unlock_excl(sb);
-    sb_wait_write(sb, SB_FREEZE_WRITE);
+    while (ret = sb_wait_write_timeout(sb, SB_FREEZE_WRITE)) {
+        if (retried) {
+            sb->s_writers.frozen = SB_UNFROZEN;
+            sb_freeze_unlock(sb, SB_FREEZE_FS);
+            deactivate_locked_super(sb);
+            return ret;
+        }
+        retried = true;
+    }
      __super_lock_excl(sb);

      /* Now we go and block page faults... */

It was simply to add a new sb_wait_write_timeout()
interface so that the writer (freeze_super) can be woken
up even when the locking order is already (R->W->R),
allowing the W (freeze_super) to be woken and the (R->R)
to be merged.

I wonder if this is a practical way since it allows
"write(fd, mmap(),..)" and workable for all approaches.
However, the main issue at least in my opinion is that
percpu_rwsem doesn't have percpu_down_write_timeout()
(but as for rwsem since down_write_killable() exists, I
think down_write_timeout() should be doable at least),
and I tried to vibe-code a version but have no idea if
it performs well.


Amir and other people may have other better ideas for
further discussion, but as Amir suggested, I've written
this...

Thanks,
Gao Xiang