Re: [PATCH v2 4/4] smb: During unmount, ensure all cached dir instances drop their dentry

[Paulo Alcantara <pc@manguebit.com>]

Paul Aurich <paul@darkrain42.org> writes:

> On 2024-11-21 23:05:51 -0300, Paulo Alcantara wrote:
>>
>>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.

OK.

> 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)?

Yes - cifs_get_dfs_tcon_super() does that.

>>> 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

Makes sense.

> 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).

No worries.  The damn thing isn't trivial to follow.

>> 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);

Right.  Can't we call cancel_work_sync() to make sure that any leases
breakes are processed on the cached directory handle before calling the
above?

> 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?).

Comment is correct as a single tcon may be shared among different
mounts.

Consider the following where a single tcon is shared:

mount.cifs //srv/share /mnt/1 -o $opts
mount.cifs //srv/share/dir /mnt/2 -o $opts

There will be two different superblocks that end up using same tcon.

> 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.

Yeah, multiuser tcons just make it even more complicated.

Sorry for the delay as I've been quite busy with other stuff.

Great work, BTW.