Re: [PATCH 2/7] cifs: protect cfid accesses with fid_lock

[Paul Aurich <paul@darkrain42.org>]

On 2025-06-12 18:35:13 +0530, Shyam Prasad N wrote:
>On Thu, Jun 12, 2025 at 8:55 AM Paul Aurich <paul@darkrain42.org> wrote:
>>
>> On 2025-06-04 15:48:11 +0530, nspmangalore@gmail.com wrote:
>> >From: Shyam Prasad N <sprasad@microsoft.com>
>> >
>> >@@ -220,17 +225,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>> >               goto out;
>> >       }
>> >
>> >-      if (!npath[0]) {
>> >-              dentry = dget(cifs_sb->root);
>> >-      } else {
>> >-              dentry = path_to_dentry(cifs_sb, npath);
>> >-              if (IS_ERR(dentry)) {
>> >-                      rc = -ENOENT;
>> >-                      goto out;
>> >-              }
>> >-      }
>> >-      cfid->dentry = dentry;
>> >-      cfid->tcon = tcon;
>>
>> I think moving this down below to after the "At this point the directory
>> handle is fully cached" comment is going to regress c353ee4fb119 ("smb:
>> Initialize cfid->tcon before performing network ops").
>
>Hi Paul,
>
>Are you referring to the tcon reference?
>If so, I think the way to fix that is to get a reference (tc_count++)
>on tcon right when we add it to the cfid->tcon, and put the reference
>in smb2_close_cached_fid.
>I think that should take care of any concerns of race. Let me know if
>you disagree.

I don't think that is sufficient.  cfid->tcon needs to be initialized prior to 
the network operations in open_cached_dir.  If we receive a lease break from 
the server and the delayed work involved in that runs *before* cfid->tcon is 
initialized, it leaks a ref.  (cached_dir_lease_break has a direct copy of the 
tcon passed as an arg and takes a ref from that, but cfid->tcon is still NULL)

T1                 T2

open_cached_dir
   <open+lease acquisition>
   (gets far enough along that the file is open and lease key initialized)

                    // receives a lease break from the server
                    cached_dir_lease_break
                      // takes a ref on tcon (via arg to function, not cfid->tcon)
                    cached_dir_put_work
                    cached_dir_offload_close
                      // cifs_put_tcon on cfid->tcon, which is still NULL

open_cached_dir continues on at this point

Another reason you need to initialize cfid->tcon earlier is that in the error 
paths, cfid->tcon needs to be valid so that smb2_close_cached_fid can actually 
close it.  Before c353ee4fb119, the tcon was initialized at the same time 
cfid->is_open was set.

>>
>> >       /*
>> >        * We do not hold the lock for the open because in case
>> >@@ -302,9 +296,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon,
>> >               }
>> >               goto oshr_free;
>> >       }
>> >-      cfid->is_open = true;
>>
>> Moving this down below is going to regress part of 66d45ca1350a ("cifs: Check
>> the lease context if we actually got a lease"), I think. If parsing the lease
>> fails, the cfid is disposed, but since `is_open` is false, the code won't call
>> SMB2_close.
>
>That's a good point. I'll submit a patch to fix this.
>

>-- 
>Regards,
>Shyam

-- 
~Paul