vfs: introduce UMOUNT_WAIT which waits for umount completion

fs/namespace.c SYSCALL_DEFINE2(umount, char __user *, name, int, flags) · line 1667

Quoted 3 lines · Jaegeuk Kim

fs/namespace.c +3 -0

+	if (!retval && (flags & UMOUNT_WAIT)) {
+		if (likely(!(current->flags & PF_KTHREAD)))
+			task_work_run();

This is complete crap. The same damn thing will be done by caller of sys_umount() pretty much immediately afterwards. I'm not sure what it is that you are trying to paper over, but this is just plain wrong.

What _is_ the semantics of UMOUNT_WAIT? What does it guarantee, and what would be supplying it to umount(2)?

Hi Al,

Quoted 9 lines · Al Viro

fs/namespace.c SYSCALL_DEFINE2(umount, char __user *, name, int, flags) · line 1667

Quoted 3 lines · Jaegeuk Kim

fs/namespace.c +3 -0

+	if (!retval && (flags & UMOUNT_WAIT)) {
+		if (likely(!(current->flags & PF_KTHREAD)))
+			task_work_run();

This is complete crap. The same damn thing will be done by caller of sys_umount() pretty much immediately afterwards. I'm not sure what it is that you are trying to paper over, but this is just plain wrong.

Okay.

Quoted 2 lines

What _is_ the semantics of UMOUNT_WAIT? What does it guarantee, and what would be supplying it to umount(2)?

When android tries to reboot the system, it calls umount(2) without any flag. Then, mntput_no_expire() will add delayed_mntput_work() which finally does cleanup_mnt() later. In the mean time, android proceeded to shutdown all the UFS devices.

Quoted 18 lines · Jaegeuk Kim

Hi Al,

Quoted 9 lines · Al Viro

fs/namespace.c SYSCALL_DEFINE2(umount, char __user *, name, int, flags) · line 1667

Quoted 3 lines · Jaegeuk Kim

fs/namespace.c +3 -0

+	if (!retval && (flags & UMOUNT_WAIT)) {
+		if (likely(!(current->flags & PF_KTHREAD)))
+			task_work_run();

This is complete crap. The same damn thing will be done by caller of sys_umount() pretty much immediately afterwards. I'm not sure what it is that you are trying to paper over, but this is just plain wrong.

Okay.

Quoted 2 lines

What _is_ the semantics of UMOUNT_WAIT? What does it guarantee, and what would be supplying it to umount(2)?

Quoted 4 lines

When android tries to reboot the system, it calls umount(2) without any flag. Then, mntput_no_expire() will add delayed_mntput_work() which finally does cleanup_mnt() later. In the mean time, android proceeded to shutdown all the UFS devices.

Why has task_work_add() failed? Or is that umount(2) issued by a kernel thread?

Android triggers umount(2) by init process, which is definitely not a kernel thread. But, we've seen some kernel panics which say umount(2) was succeeded, but ext4 triggered a kernel panic due to EIO after then like below. I'm also not sure task_work_run() would be also safe enoughly. May I ask where I can find sys_umount() calls task_work_run()?

Quoted 5 lines · Jaegeuk Kim

Android triggers umount(2) by init process, which is definitely not a kernel thread. But, we've seen some kernel panics which say umount(2) was succeeded, but ext4 triggered a kernel panic due to EIO after then like below. I'm also not sure task_work_run() would be also safe enoughly. May I ask where I can find sys_umount() calls task_work_run()?

ret_{fast,slow}_syscall ->

	slow_work_pending ->
		do_work_pending() ->
			tracehook_notify_resume() ->
				task_work_run()

It's not sys_umount() (or any other sys_...()) - it's syscall dispatcher after having called one of those and before returning to userland. What is guaranteed is that after successful task_work_add() the damn thing will be run in context of originating process before it returns from syscall. So any subsequent syscalls from that process are guaranteed to happen after the work has run. The same happens if the process exits rather than returns to userland (do_exit() -> exit_task_work() -> task_work_run()), but for that you would need it to die in umount(2) (e.g. get kill -9 delivered on the way out).

Please, check if you are seeing task_work_add() failure in there and if you do, I would like to see a stack trace. IOW, slap WARN_ON(1); right after

                        if (!task_work_add(task, &mnt->mnt_rcu, true))
                                return;

and see what (if anything) gets printed.

Quoted 23 lines · Al Viro

Quoted 5 lines · Jaegeuk Kim

Android triggers umount(2) by init process, which is definitely not a kernel thread. But, we've seen some kernel panics which say umount(2) was succeeded, but ext4 triggered a kernel panic due to EIO after then like below. I'm also not sure task_work_run() would be also safe enoughly. May I ask where I can find sys_umount() calls task_work_run()?

ret_{fast,slow}_syscall ->

	slow_work_pending ->
		do_work_pending() ->
			tracehook_notify_resume() ->
				task_work_run()

It's not sys_umount() (or any other sys_...()) - it's syscall dispatcher after having called one of those and before returning to userland. What is guaranteed is that after successful task_work_add() the damn thing will be run in context of originating process before it returns from syscall. So any subsequent syscalls from that process are guaranteed to happen after the work has run. The same happens if the process exits rather than returns to userland (do_exit() -> exit_task_work() -> task_work_run()), but for that you would need it to die in umount(2) (e.g. get kill -9 delivered on the way out).

Quoted 5 lines

Please, check if you are seeing task_work_add() failure in there and if you do, I would like to see a stack trace. IOW, slap WARN_ON(1); right after

                        if (!task_work_add(task, &mnt->mnt_rcu, true))
                                return;

and see what (if anything) gets printed.

AFAICS, for task_work_add() to fail here we need a final mntput() to be run in context of a thread that already had exit_signals() run *and* subsequent task_work_run() run to completion (with all pending callbacks executed, along with all callbacks added by those, etc.)

For that to have happened during umount(2) we would've needed
* killing signal delivered while going through the syscall
* final mntput() to have been done *NOT* from sys_umount() (otherwise
the work would've been added before we got to exit_signals())
* final mntput() to have been done *NOT* from any task_work callbacks
(otherwise it would've been added before we'd observed a combination of empty list of pending work with PF_EXITING)

I really want to see the stack trace of that failing task_work_add(), if that's what actually happens there. What kind of a reproducer do you have for that?

Quoted 43 lines · Al Viro

Quoted 23 lines · Al Viro

Quoted 5 lines · Jaegeuk Kim

Android triggers umount(2) by init process, which is definitely not a kernel thread. But, we've seen some kernel panics which say umount(2) was succeeded, but ext4 triggered a kernel panic due to EIO after then like below. I'm also not sure task_work_run() would be also safe enoughly. May I ask where I can find sys_umount() calls task_work_run()?

ret_{fast,slow}_syscall ->

	slow_work_pending ->
		do_work_pending() ->
			tracehook_notify_resume() ->
				task_work_run()

It's not sys_umount() (or any other sys_...()) - it's syscall dispatcher after having called one of those and before returning to userland. What is guaranteed is that after successful task_work_add() the damn thing will be run in context of originating process before it returns from syscall. So any subsequent syscalls from that process are guaranteed to happen after the work has run. The same happens if the process exits rather than returns to userland (do_exit() -> exit_task_work() -> task_work_run()), but for that you would need it to die in umount(2) (e.g. get kill -9 delivered on the way out).

Quoted 5 lines

Please, check if you are seeing task_work_add() failure in there and if you do, I would like to see a stack trace. IOW, slap WARN_ON(1); right after

                        if (!task_work_add(task, &mnt->mnt_rcu, true))
                                return;

and see what (if anything) gets printed.

AFAICS, for task_work_add() to fail here we need a final mntput() to be run in context of a thread that already had exit_signals() run *and* subsequent task_work_run() run to completion (with all pending callbacks executed, along with all callbacks added by those, etc.)

For that to have happened during umount(2) we would've needed
* killing signal delivered while going through the syscall
* final mntput() to have been done *NOT* from sys_umount() (otherwise
the work would've been added before we got to exit_signals())
* final mntput() to have been done *NOT* from any task_work callbacks
(otherwise it would've been added before we'd observed a combination of empty list of pending work with PF_EXITING)

Quoted 2 lines

I really want to see the stack trace of that failing task_work_add(), if that's what actually happens there. What kind of a reproducer do you have for that?

I've got this error from Android user, so there's no reproducer unfortunately. So, I wrote a script capturing WARN_ON after reboot running at every minute, but couldn't have got the error since yesterday so far.

Instead, I put more traces in the reboot procedure, and got a clue to suspect the below flow.

delayed_fput() init
- umount
- mntput()
- mntput_no_expire() - mntput_no_expire()
- mnt_add_count(-1);
- mnt_get_count() return;
- return 0;
- mnt_add_count(-1);
- delayed_mntput_work
- device_shutdown
- ext4_put_super()
- EIO

Does this make any sense?

Quoted 16 lines · Jaegeuk Kim

Instead, I put more traces in the reboot procedure, and got a clue to suspect the below flow.

delayed_fput() init
- umount
- mntput()
- mntput_no_expire() - mntput_no_expire()
- mnt_add_count(-1);
- mnt_get_count() return;
- return 0;
- mnt_add_count(-1);
- delayed_mntput_work
- device_shutdown
- ext4_put_super()
- EIO

Quoted 1 line

Does this make any sense?

Which filesystem it is? With root I would've expected remount ro done by sys_umount(); with anything else... How has it managed to avoid -EBUSY? If it was umount -l (IOW, MNT_DETACH), I can see that happening, but... How would flushing prevent the scenario when the same opened file had remained open until after the umount(2) return?

In other words, where has that fput() come from and how had it managed to get past the umount(2)?

Quoted 5 lines

Which filesystem it is? With root I would've expected remount ro done by sys_umount(); with anything else... How has it managed to avoid -EBUSY? If it was umount -l (IOW, MNT_DETACH), I can see that happening, but... How would flushing prevent the scenario when the same opened file had remained open until after the umount(2) return?

It's ext4, and we use umount(0) and retry it several times if -EBUSY happens.

But, I don't see -EBUSY error in the log.

Quoted 2 lines

In other words, where has that fput() come from and how had it managed to get past the umount(2)?

Huge number of fput() were called by system drivers when init kills all the processes before umount(2). So, most of fput() were added in delayed_fput_list.

Then, it seems there is a race between delayed_fput() and umount(). Anyway, even after umount returns zero, it seems ext4's superblock is still alive and waiting for delayed_fput() which will finally call put_super.

Quoted 6 lines · Jaegeuk Kim

Quoted 5 lines

Which filesystem it is? With root I would've expected remount ro done by sys_umount(); with anything else... How has it managed to avoid -EBUSY? If it was umount -l (IOW, MNT_DETACH), I can see that happening, but... How would flushing prevent the scenario when the same opened file had remained open until after the umount(2) return?

Quoted 1 line

It's ext4, and we use umount(0) and retry it several times if -EBUSY happens.

????

umount(0) will result in EFAULT; what are you talking about?

Quoted 2 lines

But, I don't see -EBUSY error in the log.

Sorry, I'd been unclear - where is it mounted? Is that the root filesystem?

Quoted 5 lines

Quoted 2 lines

In other words, where has that fput() come from and how had it managed to get past the umount(2)?

Huge number of fput() were called by system drivers when init kills all the processes before umount(2). So, most of fput() were added in delayed_fput_list.

Umm... What do you mean by system drivers? If it was held by userland processes, then we are back to the same question - why has task_work_add() failed in fput()? If it had been kernel threads, which files had they been holding open?

Quoted 4 lines

Then, it seems there is a race between delayed_fput() and umount(). Anyway, even after umount returns zero, it seems ext4's superblock is still alive and waiting for delayed_fput() which will finally call put_super.

That might be more than one mount of the same fs (in different namespaces, for example) with umount taking out one of those, with the other having been hit with umount -l before that, with some opened files being the only thing that used to keep it alive.

I'd like to see /proc/1/mountinfo and fuser output, TBH... I'm not familiar enough with Android userland setup, so my apologies for dumb questions ;-/

So, I digged it in more detail, and found, in drivers/android/binder.c [1],
- binder_ioctl()
- create a kernel thread
- zombie_cleanup_check()
- binder_defer_work()
- queue_work(..., &binder_deferred_work);

- binder_deferred_func()
- binder_clear_zombies()
- binder_proc_clear_zombies()
- put_files_struct()
- close_files()
- filp_close()
- fput()

It seems binder holds some proc files.

Quoted 16 lines · Jaegeuk Kim

So, I digged it in more detail, and found, in drivers/android/binder.c [1],
- binder_ioctl()
- create a kernel thread
- zombie_cleanup_check()
- binder_defer_work()
- queue_work(..., &binder_deferred_work);

- binder_deferred_func()
- binder_clear_zombies()
- binder_proc_clear_zombies()
- put_files_struct()
- close_files()
- filp_close()
- fput()

Quoted 1 line

It seems binder holds some proc files.

If binder was holding some files open, then umount should have failed with EBUSY, no?

Does Android use mount namespaces at all?

- Ted

The mntput() in delayed_fput() is the last function call. So before that moment, sys_umount() may see mnt_get_count() as 2, so it avoids EBUSY condition. I'm not sure why it check over 2 tho.

Quoted 3 lines · Jaegeuk Kim

The mntput() in delayed_fput() is the last function call. So before that moment, sys_umount() may see mnt_get_count() as 2, so it avoids EBUSY condition. I'm not sure why it check over 2 tho.

Because it has just grabbed a reference itself, in addition to the one that keeps the damn thing alive (due to being mounted). So it bloody well should have triggered -EBUSY, if they refer to the same vfsmount.

Quoted 16 lines · Jaegeuk Kim

So, I digged it in more detail, and found, in drivers/android/binder.c [1],
- binder_ioctl()
- create a kernel thread
- zombie_cleanup_check()
- binder_defer_work()
- queue_work(..., &binder_deferred_work);

- binder_deferred_func()
- binder_clear_zombies()
- binder_proc_clear_zombies()
- put_files_struct()
- close_files()
- filp_close()
- fput()

Quoted 1 line

It seems binder holds some proc files.

If binder was holding some files open, then umount should have failed with EBUSY, no?

Does Android use mount namespaces at all?

This patch introduces UMOUNT_WAIT flag for umount(2) which let user wait for umount(2) to complete filesystem shutdown. This should fix a kernel panic triggered when a living filesystem tries to access dead block device after device_shutdown done by kernel_restart as below.

Quoted 4 lines · Jaegeuk Kim

This patch introduces UMOUNT_WAIT flag for umount(2) which let user wait for umount(2) to complete filesystem shutdown. This should fix a kernel panic triggered when a living filesystem tries to access dead block device after device_shutdown done by kernel_restart as below.

NAK. This is just papering over the race you've got; it does not fix it. You count upon the kernel threads in question having already gotten past scheduling delayed fput, but what's there to guarantee that? You are essentially adding a "flush all pending fput that had already been scheduled" syscall. It
a) doesn't belong in umount(2) and
b) doesn't fix the race.

It might change the timing enough to have your specific reproducer survive, but that kind of approach is simply wrong.
Incidentally, the name is a misnomer - it does *NOT* wait for completion of fs shutdown. Proof: have a filesystem mounted in two namespaces and issue that thing in one of them. Then observe how it's still alive, well and accessible in another.

The only case that gets affected by it is when another mount is heading for shutdown and is in a very specific part of that. That is waited for. If it's just before *OR* just past that stage, you are fucked.

And yes, "just past" is also affected. Look:
CPU1: delayed_fput()

        struct llist_node *node = llist_del_all(&delayed_fput_list);

delayed_fput_list() is empty now
llist_for_each_entry_safe(f, t, node, f_u.fu_llist)
__fput(f);
CPU2: your umount UMOUNT_WAIT
flush_delayed_fput()
does nothing, the list is empty

	....

flush_scheduled_work()
waits for delayed_fput() to finish
CPU1:

	finish __fput()
	call mntput() from it
	schedule_delayed_work(&delayed_mntput_work, 1);

CPU2:
OK, everything scheduled prior to call of flush_scheduled_work() is completed,
we are done.
return from umount(2)
(in bogus userland code) tell it to shut devices down
...
oops, that delayed_mntput_work we'd scheduled there got to run. Too bad...

Quoted 2 lines

flush_delayed_fput()
does nothing, the list is empty

how about waiting for workqueue completion here?

Quoted 1 line

	....

If all the __fput()s are not finished, do_umount() will return -EBUSY.

Quoted 7 lines · Jaegeuk Kim

Quoted 2 lines

flush_delayed_fput()
does nothing, the list is empty

how about waiting for workqueue completion here?

Quoted 1 line

	....

Quoted 1 line

If all the __fput()s are not finished, do_umount() will return -EBUSY.

Hell, no. That's only when they are all on the same vfsmount. And in that case you don't need any waiting - if any of those mntput() is not past the unlock_mount_hash() in mntput_no_expire(), you will get -EBUSY. And if they all are, the caller of umount(2) will end up dropping the last reference. In which case the shutdown will be scheduled via task_work_add() and processed before umount(2) returns to userland.

The whole problem is that you have several vfsmounts over the same filesystem (== same struct super_block), some of them held by kernel threads of yours. umount(2) doesn't affect those and isn't affected by those. What you do is, AFAICS,
ask the kernel threads to start shutting down
umount()
shut device down, hoping that all vfsmounts that used
to be held by those threads are gone by that point.

Your patch tries to stick "flush the pending work" in the umount(). With no warranty that it will catch that stuff in the stage where flushing will affect anything.

Quoted 5 lines

fs/file_table.c +5 -0

+void flush_delayed_fput_wait(void)
+{
+	delayed_fput(NULL);
+	flush_delayed_work(&delayed_fput_work);
+}

fs/namespace.c static void delayed_mntput(struct work_struct *unused) · line 1141

Quoted 5 lines

fs/namespace.c +5 -0

+void flush_delayed_mntput_wait(void)
+{
+	delayed_mntput(NULL);
+	flush_delayed_work(&delayed_mntput_work);
+}

It's still a broken approach. What I don't understand is why bother with that sort of brittle logics in the first place. Why not simply open the damn thing with O_EXCL before proceeding to device shutdown? And if you get "busy" from that, wait and retry...

Yes, and actually, android retries umount(2) for several seconds, if it gets failure. So, first I thought it'd be better to make umount() more deterministic.

I'm not sure how many times we can retry and wait for this. IMHO, it'd be better to use this together with the new flag, since this can detect unclosed namespace given successful umount(2).

Quoted 7 lines · Jaegeuk Kim

Yes, and actually, android retries umount(2) for several seconds, if it gets failure. So, first I thought it'd be better to make umount() more deterministic.

I'm not sure how many times we can retry and wait for this. IMHO, it'd be better to use this together with the new flag, since this can detect unclosed namespace given successful umount(2).

Even if you could make umount wait for the binder deferred workqueue to exit, if it takes time, it takes time. And that's not the core VFS's fault. It's binder's.

So... why not fix this by adding a binder ioctl which forces its workqueue to be flushed, and use that *before* you try calling mount?

If that binder ioctl takes minutes or hours to complete, then that's a binder bug, and needs to be fixed in binder.

- Ted