Linux CIFS filesystem development
 help / color / mirror / Atom feed
* [PATCH 00/20] smb: client: cached dir fixes and improvements
@ 2025-09-29 13:27 Enzo Matsumiya
  2025-09-29 13:27 ` [PATCH 01/20] smb: client: remove cfids_invalidation_worker Enzo Matsumiya
                   ` (20 more replies)
  0 siblings, 21 replies; 29+ messages in thread
From: Enzo Matsumiya @ 2025-09-29 13:27 UTC (permalink / raw)
  To: linux-cifs
  Cc: smfrench, pc, ronniesahlberg, sprasad, tom, bharathsm,
	henrique.carvalho

Hi,

This patch series aims to refactor cached dir related code in order to
improve performance, improve code maintenance/readability, and of course
fix several, existing and potential, bugs.

Please note that the below only makes sense to the whole series applied.

Semantic fixes:
- cfid->has_lease vs cfid->is_open: when opening a cached dir, we get a fid
  (is_open) and a lease (has_lease), however, has_lease is used differently
  throughout the code, meaning, most of the time, that the cfid is 'usable'
  (fix in patch 11)
- refcounting also follows has_lease, up to a point, when we need to
  'steal' the reference, then we might have a cfid with 2 refs but
  has_lease == false (fix in patches 1-5)
- cfid lookup: currently done with open_cached_dir() with @lookup_only arg,
  but that is not visibly good-looking and also highly inflexible (because
  it only works for paths (char *).


Technical fixes:
- due to the many "Dentry still in use" bugs, cleaning up a cfid has become
  too complex -- there are 3 workers to do that asynchronously, and the
  release callback itself.  Complexity aside, this still has bugs because
  open_cached_dir() design doesn't account for any concurrent invalidation,
  leading sometimes to double opens/closes, sometimes straight UAF/deadlock
  bugs (examples upon request).
  (fix in patches 1-11)
- locking: the list lock is not used consistently; sometimes protecting only
  the list, sometimes protecting only a cfid, sometimes both.
  cfid->fid_lock only protects ->dentry, nothing else.  This leads to
  inconsistent data being read when a concurrent invalidation occurs, e.g.
  cached_dir_lease_break() (sets ->time = 0) vs cifs_dentry_needs_reval()
  (reads ->time unlocked)
  * also, open_cached_dir() always assume it has >1 refs, but such
    assumption is proven wrong when SMB2_open_init() triggers
    smb2_reconnect(), and kref_put() is ran locked in the rc != 0 case,
    leading to a deadlock because the extra ref has been dropped async
  (both fixed in patch 19 and others)

Improvements:
Having all above fixes and changes allows a cleaner code with a simpler
design:
- code readability is improved (cf. whole series)
- usage of cached dirs in places that weren't making use of it (cf. patches
  12-18)
- patch 19 (locking) not only fixes the synchronization problems, but RCU +
  seqcounting allows faster lookups (read-mostly) while also allowing
  consistent reads and stability for callers (prevents UAF)
- because a directory is always a parent, bake-in support for when opening
  a path, ParentLeaseKey can be set for any target child (cf. patch 12)


Cheers,

Enzo Matsumiya (20):
  smb: client: remove cfids_invalidation_worker
  smb: client: remove cached_dir_offload_close/close_work
  smb: client: remove cached_dir_put_work/put_work
  smb: client: remove cached_fids->dying list
  smb: client: remove cached_fid->on_list
  smb: client: merge {close,invalidate}_all_cached_dirs()
  smb: client: merge free_cached_dir in release callback
  smb: client: split find_or_create_cached_dir()
  smb: client: enhance cached dir lookups
  smb: client: refactor dropping cached dirs
  smb: client: simplify cached_fid state checking
  smb: client: prevent lease breaks of cached parents when opening
    children
  smb: client: actually use cached dirs on readdir
  smb: client: wait for concurrent caching of dirents in cifs_readdir()
  smb: client: remove cached_dirent->fattr
  smb: client: add is_dir argument to query_path_info
  smb: client: use cached dir on queryfs/smb2_compound_op
  smb: client: fix dentry revalidation of cached root
  smb: client: rework cached dirs synchronization
  smb: client: cleanup open_cached_dir()

 fs/smb/client/cached_dir.c | 946 ++++++++++++++++---------------------
 fs/smb/client/cached_dir.h |  74 +--
 fs/smb/client/cifs_debug.c |   7 +-
 fs/smb/client/cifsfs.c     |   2 +-
 fs/smb/client/cifsglob.h   |   5 +-
 fs/smb/client/dir.c        |  27 +-
 fs/smb/client/file.c       |   2 +-
 fs/smb/client/inode.c      |  38 +-
 fs/smb/client/misc.c       |   9 +-
 fs/smb/client/readdir.c    | 146 +++---
 fs/smb/client/smb1ops.c    |   6 +-
 fs/smb/client/smb2inode.c  |  48 +-
 fs/smb/client/smb2misc.c   |   2 +-
 fs/smb/client/smb2ops.c    |  49 +-
 fs/smb/client/smb2pdu.c    |  99 +++-
 fs/smb/client/smb2proto.h  |  10 +-
 16 files changed, 733 insertions(+), 737 deletions(-)

-- 
2.49.0


^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2025-10-03 17:26 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-29 13:27 [PATCH 00/20] smb: client: cached dir fixes and improvements Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 01/20] smb: client: remove cfids_invalidation_worker Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 02/20] smb: client: remove cached_dir_offload_close/close_work Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 03/20] smb: client: remove cached_dir_put_work/put_work Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 04/20] smb: client: remove cached_fids->dying list Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 05/20] smb: client: remove cached_fid->on_list Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 06/20] smb: client: merge {close,invalidate}_all_cached_dirs() Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 07/20] smb: client: merge free_cached_dir in release callback Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 08/20] smb: client: split find_or_create_cached_dir() Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 09/20] smb: client: enhance cached dir lookups Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 10/20] smb: client: refactor dropping cached dirs Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 11/20] smb: client: simplify cached_fid state checking Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 12/20] smb: client: prevent lease breaks of cached parents when opening children Enzo Matsumiya
2025-09-29 14:23   ` Steve French
2025-09-29 17:17   ` Enzo Matsumiya
2025-09-29 13:27 ` [PATCH 13/20] smb: client: actually use cached dirs on readdir Enzo Matsumiya
2025-10-03 17:26   ` Dan Carpenter
2025-09-29 13:27 ` [PATCH 14/20] smb: client: wait for concurrent caching of dirents in cifs_readdir() Enzo Matsumiya
2025-09-29 13:28 ` [PATCH 15/20] smb: client: remove cached_dirent->fattr Enzo Matsumiya
2025-09-29 13:28 ` [PATCH 16/20] smb: client: add is_dir argument to query_path_info Enzo Matsumiya
2025-10-03 17:20   ` Dan Carpenter
2025-09-29 13:28 ` [PATCH 17/20] smb: client: use cached dir on queryfs/smb2_compound_op Enzo Matsumiya
2025-09-29 14:26   ` Steve French
2025-09-29 13:28 ` [PATCH 18/20] smb: client: fix dentry revalidation of cached root Enzo Matsumiya
2025-09-29 13:28 ` [PATCH 19/20] smb: client: rework cached dirs synchronization Enzo Matsumiya
2025-09-30 19:02   ` kernel test robot
2025-09-29 13:28 ` [PATCH 20/20] smb: client: cleanup open_cached_dir() Enzo Matsumiya
2025-09-29 14:05 ` [PATCH 00/20] smb: client: cached dir fixes and improvements Steve French
2025-09-29 14:44   ` Enzo Matsumiya

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox