[PATCH 2/2] smb: invalidate and close cached directory when creating child entries

[PATCH 2/2] smb: invalidate and close cached directory when creating child entries

[Bharath SM]

When a parent lease key is passed to the server during a create operation
while holding a directory lease, the server may not send a lease break to
the client. In such cases, it becomes the client’s responsibility to
ensure cache consistency.

This led to a problem where directory listings (e.g., `ls` or `readdir`)
could return stale results after a new file is created.
eg:
ls /mnt/share/
touch /mnt/share/file1
ls /mnt/share/

In this scenario, the final `ls` may not show  `file1` due to the stale
directory cache.

For now, fix this by marking the cached directory as invalid if using
the parent lease key during create, and explicitly closing the cached
directory after successful file creation.

Fixes: 037e1bae588eacf ("smb: client: use ParentLeaseKey in cifs_do_create")

Signed-off-by: Bharath SM <bharathsm@microsoft.com>
---
 fs/smb/client/dir.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 1c6e5389c51f..f2c87515dadb 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -190,6 +190,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
 	int disposition;
 	struct TCP_Server_Info *server = tcon->ses->server;
 	struct cifs_open_parms oparms;
+	struct cached_fid *parent_cfid = NULL;
 	int rdwr_for_fscache = 0;
 	__le32 lease_flags = 0;
 
@@ -313,10 +314,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
 	if (!tcon->unix_ext && (mode & S_IWUGO) == 0)
 		create_options |= CREATE_OPTION_READONLY;
 
+
 retry_open:
 	if (tcon->cfids && direntry->d_parent && server->dialect >= SMB30_PROT_ID) {
-		struct cached_fid *parent_cfid;
-
+		parent_cfid = NULL;
 		spin_lock(&tcon->cfids->cfid_list_lock);
 		list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
 			if (parent_cfid->dentry == direntry->d_parent) {
@@ -327,6 +328,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
 					memcpy(fid->parent_lease_key,
 					       parent_cfid->fid.lease_key,
 					       SMB2_LEASE_KEY_SIZE);
+					parent_cfid->dirents.is_valid = false;
 				}
 				break;
 			}
@@ -355,6 +357,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
 		}
 		goto out;
 	}
+
+	if (parent_cfid && !parent_cfid->dirents.is_valid)
+		close_cached_dir(parent_cfid);
+
 	if (rdwr_for_fscache == 2)
 		cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
 
-- 
2.43.0

Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries

[Dan Carpenter]

Hi Bharath,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bharath-SM/smb-invalidate-and-close-cached-directory-when-creating-child-entries/20250701-025420
base:   git://git.samba.org/sfrench/cifs-2.6.git for-next
patch link:    https://lore.kernel.org/r/20250630185303.12087-1-bharathsm%40microsoft.com
patch subject: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries
config: i386-randconfig-141-20250702 (https://download.01.org/0day-ci/archive/20250702/202507021119.3IUZ9mSr-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202507021119.3IUZ9mSr-lkp@intel.com/

smatch warnings:
fs/smb/client/dir.c:361 cifs_do_create() warn: iterator used outside loop: 'parent_cfid'

vim +/parent_cfid +361 fs/smb/client/dir.c

67750fb9e07940 fs/cifs/dir.c       Jeff Layton       2008-05-09  310  	/*
67750fb9e07940 fs/cifs/dir.c       Jeff Layton       2008-05-09  311  	 * if we're not using unix extensions, see if we need to set
67750fb9e07940 fs/cifs/dir.c       Jeff Layton       2008-05-09  312  	 * ATTR_READONLY on the create call
67750fb9e07940 fs/cifs/dir.c       Jeff Layton       2008-05-09  313  	 */
f818dd55c4a8b3 fs/cifs/dir.c       Steve French      2009-01-19  314  	if (!tcon->unix_ext && (mode & S_IWUGO) == 0)
67750fb9e07940 fs/cifs/dir.c       Jeff Layton       2008-05-09  315  		create_options |= CREATE_OPTION_READONLY;
67750fb9e07940 fs/cifs/dir.c       Jeff Layton       2008-05-09  316  
129f2ba6d160a9 fs/smb/client/dir.c Bharath SM        2025-07-01  317  
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  318  retry_open:
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  319  	if (tcon->cfids && direntry->d_parent && server->dialect >= SMB30_PROT_ID) {
129f2ba6d160a9 fs/smb/client/dir.c Bharath SM        2025-07-01  320  		parent_cfid = NULL;

This assignment should be removed because the parent_cfid pointer will
be set again inside the list_for_each_entry() loop.

037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  321  		spin_lock(&tcon->cfids->cfid_list_lock);
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  322  		list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  323  			if (parent_cfid->dentry == direntry->d_parent) {
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  324  				cifs_dbg(FYI, "found a parent cached file handle\n");
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  325  				if (parent_cfid->has_lease && parent_cfid->time) {
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  326  					lease_flags
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  327  						|= SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET_LE;
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  328  					memcpy(fid->parent_lease_key,
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  329  					       parent_cfid->fid.lease_key,
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  330  					       SMB2_LEASE_KEY_SIZE);
129f2ba6d160a9 fs/smb/client/dir.c Bharath SM        2025-07-01  331  					parent_cfid->dirents.is_valid = false;
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  332  				}
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  333  				break;
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  334  			}
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  335  		}

After the loop if we don't hit a break then parent_cfid will point to
invalid memory.  (It won't be NULL).

037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  336  		spin_unlock(&tcon->cfids->cfid_list_lock);
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  337  	}
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  338  
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  339  	oparms = (struct cifs_open_parms) {
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  340  		.tcon = tcon,
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  341  		.cifs_sb = cifs_sb,
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  342  		.desired_access = desired_access,
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  343  		.create_options = cifs_create_options(cifs_sb, create_options),
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  344  		.disposition = disposition,
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  345  		.path = full_path,
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  346  		.fid = fid,
037e1bae588eac fs/smb/client/dir.c Henrique Carvalho 2025-05-28  347  		.lease_flags = lease_flags,
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  348  		.mode = mode,
de036dcaca65cf fs/cifs/dir.c       Volker Lendecke   2023-01-11  349  	};
226730b4d8adae fs/cifs/dir.c       Pavel Shilovsky   2013-07-05  350  	rc = server->ops->open(xid, &oparms, oplock, buf);
^1da177e4c3f41 fs/cifs/dir.c       Linus Torvalds    2005-04-16  351  	if (rc) {
f96637be081141 fs/cifs/dir.c       Joe Perches       2013-05-04  352  		cifs_dbg(FYI, "cifs_create returned 0x%x\n", rc);
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  353  		if (rc == -EACCES && rdwr_for_fscache == 1) {
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  354  			desired_access &= ~GENERIC_READ;
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  355  			rdwr_for_fscache = 2;
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  356  			goto retry_open;
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  357  		}
d2c127197dfc0b fs/cifs/dir.c       Miklos Szeredi    2012-06-05  358  		goto out;
c3b2a0c640bff7 fs/cifs/dir.c       Steve French      2009-02-20  359  	}
129f2ba6d160a9 fs/smb/client/dir.c Bharath SM        2025-07-01  360  
129f2ba6d160a9 fs/smb/client/dir.c Bharath SM        2025-07-01 @361  	if (parent_cfid && !parent_cfid->dirents.is_valid)
                                                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
So this check is reading from invalid memory.

129f2ba6d160a9 fs/smb/client/dir.c Bharath SM        2025-07-01  362  		close_cached_dir(parent_cfid);
129f2ba6d160a9 fs/smb/client/dir.c Bharath SM        2025-07-01  363  
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  364  	if (rdwr_for_fscache == 2)
e9e62243a3e232 fs/smb/client/dir.c David Howells     2024-04-02  365  		cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
c3b2a0c640bff7 fs/cifs/dir.c       Steve French      2009-02-20  366  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries

[Henrique Carvalho]

Hi Bharath,

On Tue, Jul 01, 2025 at 12:23:03AM +0530, Bharath SM wrote:
> @@ -190,6 +190,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>  	int disposition;
>  	struct TCP_Server_Info *server = tcon->ses->server;
>  	struct cifs_open_parms oparms;
> +	struct cached_fid *parent_cfid = NULL;
>  	int rdwr_for_fscache = 0;
>  	__le32 lease_flags = 0;
>
> @@ -313,10 +314,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>  	if (!tcon->unix_ext && (mode & S_IWUGO) == 0)
>  		create_options |= CREATE_OPTION_READONLY;
>  
> +
>  retry_open:
>  	if (tcon->cfids && direntry->d_parent && server->dialect >= SMB30_PROT_ID) {
> -		struct cached_fid *parent_cfid;
> -
> +		parent_cfid = NULL;

I believe setting to NULL here is unnecessary, no?

>  		spin_lock(&tcon->cfids->cfid_list_lock);
>  		list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
>  			if (parent_cfid->dentry == direntry->d_parent) {
> @@ -327,6 +328,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>  					memcpy(fid->parent_lease_key,
>  					       parent_cfid->fid.lease_key,
>  					       SMB2_LEASE_KEY_SIZE);
> +					parent_cfid->dirents.is_valid = false;

Shouldn't we set dirents.is_valid to false only after the open is
successful?

>  				}
>  				break;
>  			}
> @@ -355,6 +357,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>  		}
>  		goto out;
>  	}
> +
> +	if (parent_cfid && !parent_cfid->dirents.is_valid)
> +		close_cached_dir(parent_cfid);
> +
>  	if (rdwr_for_fscache == 2)
>  		cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
>

Apart from the above,

Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>

Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries

[Enzo Matsumiya]

On 07/02, Henrique Carvalho wrote:
>Hi Bharath,
>
>On Tue, Jul 01, 2025 at 12:23:03AM +0530, Bharath SM wrote:
>> @@ -190,6 +190,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>>  	int disposition;
>>  	struct TCP_Server_Info *server = tcon->ses->server;
>>  	struct cifs_open_parms oparms;
>> +	struct cached_fid *parent_cfid = NULL;
>>  	int rdwr_for_fscache = 0;
>>  	__le32 lease_flags = 0;
>>
>> @@ -313,10 +314,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>>  	if (!tcon->unix_ext && (mode & S_IWUGO) == 0)
>>  		create_options |= CREATE_OPTION_READONLY;
>>
>> +
>>  retry_open:
>>  	if (tcon->cfids && direntry->d_parent && server->dialect >= SMB30_PROT_ID) {
>> -		struct cached_fid *parent_cfid;
>> -
>> +		parent_cfid = NULL;
>
>I believe setting to NULL here is unnecessary, no?

It's for the cases it loops back to retry_open.

>>  		spin_lock(&tcon->cfids->cfid_list_lock);
>>  		list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
>>  			if (parent_cfid->dentry == direntry->d_parent) {
>> @@ -327,6 +328,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>>  					memcpy(fid->parent_lease_key,
>>  					       parent_cfid->fid.lease_key,
>>  					       SMB2_LEASE_KEY_SIZE);
>> +					parent_cfid->dirents.is_valid = false;
>
>Shouldn't we set dirents.is_valid to false only after the open is
>successful?

Agreed.  Even though the most common failure cases will trigger a
reconnect anyway (i.e. cache invalidation), it makes sense to keep the
cache for the other cases.

Also, open_cached_dir_by_dentry() gets a cfid ref, why not use it and
have ->has_lease and ->time checked on success?  It would also look
cleaner.

Also 2: ->dirents should be accessed locked with its mutex, otherwise
there's a risk of race with cifs_readdir() and potentially UAF on the
close_cached_dir() below.

>>  				}
>>  				break;
>>  			}
>> @@ -355,6 +357,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
>>  		}
>>  		goto out;
>>  	}
>> +
>> +	if (parent_cfid && !parent_cfid->dirents.is_valid)
>> +		close_cached_dir(parent_cfid);
>> +
>>  	if (rdwr_for_fscache == 2)
>>  		cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
>>
>
>Apart from the above,
>
>Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>


Cheers,

Enzo

Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries

[Steve French]

I tried a simple experiment with this patch, and it did fix the
problem where the new file did not show up in the cached directory
until lease expires, but I am very concerned because it added 50
percent more roundtrips for a simple example (see below - went from 12
to 18).

root@smfrench-ThinkPad-P52:/home/smfrench/cifs-2.6/fs/smb# ls
/mnt/scratch ; ls /mnt/scratch/dir1 ;  touch /mnt/scratch/dir1/filenew
; ls /mnt/scratch/dir1 ; sleep 2 ; stat /mnt/scratch ; stat
/mnt/scratch/dir1

The problem is that instead of just invalidating the directory
contents, the patch is closing the dir1 in this example but this also
strangely causes the "stat of /mnt/scratch" ie the parent directory to
not use the lease either.  If the directory is left open, and the
lease held, but just the use of the dir contents invalidated, then we
could simply do the querydir, not the extra open, and it would also
likely fix the strange extra query info on the parent directory that
the patch causes in the example I used.

The 50% extra roundtrips concern me

On Wed, Jul 2, 2025 at 3:31 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 07/02, Henrique Carvalho wrote:
> >Hi Bharath,
> >
> >On Tue, Jul 01, 2025 at 12:23:03AM +0530, Bharath SM wrote:
> >> @@ -190,6 +190,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
> >>      int disposition;
> >>      struct TCP_Server_Info *server = tcon->ses->server;
> >>      struct cifs_open_parms oparms;
> >> +    struct cached_fid *parent_cfid = NULL;
> >>      int rdwr_for_fscache = 0;
> >>      __le32 lease_flags = 0;
> >>
> >> @@ -313,10 +314,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
> >>      if (!tcon->unix_ext && (mode & S_IWUGO) == 0)
> >>              create_options |= CREATE_OPTION_READONLY;
> >>
> >> +
> >>  retry_open:
> >>      if (tcon->cfids && direntry->d_parent && server->dialect >= SMB30_PROT_ID) {
> >> -            struct cached_fid *parent_cfid;
> >> -
> >> +            parent_cfid = NULL;
> >
> >I believe setting to NULL here is unnecessary, no?
>
> It's for the cases it loops back to retry_open.
>
> >>              spin_lock(&tcon->cfids->cfid_list_lock);
> >>              list_for_each_entry(parent_cfid, &tcon->cfids->entries, entry) {
> >>                      if (parent_cfid->dentry == direntry->d_parent) {
> >> @@ -327,6 +328,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
> >>                                      memcpy(fid->parent_lease_key,
> >>                                             parent_cfid->fid.lease_key,
> >>                                             SMB2_LEASE_KEY_SIZE);
> >> +                                    parent_cfid->dirents.is_valid = false;
> >
> >Shouldn't we set dirents.is_valid to false only after the open is
> >successful?
>
> Agreed.  Even though the most common failure cases will trigger a
> reconnect anyway (i.e. cache invalidation), it makes sense to keep the
> cache for the other cases.
>
> Also, open_cached_dir_by_dentry() gets a cfid ref, why not use it and
> have ->has_lease and ->time checked on success?  It would also look
> cleaner.
>
> Also 2: ->dirents should be accessed locked with its mutex, otherwise
> there's a risk of race with cifs_readdir() and potentially UAF on the
> close_cached_dir() below.
>
> >>                              }
> >>                              break;
> >>                      }
> >> @@ -355,6 +357,10 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
> >>              }
> >>              goto out;
> >>      }
> >> +
> >> +    if (parent_cfid && !parent_cfid->dirents.is_valid)
> >> +            close_cached_dir(parent_cfid);
> >> +
> >>      if (rdwr_for_fscache == 2)
> >>              cifs_invalidate_cache(inode, FSCACHE_INVAL_DIO_WRITE);
> >>
> >
> >Apart from the above,
> >
> >Reviewed-by: Henrique Carvalho <henrique.carvalho@suse.com>
>
>
> Cheers,
>
> Enzo



-- 
Thanks,

Steve