smb: client: rework cached dirs synchronization

This patch adds usage of RCU and seqlocks for cached dir (list and
entries).

Traversing the list under RCU allows faster lookups (no locks) and also
guarantees that entries being read are not gone (i.e. prevents UAF).

seqlocks provides atomicity/consistency when reading entries, allowing
callers to re-check cfid in case of write-side invalidation.

Combined with refcounting, this new approach provides safety for
callers and flexibility for future enhancements.

Other:
- now that we can inform lookups of cfid changes, set cfid->dentry
  earlier in open_cached_dir() to allow by-dentry lookups to find
  this cfid

Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de>

3 files changed, 166 insertions, 92 deletions

diff --git a/fs/smb/client/cached_dir.c b/fs/smb/client/cached_dir.c
index 361f9529ea8a..c75ae54654ac 100644
--- a/fs/smb/client/cached_dir.c
+++ b/fs/smb/client/cached_dir.c
@@ -16,12 +16,12 @@
 #define CFID_INVALID_TIME 1
 
 static struct cached_fid *init_cached_dir(const char *path);
-static void smb2_close_cached_fid(struct kref *ref);
+static void cfid_release_ref(struct kref *ref);
 
 static inline void invalidate_cfid(struct cached_fid *cfid)
 {
 	/* callers must hold the list lock and do any list operations (del/move) themselves */
-	lockdep_assert_held(&cfid->cfids->cfid_list_lock);
+	lockdep_assert_held(&cfid->cfids->entries_seqlock.lock);
 
 	if (cfid_is_valid(cfid))
 		cfid->cfids->num_entries--;
@@ -31,20 +31,19 @@ static inline void invalidate_cfid(struct cached_fid *cfid)
 	cfid->atime = CFID_INVALID_TIME;
 }
 
-static inline void drop_cfid(struct cached_fid *cfid)
+static inline void __drop_cfid(struct cached_fid *cfid)
 {
 	struct dentry *dentry = NULL;
 	u64 pfid = 0, vfid = 0;
 
-	spin_lock(&cfid->cfids->cfid_list_lock);
+	write_seqlock(&cfid->cfids->entries_seqlock);
+	write_seqlock(&cfid->seqlock);
 	invalidate_cfid(cfid);
-
-	spin_lock(&cfid->fid_lock);
 	swap(cfid->dentry, dentry);
 	swap(cfid->fid.persistent_fid, pfid);
 	swap(cfid->fid.volatile_fid, vfid);
-	spin_unlock(&cfid->fid_lock);
-	spin_unlock(&cfid->cfids->cfid_list_lock);
+	write_sequnlock(&cfid->seqlock);
+	write_sequnlock(&cfid->cfids->entries_seqlock);
 
 	dput(dentry);
 
@@ -58,11 +57,18 @@ static inline void drop_cfid(struct cached_fid *cfid)
 	}
 }
 
+static inline void drop_cfid(struct cached_fid *cfid)
+{
+	__drop_cfid(cfid);
+	kref_put(&cfid->refcount, cfid_release_ref);
+}
+
 /*
  * Find a cached dir based on @key and @mode (raw lookup).
  * The only validation done here is if cfid is going down (->ctime == CFID_INVALID_TIME).
  *
  * If @wait_open is true, keep retrying until cfid transitions from 'opening' to valid/invalid.
+ * Will also keep retrying on list seqcount invalidations.
  *
  * Callers must handle any other validation as needed.
  * Returned cfid, if found, has a ref taken, regardless of state.
@@ -72,7 +78,7 @@ static struct cached_fid *find_cfid(struct cached_fids *cfids, const void *key,
 {
 	struct cached_fid *cfid, *found;
 	const char *parent_path = NULL;
-	bool match;
+	unsigned int lseq = 0;
 
 	if (!cfids || !key)
 		return NULL;
@@ -97,8 +103,16 @@ static struct cached_fid *find_cfid(struct cached_fids *cfids, const void *key,
 retry_find:
 	found = NULL;
 
-	spin_lock(&cfids->cfid_list_lock);
-	list_for_each_entry(cfid, &cfids->entries, entry) {
+	rcu_read_lock();
+	lseq = read_seqbegin(&cfids->entries_seqlock);
+	list_for_each_entry_rcu(cfid, &cfids->entries, entry) {
+		bool match = false;
+
+		if (need_seqretry(&cfids->entries_seqlock, lseq)) {
+			found = ERR_PTR(-ECHILD);
+			break;
+		}
+
 		/* don't even bother checking if it's going away */
 		if (cfid->ctime == CFID_INVALID_TIME)
 			continue;
@@ -115,23 +129,59 @@ retry_find:
 		if (!match)
 			continue;
 
-		/* only get a ref here if not waiting for open */
-		if (!wait_open)
-			kref_get(&cfid->refcount);
+		if (wait_open && !cfid->ctime) {
+			unsigned int cseq = read_seqbegin(&cfid->seqlock);
+
+			if (!cfid->ctime)
+				found = ERR_PTR(-ECHILD);
+			else if (!cfid_is_valid(cfid))
+				found = ERR_PTR(-EINVAL);
+
+			if (read_seqretry(&cfid->seqlock, cseq) && !found)
+				found = ERR_PTR(-ECHILD);
+
+			if (found)
+				break;
+		}
+
+		kref_get(&cfid->refcount);
 		found = cfid;
 		break;
 	}
-	spin_unlock(&cfids->cfid_list_lock);
 
-	if (wait_open && found) {
-		/* cfid is being opened in open_cached_dir(), retry lookup */
-		if (!found->ctime)
-			goto retry_find;
+	if (read_seqretry(&cfids->entries_seqlock, lseq)) {
+		if (wait_open) {
+			if (!found) {
+				found = ERR_PTR(-ECHILD);
+
+				/*
+				 * Not found but caller requested wait for open.
+				 * The list seqcount invalidation might have been our open, retry
+				 * only once more (in case it wasn't).
+				 */
+				wait_open = false;
+			}
+		}
 
-		/* we didn't get a ref above, so get one now */
-		kref_get(&found->refcount);
+		if (found && !IS_ERR(found)) {
+			/* can't put ref under RCU lock, do it below */
+			cfid = found;
+			found = ERR_PTR(-EUCLEAN);
+		}
+	}
+	rcu_read_unlock();
+
+	if (PTR_ERR(found) == -EUCLEAN) {
+		kref_put(&cfid->refcount, cfid_release_ref);
+		found = ERR_PTR(-ECHILD);
 	}
 
+	if (PTR_ERR(found) == -ECHILD)
+		goto retry_find;
+
+	if (IS_ERR(found))
+		found = NULL;
+
 	kfree(parent_path);
 
 	return found;
@@ -211,7 +261,7 @@ struct cached_fid *find_cached_dir(struct cached_fids *cfids, const void *key, i
 		if (cfid_is_valid(cfid)) {
 			cfid->atime = jiffies;
 		} else {
-			kref_put(&cfid->refcount, smb2_close_cached_fid);
+			kref_put(&cfid->refcount, cfid_release_ref);
 			cfid = NULL;
 		}
 	}
@@ -273,34 +323,35 @@ replay_again:
 	if (!server->ops->new_lease_key)
 		return -EIO;
 
-	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
-	if (!utf16_path)
-		return -ENOMEM;
-
 	/* find_cached_dir() already validates cfid if found, so no need to check here again */
 	cfid = find_cached_dir(cfids, path, CFID_LOOKUP_PATH);
 	if (cfid) {
-		rc = 0;
-		goto out;
+		*ret_cfid = cfid;
+		return 0;
 	}
 
-	spin_lock(&cfids->cfid_list_lock);
+	utf16_path = cifs_convert_path_to_utf16(path, cifs_sb);
+	if (!utf16_path)
+		return -ENOMEM;
+
+	read_seqlock_excl(&cfids->entries_seqlock);
 	if (cfids->num_entries >= tcon->max_cached_dirs) {
-		spin_unlock(&cfids->cfid_list_lock);
+		read_sequnlock_excl(&cfids->entries_seqlock);
 		rc = -ENOENT;
 		goto out;
 	}
+	read_sequnlock_excl(&cfids->entries_seqlock);
 
+	/* no ned to lock cfid or entries yet */
 	cfid = init_cached_dir(path);
 	if (!cfid) {
-		spin_unlock(&cfids->cfid_list_lock);
 		rc = -ENOMEM;
 		goto out;
 	}
 
 	cfid->cfids = cfids;
 	cfid->tcon = tcon;
-	spin_unlock(&cfids->cfid_list_lock);
+	pfid = &cfid->fid;
 
 	/*
 	 * Skip any prefix paths in @path as lookup_noperm_positive_unlocked() ends up
@@ -325,16 +376,16 @@ replay_again:
 			goto out;
 		}
 	}
+
+	write_seqlock(&cfids->entries_seqlock);
+	write_seqlock(&cfid->seqlock);
 	cfid->dentry = dentry;
-	cfid->tcon = tcon;
 	dentry = NULL;
+	write_sequnlock(&cfid->seqlock);
 
-	spin_lock(&cfids->cfid_list_lock);
 	cfids->num_entries++;
-	list_add(&cfid->entry, &cfids->entries);
-	spin_unlock(&cfids->cfid_list_lock);
-
-	pfid = &cfid->fid;
+	list_add_rcu(&cfid->entry, &cfids->entries);
+	write_sequnlock(&cfids->entries_seqlock);
 
 	/*
 	 * We do not hold the lock for the open because in case
@@ -400,8 +451,6 @@ replay_again:
 		goto oshr_free;
 	}
 
-	spin_lock(&cfids->cfid_list_lock);
-
 	o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base;
 	oparms.fid->persistent_fid = o_rsp->PersistentFileId;
 	oparms.fid->volatile_fid = o_rsp->VolatileFileId;
@@ -411,30 +460,23 @@ replay_again:
 
 
 	if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) {
-		spin_unlock(&cfids->cfid_list_lock);
 		rc = -EINVAL;
 		goto oshr_free;
 	}
 
-	rc = smb2_parse_contexts(server, rsp_iov,
-				 &oparms.fid->epoch,
-				 oparms.fid->lease_key,
+	rc = smb2_parse_contexts(server, rsp_iov, &oparms.fid->epoch, oparms.fid->lease_key,
 				 &oplock, NULL, NULL);
-	if (rc) {
-		spin_unlock(&cfids->cfid_list_lock);
+	if (rc)
 		goto oshr_free;
-	}
 
 	rc = -EINVAL;
-	if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) {
-		spin_unlock(&cfids->cfid_list_lock);
+	if (!(oplock & SMB2_LEASE_READ_CACHING_HE))
 		goto oshr_free;
-	}
+
 	qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
-	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) {
-		spin_unlock(&cfids->cfid_list_lock);
+	if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info))
 		goto oshr_free;
-	}
+
 	if (!smb2_validate_and_copy_iov(le16_to_cpu(qi_rsp->OutputBufferOffset),
 					sizeof(struct smb2_file_all_info), &rsp_iov[1],
 					sizeof(struct smb2_file_all_info), (char *)&info)) {
@@ -445,10 +487,6 @@ replay_again:
 		}
 	}
 
-	cfid->ctime = jiffies;
-	cfid->atime = jiffies;
-	spin_unlock(&cfids->cfid_list_lock);
-	/* At this point the directory handle is fully cached */
 	rc = 0;
 oshr_free:
 	SMB2_open_free(&rqst[0]);
@@ -456,16 +494,22 @@ oshr_free:
 	free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base);
 	free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
 out:
-	/* cfid invalidated in the mean time, drop it below */
-	if (!rc && !cfid_is_valid(cfid))
+	/* cfid only becomes fully valid below, so can't use cfid_is_valid() here */
+	if (!rc && cfid->ctime == CFID_INVALID_TIME)
 		rc = -ENOENT;
 
 	if (rc) {
-		if (cfid) {
+		dput(dentry);
+
+		if (cfid)
 			drop_cfid(cfid);
-			kref_put(&cfid->refcount, smb2_close_cached_fid);
-		}
 	} else {
+		/* seqlocked-write will inform concurrent lookups of opening -> open transition */
+		write_seqlock(&cfid->seqlock);
+		cfid->ctime = jiffies;
+		cfid->atime = jiffies;
+		write_sequnlock(&cfid->seqlock);
+
 		*ret_cfid = cfid;
 		atomic_inc(&tcon->num_remote_opens);
 	}
@@ -485,7 +529,6 @@ static void __invalidate_cached_dirents(struct cached_fid *cfid)
 	if (!cfid)
 		return;
 
-	mutex_lock(&cfid->dirents.de_mutex);
 	list_for_each_entry_safe(de, q, &cfid->dirents.entries, entry) {
 		list_del(&de->entry);
 		kfree(de->name);
@@ -496,15 +539,13 @@ static void __invalidate_cached_dirents(struct cached_fid *cfid)
 	cfid->dirents.is_failed = false;
 	cfid->dirents.file = NULL;
 	cfid->dirents.pos = 0;
-	mutex_unlock(&cfid->dirents.de_mutex);
 }
 
-static void smb2_close_cached_fid(struct kref *ref)
+static void cfid_rcu_free(struct rcu_head *rcu)
 {
-	struct cached_fid *cfid = container_of(ref, struct cached_fid, refcount);
+	struct cached_fid *cfid = container_of(rcu, struct cached_fid, rcu);
 
 	__invalidate_cached_dirents(cfid);
-	drop_cfid(cfid);
 
 	kfree(cfid->file_all_info);
 	cfid->file_all_info = NULL;
@@ -513,6 +554,14 @@ static void smb2_close_cached_fid(struct kref *ref)
 	kfree(cfid);
 }
 
+static void cfid_release_ref(struct kref *ref)
+{
+	struct cached_fid *cfid = container_of(ref, struct cached_fid, refcount);
+
+	__drop_cfid(cfid);
+	call_rcu_hurry(&cfid->rcu, cfid_rcu_free);
+}
+
 bool drop_cached_dir(struct cached_fids *cfids, const void *key, int mode)
 {
 	struct cached_fid *cfid;
@@ -533,13 +582,16 @@ bool drop_cached_dir(struct cached_fids *cfids, const void *key, int mode)
 		drop_cfid(cfid);
 	} else {
 		/* we're locked in smb2_is_valid_lease_break(), so can't dput/close here */
-		spin_lock(&cfids->cfid_list_lock);
+		write_seqlock(&cfids->entries_seqlock);
+		write_seqlock(&cfid->seqlock);
 		invalidate_cfid(cfid);
-		spin_unlock(&cfids->cfid_list_lock);
+		write_sequnlock(&cfid->seqlock);
+		write_sequnlock(&cfids->entries_seqlock);
+
+		/* put lookup ref */
+		kref_put(&cfid->refcount, cfid_release_ref);
 	}
 
-	/* put lookup ref */
-	kref_put(&cfid->refcount, smb2_close_cached_fid);
 	mod_delayed_work(cfid_put_wq, &cfids->laundromat_work, 0);
 
 	return true;
@@ -560,33 +612,39 @@ void invalidate_cached_dirents(struct cached_fids *cfids, const void *key, int m
 
 	cfid = find_cfid(cfids, key, mode, false);
 	if (cfid) {
-		__invalidate_cached_dirents(cfid);
-		kref_put(&cfid->refcount, smb2_close_cached_fid);
+		if (cfid_is_valid(cfid)) {
+			mutex_lock(&cfid->dirents.de_mutex);
+			__invalidate_cached_dirents(cfid);
+			mutex_unlock(&cfid->dirents.de_mutex);
+		}
+		kref_put(&cfid->refcount, cfid_release_ref);
 	}
 }
 
 void close_cached_dir(struct cached_fid *cfid)
 {
-	kref_put(&cfid->refcount, smb2_close_cached_fid);
+	kref_put(&cfid->refcount, cfid_release_ref);
 }
 
 static void invalidate_all_cfids(struct cached_fids *cfids, bool closed)
 {
-	struct cached_fid *cfid, *q;
+	struct cached_fid *cfid;
 
 	if (!cfids)
 		return;
 
 	/* mark all the cfids as closed and invalidate them for laundromat cleanup */
-	spin_lock(&cfids->cfid_list_lock);
-	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
+	write_seqlock(&cfids->entries_seqlock);
+	list_for_each_entry(cfid, &cfids->entries, entry) {
+		write_seqlock(&cfid->seqlock);
 		invalidate_cfid(cfid);
 		if (closed) {
 			cfid->fid.persistent_fid = 0;
 			cfid->fid.volatile_fid = 0;
 		}
+		write_sequnlock(&cfid->seqlock);
 	}
-	spin_unlock(&cfids->cfid_list_lock);
+	write_sequnlock(&cfids->entries_seqlock);
 
 	/* run laundromat unconditionally now as there might have been previously queued work */
 	mod_delayed_work(cfid_put_wq, &cfids->laundromat_work, 0);
@@ -647,7 +705,7 @@ static struct cached_fid *init_cached_dir(const char *path)
 	INIT_LIST_HEAD(&cfid->entry);
 	INIT_LIST_HEAD(&cfid->dirents.entries);
 	mutex_init(&cfid->dirents.de_mutex);
-	spin_lock_init(&cfid->fid_lock);
+	seqlock_init(&cfid->seqlock);
 
 	/* this is our ref */
 	kref_init(&cfid->refcount);
@@ -663,20 +721,26 @@ static struct cached_fid *init_cached_dir(const char *path)
 
 static void cfids_laundromat_worker(struct work_struct *work)
 {
-	struct cached_fids *cfids;
 	struct cached_fid *cfid, *q;
+	struct cached_fids *cfids;
 	LIST_HEAD(entry);
 
 	cfids = container_of(work, struct cached_fids, laundromat_work.work);
 
-	spin_lock(&cfids->cfid_list_lock);
+	synchronize_rcu();
+
+	write_seqlock(&cfids->entries_seqlock);
 	list_for_each_entry_safe(cfid, q, &cfids->entries, entry) {
+		write_seqlock(&cfid->seqlock);
 		if (cfid_expired(cfid)) {
 			invalidate_cfid(cfid);
-			list_move(&cfid->entry, &entry);
+			/* can't use list_move() here because of possible RCU readers */
+			list_del_rcu(&cfid->entry);
+			list_add(&cfid->entry, &entry);
 		}
+		write_sequnlock(&cfid->seqlock);
 	}
-	spin_unlock(&cfids->cfid_list_lock);
+	write_sequnlock(&cfids->entries_seqlock);
 
 	list_for_each_entry_safe(cfid, q, &entry, entry) {
 		list_del(&cfid->entry);
@@ -692,7 +756,6 @@ static void cfids_laundromat_worker(struct work_struct *work)
 		 * No risk for a double list_del() here because cfid is only on this list now.
 		 */
 		drop_cfid(cfid);
-		kref_put(&cfid->refcount, smb2_close_cached_fid);
 	}
 
 	queue_delayed_work(cfid_put_wq, &cfids->laundromat_work, dir_cache_timeout * HZ);
@@ -705,7 +768,8 @@ struct cached_fids *init_cached_dirs(void)
 	cfids = kzalloc(sizeof(*cfids), GFP_KERNEL);
 	if (!cfids)
 		return NULL;
-	spin_lock_init(&cfids->cfid_list_lock);
+
+	seqlock_init(&cfids->entries_seqlock);
 	INIT_LIST_HEAD(&cfids->entries);
 
 	INIT_DELAYED_WORK(&cfids->laundromat_work, cfids_laundromat_worker);
diff --git a/fs/smb/client/cached_dir.h b/fs/smb/client/cached_dir.h
index 8bd04a0423ad..0c3762a092b2 100644
--- a/fs/smb/client/cached_dir.h
+++ b/fs/smb/client/cached_dir.h
@@ -8,6 +8,8 @@
 #ifndef _CACHED_DIR_H
 #define _CACHED_DIR_H
 
+#include <linux/seqlock.h>
+
 struct cached_dirent {
 	struct list_head entry;
 	char *name;
@@ -33,13 +35,19 @@ struct cached_dirents {
 
 struct cached_fid {
 	struct list_head entry;
+	struct rcu_head rcu;
+	/*
+	 * ->seqlock must be used:
+	 * - write-locked when updating
+	 * - rcu_read_lock() + seqcounted on reads
+	 */
+	seqlock_t seqlock;
 	struct cached_fids *cfids;
 	const char *path;
 	unsigned long ctime; /* (jiffies) creation time, when cfid was created (cached) */
 	unsigned long atime; /* (jiffies) access time, when it was last used */
 	struct kref refcount;
 	struct cifs_fid fid;
-	spinlock_t fid_lock;
 	struct cifs_tcon *tcon;
 	struct dentry *dentry;
 	struct smb2_file_all_info *file_all_info;
@@ -48,11 +56,12 @@ struct cached_fid {
 
 /* default MAX_CACHED_FIDS is 16 */
 struct cached_fids {
-	/* Must be held when:
-	 * - accessing the cfids->entries list
-	 * - accessing cfids->num_entries
+	/*
+	 * ->entries_seqlock must be used when accessing ->entries or ->num_entries:
+	 * - write-locked when updating
+	 * - rcu_read_lock() + seqcounted on reads
 	 */
-	spinlock_t cfid_list_lock;
+	seqlock_t entries_seqlock;
 	int num_entries;
 	struct list_head entries;
 	struct delayed_work laundromat_work;
diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
index bb27b9c97724..72e63db75403 100644
--- a/fs/smb/client/cifs_debug.c
+++ b/fs/smb/client/cifs_debug.c
@@ -306,7 +306,8 @@ static int cifs_debug_dirs_proc_show(struct seq_file *m, void *v)
 				cfids = tcon->cfids;
 				if (!cfids)
 					continue;
-				spin_lock(&cfids->cfid_list_lock); /* check lock ordering */
+
+				read_seqlock_excl(&cfids->entries_seqlock);
 				seq_printf(m, "Num entries: %d\n", cfids->num_entries);
 				list_for_each_entry(cfid, &cfids->entries, entry) {
 					seq_printf(m, "0x%x 0x%llx 0x%llx     %s",
@@ -320,7 +321,7 @@ static int cifs_debug_dirs_proc_show(struct seq_file *m, void *v)
 						seq_printf(m, ", valid dirents");
 					seq_printf(m, "\n");
 				}
-				spin_unlock(&cfids->cfid_list_lock);
+				read_sequnlock_excl(&cfids->entries_seqlock);
 			}
 		}
 	}