* [PATCH 2/2] smb: invalidate and close cached directory when creating child entries
@ 2025-06-30 18:53 Bharath SM
2025-07-02 11:32 ` Dan Carpenter
2025-07-02 16:55 ` Henrique Carvalho
0 siblings, 2 replies; 6+ messages in thread
From: Bharath SM @ 2025-06-30 18:53 UTC (permalink / raw)
To: linux-cifs, smfrench, pc, sprasad, paul, henrique.carvalho; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries
@ 2025-07-02 3:25 kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2025-07-02 3:25 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250630185303.12087-1-bharathsm@microsoft.com>
References: <20250630185303.12087-1-bharathsm@microsoft.com>
TO: Bharath SM <bharathsm.hsk@gmail.com>
TO: linux-cifs@vger.kernel.org
TO: smfrench@gmail.com
TO: pc@manguebit.com
TO: sprasad@microsoft.com
TO: paul@darkrain42.org
TO: henrique.carvalho@suse.com
CC: Bharath SM <bharathsm@microsoft.com>
Hi Bharath,
kernel test robot noticed the following build warnings:
[auto build test WARNING on cifs/for-next]
[also build test WARNING on linus/master v6.16-rc4]
[cannot apply to next-20250701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
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
:::::: branch date: 32 hours ago
:::::: commit date: 32 hours ago
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 <error27@gmail.com>
| 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
d2c127197dfc0b fs/cifs/dir.c Miklos Szeredi 2012-06-05 281
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 282 desired_access = 0;
608712fe860949 fs/cifs/dir.c Jeff Layton 2010-10-15 283 if (OPEN_FMODE(oflags) & FMODE_READ)
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 284 desired_access |= GENERIC_READ; /* is this too little? */
608712fe860949 fs/cifs/dir.c Jeff Layton 2010-10-15 285 if (OPEN_FMODE(oflags) & FMODE_WRITE)
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 286 desired_access |= GENERIC_WRITE;
e9e62243a3e232 fs/smb/client/dir.c David Howells 2024-04-02 287 if (rdwr_for_fscache == 1)
e9e62243a3e232 fs/smb/client/dir.c David Howells 2024-04-02 288 desired_access |= GENERIC_READ;
^1da177e4c3f41 fs/cifs/dir.c Linus Torvalds 2005-04-16 289
d2c127197dfc0b fs/cifs/dir.c Miklos Szeredi 2012-06-05 290 disposition = FILE_OVERWRITE_IF;
e08fc0457af28f fs/cifs/dir.c Miklos Szeredi 2005-09-06 291 if ((oflags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))
^1da177e4c3f41 fs/cifs/dir.c Linus Torvalds 2005-04-16 292 disposition = FILE_CREATE;
e08fc0457af28f fs/cifs/dir.c Miklos Szeredi 2005-09-06 293 else if ((oflags & (O_CREAT | O_TRUNC)) == (O_CREAT | O_TRUNC))
^1da177e4c3f41 fs/cifs/dir.c Linus Torvalds 2005-04-16 294 disposition = FILE_OVERWRITE_IF;
e08fc0457af28f fs/cifs/dir.c Miklos Szeredi 2005-09-06 295 else if ((oflags & O_CREAT) == O_CREAT)
^1da177e4c3f41 fs/cifs/dir.c Linus Torvalds 2005-04-16 296 disposition = FILE_OPEN_IF;
ad7a2926b9e53c fs/cifs/dir.c Steve French 2008-02-07 297 else
f96637be081141 fs/cifs/dir.c Joe Perches 2013-05-04 298 cifs_dbg(FYI, "Create flag not set in create function\n");
^1da177e4c3f41 fs/cifs/dir.c Linus Torvalds 2005-04-16 299
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 300 /*
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 301 * BB add processing to set equivalent of mode - e.g. via CreateX with
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 302 * ACLs
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 303 */
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 304
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 305 if (!server->ops->open) {
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 306 rc = -ENOSYS;
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 307 goto out;
253641388a4925 fs/cifs/dir.c Pavel Shilovsky 2012-09-18 308 }
^1da177e4c3f41 fs/cifs/dir.c Linus Torvalds 2005-04-16 309
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;
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 }
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)
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries
2025-06-30 18:53 [PATCH 2/2] smb: invalidate and close cached directory when creating child entries Bharath SM
@ 2025-07-02 11:32 ` Dan Carpenter
2025-07-02 16:55 ` Henrique Carvalho
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2025-07-02 11:32 UTC (permalink / raw)
To: oe-kbuild, Bharath SM, linux-cifs, smfrench, pc, sprasad, paul,
henrique.carvalho
Cc: lkp, oe-kbuild-all, Bharath SM
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries
2025-06-30 18:53 [PATCH 2/2] smb: invalidate and close cached directory when creating child entries Bharath SM
2025-07-02 11:32 ` Dan Carpenter
@ 2025-07-02 16:55 ` Henrique Carvalho
2025-07-02 20:31 ` Enzo Matsumiya
1 sibling, 1 reply; 6+ messages in thread
From: Henrique Carvalho @ 2025-07-02 16:55 UTC (permalink / raw)
To: Bharath SM; +Cc: linux-cifs, smfrench, pc, sprasad, paul, Bharath SM
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries
2025-07-02 16:55 ` Henrique Carvalho
@ 2025-07-02 20:31 ` Enzo Matsumiya
2025-07-03 5:55 ` Steve French
0 siblings, 1 reply; 6+ messages in thread
From: Enzo Matsumiya @ 2025-07-02 20:31 UTC (permalink / raw)
To: Henrique Carvalho
Cc: Bharath SM, linux-cifs, smfrench, pc, sprasad, paul, Bharath SM
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] smb: invalidate and close cached directory when creating child entries
2025-07-02 20:31 ` Enzo Matsumiya
@ 2025-07-03 5:55 ` Steve French
0 siblings, 0 replies; 6+ messages in thread
From: Steve French @ 2025-07-03 5:55 UTC (permalink / raw)
To: Enzo Matsumiya
Cc: Henrique Carvalho, Bharath SM, linux-cifs, pc, sprasad, paul,
Bharath SM
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-03 5:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-30 18:53 [PATCH 2/2] smb: invalidate and close cached directory when creating child entries Bharath SM
2025-07-02 11:32 ` Dan Carpenter
2025-07-02 16:55 ` Henrique Carvalho
2025-07-02 20:31 ` Enzo Matsumiya
2025-07-03 5:55 ` Steve French
-- strict thread matches above, loose matches on Subject: below --
2025-07-02 3:25 kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.