On 2024-11-21 23:05:51 -0300, Paulo Alcantara wrote:
Hi Paul,
Thanks for looking into this! Really appreciate it.
Paul Aurich <paul@xxxxxxxxxxxxxx> writes:
The unmount process (cifs_kill_sb() calling close_all_cached_dirs()) can
race with various cached directory operations, which ultimately results
in dentries not being dropped and these kernel BUGs:
BUG: Dentry ffff88814f37e358{i=1000000000080,n=/} still in use (2) [unmount of cifs cifs]
VFS: Busy inodes after unmount of cifs (cifs)
------------[ cut here ]------------
kernel BUG at fs/super.c:661!
This happens when a cfid is in the process of being cleaned up when, and
has been removed from the cfids->entries list, including:
- Receiving a lease break from the server
- Server reconnection triggers invalidate_all_cached_dirs(), which
removes all the cfids from the list
- The laundromat thread decides to expire an old cfid.
To solve these problems, dropping the dentry is done in queued work done
in a newly-added cfid_put_wq workqueue, and close_all_cached_dirs()
flushes that workqueue after it drops all the dentries of which it's
aware. This is a global workqueue (rather than scoped to a mount), but
the queued work is minimal.
Why does it need to be a global workqueue? Can't you make it per tcon?
The problem with a per-tcon workqueue is I didn't see clean way to deal with
multiuser mounts and flushing the workqueue in close_all_cached_dirs() -- when
dealing with each individual tcon, we're still holding tlink_tree_lock, so an
arbitrary sleep seems problematic.
There could be a per-sb workqueue (stored in cifs_sb or the master tcon) but
is there a way to get back to the superblock / master tcon with just a tcon
(e.g. cached_dir_lease_break, when processing a lease break)?
The final cleanup work for cleaning up a cfid is performed via work
queued in the serverclose_wq workqueue; this is done separate from
dropping the dentries so that close_all_cached_dirs() doesn't block on
any server operations.
Both of these queued works expect to invoked with a cfid reference and
a tcon reference to avoid those objects from being freed while the work
is ongoing.
Why do you need to take a tcon reference?
In the existing code (and my patch, without the refs), I was seeing an
intermittent use-after-free of the tcon or cached_fids struct by queued work
processing a lease break -- the cfid isn't linked from cached_fids, but
smb2_close_cached_fid invoking SMB2_close can race with the unmount and
cifs_put_tcon
Something like:
t1 t2
cached_dir_lease_break
smb2_cached_lease_break
smb2_close_cached_fid
SMB2_close starts
cifs_kill_sb
cifs_umount
cifs_put_link
cifs_put_tcon
SMB2_close continues
I had a version of the patch that kept the 'in flight lease breaks' on
a second list in cached_fids so that they could be cancelled synchronously
from free_cached_fids(), but I struggled with it (I can't remember exactly,
but I think I was struggling to get the linked list membership / removal
handling and num_entries handling consistent).
Can't you drop the dentries
when tearing down tcon in cifs_put_tcon()? No concurrent mounts would
be able to access or free it.
The dentries being dropped must occur before kill_anon_super(), as that's
where the 'Dentry still in use' check is. All the tcons are put in
cifs_umount(), which occurs after:
kill_anon_super(sb);
cifs_umount(cifs_sb);
The other thing is that cifs_umount_begin() has this comment, which made me
think a tcon can actually be tied to two distinct mount points:
if ((tcon->tc_count > 1) || (tcon->status == TID_EXITING)) {
/* we have other mounts to same share or we have
already tried to umount this and woken up
all waiting network requests, nothing to do */
Although, as I'm thinking about it again, I think I've misunderstood (and that
comment is wrong?).
It did cross my mind to pull some of the work out of cifs_umount into
cifs_kill_sb (specifically, I wanted to cancel prune_tlinks earlier) -- no
prune_tlinks would make it more feasible to drop tlink_tree_lock in
close_all_cached_dirs(), at which point a per-tcon workqueue is more
practical.
After running xfstests I've seen a leaked tcon in
/proc/fs/cifs/DebugData with no CIFS superblocks, which might be related
to this.
Could you please check if there is any leaked connection in
/proc/fs/cifs/DebugData after running your tests?
After I finish with my tests (I'm not using xfstests, although perhaps
I should be) and unmount the share, DebugData doesn't show any connections for
me.
~Paul
