Re: [GIT PULL] Detaching mounts on unlink for 3.15

[Al Viro <viro@ZenIV.linux.org.uk>]

On Sat, Apr 19, 2014 at 02:35:26AM +0100, Al Viro wrote:

> My apologies for confusion - I have not looked at your last commit.
> I *really* don't like that solution, but it probably does close that
> particular problem.  Consider that objection withdrawn (modulo "you
> have created a bisect hazard here, that part of series needs to be
> reordered").
> 
> I still don't think that this is the right approach.  Hell knows ;-/
> Maybe you are right about offloading auto-close to a wq...  The whole
> mnt_pin/mnt_unpin is bloody ugly, especially with multiple references
> held by kernel/acct.c-opened files ;-/

Actually, it's worse than ugly.  Consider the following race:
* umount(2) decides that victim isn't busy and does everything up to
the final mntput_no_expire().
* acct(NULL) is called, and gets to
        if (old_acct) {
                mnt_unpin(old_acct->f_path.mnt);
                spin_unlock(&acct_lock);   
                do_acct_process(acct, old_ns, old_acct);
                filp_close(old_acct, NULL);
                spin_lock(&acct_lock);
        }
It starts writing and blocks.  Now mnt_pinned is 0 and refcount is 1, so
mntput_no_expire() from umount(2) does not see anything untowards and just
returns.  Eventually, write finishes and acct(2) does filp_close().  Fine,
but by that point umount(2) has already returned to userland and we have
the problem I'd been complaining about in your solution.  And your patches
won't go anywhere near wait_for_completion() in that case, so they don't
close that hole either...

Not your fault, and not that scary wrt dirty shutdown, but it still needs
fixing.  While we are at it, consider what happens if something is busy
in acct_process() while we are hitting that race.  This filp_close()
will happen before the final fput(), so the actual fs shutdown is moved
to whatever exiting process that was in acct_process() at that point.
Might be more than one of those, even - then the last one to finish
writing will end up carrying the bucket.

Actually, even without umount(2) in the mix (just acct(NULL) vs. exiting
processes) it's somewhat fishy - we are, after all, calling ->flush()
before the final entries are written.

That, BTW, is another reason why "let's write one last entry on acct(NULL)"
is bogus - userland tools might hope to use that to get information about the
command that has stopped logging, but it is not guaranteed to be the last
one.