[PATCH 1/6] ksmbd: Implements sess->ksmbd_chann_list as xarray

[PATCH 1/6] ksmbd: Implements sess->ksmbd_chann_list as xarray

[Dawei Li]

For some ops on channel:
1. lookup_chann_list(), possibly on high frequency.
2. ksmbd_chann_del().

Connection is used as indexing key to lookup channel, in that case,
linear search based on list may suffer a bit for performance.

Implements sess->ksmbd_chann_list as xarray.

Signed-off-by: Dawei Li <set_pte_at@outlook.com>
---
 fs/ksmbd/mgmt/user_session.c | 61 ++++++++++++++----------------------
 fs/ksmbd/mgmt/user_session.h |  4 +--
 fs/ksmbd/smb2pdu.c           | 34 +++-----------------
 3 files changed, 30 insertions(+), 69 deletions(-)

diff --git a/fs/ksmbd/mgmt/user_session.c b/fs/ksmbd/mgmt/user_session.c
index 92b1603b5abe..a2b128dedcfc 100644
--- a/fs/ksmbd/mgmt/user_session.c
+++ b/fs/ksmbd/mgmt/user_session.c
@@ -30,15 +30,15 @@ struct ksmbd_session_rpc {
 
 static void free_channel_list(struct ksmbd_session *sess)
 {
-	struct channel *chann, *tmp;
+	struct channel *chann;
+	unsigned long index;
 
-	write_lock(&sess->chann_lock);
-	list_for_each_entry_safe(chann, tmp, &sess->ksmbd_chann_list,
-				 chann_list) {
-		list_del(&chann->chann_list);
+	xa_for_each(&sess->ksmbd_chann_list, index, chann) {
+		xa_erase(&sess->ksmbd_chann_list, index);
 		kfree(chann);
 	}
-	write_unlock(&sess->chann_lock);
+
+	xa_destroy(&sess->ksmbd_chann_list);
 }
 
 static void __session_rpc_close(struct ksmbd_session *sess,
@@ -190,21 +190,15 @@ int ksmbd_session_register(struct ksmbd_conn *conn,
 
 static int ksmbd_chann_del(struct ksmbd_conn *conn, struct ksmbd_session *sess)
 {
-	struct channel *chann, *tmp;
-
-	write_lock(&sess->chann_lock);
-	list_for_each_entry_safe(chann, tmp, &sess->ksmbd_chann_list,
-				 chann_list) {
-		if (chann->conn == conn) {
-			list_del(&chann->chann_list);
-			kfree(chann);
-			write_unlock(&sess->chann_lock);
-			return 0;
-		}
-	}
-	write_unlock(&sess->chann_lock);
+	struct channel *chann;
+
+	chann = xa_erase(&sess->ksmbd_chann_list, (long)conn);
+	if (!chann)
+		return -ENOENT;
 
-	return -ENOENT;
+	kfree(chann);
+
+	return 0;
 }
 
 void ksmbd_sessions_deregister(struct ksmbd_conn *conn)
@@ -234,7 +228,7 @@ void ksmbd_sessions_deregister(struct ksmbd_conn *conn)
 	return;
 
 sess_destroy:
-	if (list_empty(&sess->ksmbd_chann_list)) {
+	if (xa_empty(&sess->ksmbd_chann_list)) {
 		xa_erase(&conn->sessions, sess->id);
 		ksmbd_session_destroy(sess);
 	}
@@ -320,6 +314,9 @@ static struct ksmbd_session *__session_create(int protocol)
 	struct ksmbd_session *sess;
 	int ret;
 
+	if (protocol != CIFDS_SESSION_FLAG_SMB2)
+		return NULL;
+
 	sess = kzalloc(sizeof(struct ksmbd_session), GFP_KERNEL);
 	if (!sess)
 		return NULL;
@@ -329,30 +326,20 @@ static struct ksmbd_session *__session_create(int protocol)
 
 	set_session_flag(sess, protocol);
 	xa_init(&sess->tree_conns);
-	INIT_LIST_HEAD(&sess->ksmbd_chann_list);
+	xa_init(&sess->ksmbd_chann_list);
 	INIT_LIST_HEAD(&sess->rpc_handle_list);
 	sess->sequence_number = 1;
-	rwlock_init(&sess->chann_lock);
-
-	switch (protocol) {
-	case CIFDS_SESSION_FLAG_SMB2:
-		ret = __init_smb2_session(sess);
-		break;
-	default:
-		ret = -EINVAL;
-		break;
-	}
 
+	ret = __init_smb2_session(sess);
 	if (ret)
 		goto error;
 
 	ida_init(&sess->tree_conn_ida);
 
-	if (protocol == CIFDS_SESSION_FLAG_SMB2) {
-		down_write(&sessions_table_lock);
-		hash_add(sessions_table, &sess->hlist, sess->id);
-		up_write(&sessions_table_lock);
-	}
+	down_write(&sessions_table_lock);
+	hash_add(sessions_table, &sess->hlist, sess->id);
+	up_write(&sessions_table_lock);
+
 	return sess;
 
 error:
diff --git a/fs/ksmbd/mgmt/user_session.h b/fs/ksmbd/mgmt/user_session.h
index 8934b8ee275b..44a3c67b2bd9 100644
--- a/fs/ksmbd/mgmt/user_session.h
+++ b/fs/ksmbd/mgmt/user_session.h
@@ -21,7 +21,6 @@ struct ksmbd_file_table;
 struct channel {
 	__u8			smb3signingkey[SMB3_SIGN_KEY_SIZE];
 	struct ksmbd_conn	*conn;
-	struct list_head	chann_list;
 };
 
 struct preauth_session {
@@ -50,8 +49,7 @@ struct ksmbd_session {
 	char				sess_key[CIFS_KEY_SIZE];
 
 	struct hlist_node		hlist;
-	rwlock_t			chann_lock;
-	struct list_head		ksmbd_chann_list;
+	struct xarray			ksmbd_chann_list;
 	struct xarray			tree_conns;
 	struct ida			tree_conn_ida;
 	struct list_head		rpc_handle_list;
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 38fbda52e06f..771e045c8d4a 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -74,14 +74,7 @@ static inline bool check_session_id(struct ksmbd_conn *conn, u64 id)
 
 struct channel *lookup_chann_list(struct ksmbd_session *sess, struct ksmbd_conn *conn)
 {
-	struct channel *chann;
-
-	list_for_each_entry(chann, &sess->ksmbd_chann_list, chann_list) {
-		if (chann->conn == conn)
-			return chann;
-	}
-
-	return NULL;
+	return xa_load(&sess->ksmbd_chann_list, (long)conn);
 }
 
 /**
@@ -595,6 +588,7 @@ static void destroy_previous_session(struct ksmbd_conn *conn,
 	struct ksmbd_session *prev_sess = ksmbd_session_lookup_slowpath(id);
 	struct ksmbd_user *prev_user;
 	struct channel *chann;
+	long index;
 
 	if (!prev_sess)
 		return;
@@ -608,10 +602,8 @@ static void destroy_previous_session(struct ksmbd_conn *conn,
 		return;
 
 	prev_sess->state = SMB2_SESSION_EXPIRED;
-	write_lock(&prev_sess->chann_lock);
-	list_for_each_entry(chann, &prev_sess->ksmbd_chann_list, chann_list)
+	xa_for_each(&prev_sess->ksmbd_chann_list, index, chann)
 		chann->conn->status = KSMBD_SESS_EXITING;
-	write_unlock(&prev_sess->chann_lock);
 }
 
 /**
@@ -1519,19 +1511,14 @@ static int ntlm_authenticate(struct ksmbd_work *work)
 
 binding_session:
 	if (conn->dialect >= SMB30_PROT_ID) {
-		read_lock(&sess->chann_lock);
 		chann = lookup_chann_list(sess, conn);
-		read_unlock(&sess->chann_lock);
 		if (!chann) {
 			chann = kmalloc(sizeof(struct channel), GFP_KERNEL);
 			if (!chann)
 				return -ENOMEM;
 
 			chann->conn = conn;
-			INIT_LIST_HEAD(&chann->chann_list);
-			write_lock(&sess->chann_lock);
-			list_add(&chann->chann_list, &sess->ksmbd_chann_list);
-			write_unlock(&sess->chann_lock);
+			xa_store(&sess->ksmbd_chann_list, (long)conn, chann, GFP_KERNEL);
 		}
 	}
 
@@ -1606,19 +1593,14 @@ static int krb5_authenticate(struct ksmbd_work *work)
 	}
 
 	if (conn->dialect >= SMB30_PROT_ID) {
-		read_lock(&sess->chann_lock);
 		chann = lookup_chann_list(sess, conn);
-		read_unlock(&sess->chann_lock);
 		if (!chann) {
 			chann = kmalloc(sizeof(struct channel), GFP_KERNEL);
 			if (!chann)
 				return -ENOMEM;
 
 			chann->conn = conn;
-			INIT_LIST_HEAD(&chann->chann_list);
-			write_lock(&sess->chann_lock);
-			list_add(&chann->chann_list, &sess->ksmbd_chann_list);
-			write_unlock(&sess->chann_lock);
+			xa_store(&sess->ksmbd_chann_list, (long)conn, chann, GFP_KERNEL);
 		}
 	}
 
@@ -8409,14 +8391,11 @@ int smb3_check_sign_req(struct ksmbd_work *work)
 	if (le16_to_cpu(hdr->Command) == SMB2_SESSION_SETUP_HE) {
 		signing_key = work->sess->smb3signingkey;
 	} else {
-		read_lock(&work->sess->chann_lock);
 		chann = lookup_chann_list(work->sess, conn);
 		if (!chann) {
-			read_unlock(&work->sess->chann_lock);
 			return 0;
 		}
 		signing_key = chann->smb3signingkey;
-		read_unlock(&work->sess->chann_lock);
 	}
 
 	if (!signing_key) {
@@ -8476,14 +8455,11 @@ void smb3_set_sign_rsp(struct ksmbd_work *work)
 	    le16_to_cpu(hdr->Command) == SMB2_SESSION_SETUP_HE) {
 		signing_key = work->sess->smb3signingkey;
 	} else {
-		read_lock(&work->sess->chann_lock);
 		chann = lookup_chann_list(work->sess, work->conn);
 		if (!chann) {
-			read_unlock(&work->sess->chann_lock);
 			return;
 		}
 		signing_key = chann->smb3signingkey;
-		read_unlock(&work->sess->chann_lock);
 	}
 
 	if (!signing_key)
-- 
2.25.1

[PATCH 2/6] ksmbd: Implements sess->rpc_handle_list as xarray

[Dawei Li]

For some ops on rpc handle:
1. ksmbd_session_rpc_method(), possibly on high frequency.
2. ksmbd_session_rpc_close().

id is used as indexing key to lookup channel, in that case,
linear search based on list may suffer a bit for performance.

Implements sess->rpc_handle_list as xarray.

Signed-off-by: Dawei Li <set_pte_at@outlook.com>
---
 fs/ksmbd/mgmt/user_session.c | 37 ++++++++++++++----------------------
 fs/ksmbd/mgmt/user_session.h |  2 +-
 2 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/fs/ksmbd/mgmt/user_session.c b/fs/ksmbd/mgmt/user_session.c
index a2b128dedcfc..1ca2aae4c299 100644
--- a/fs/ksmbd/mgmt/user_session.c
+++ b/fs/ksmbd/mgmt/user_session.c
@@ -25,7 +25,6 @@ static DECLARE_RWSEM(sessions_table_lock);
 struct ksmbd_session_rpc {
 	int			id;
 	unsigned int		method;
-	struct list_head	list;
 };
 
 static void free_channel_list(struct ksmbd_session *sess)
@@ -58,15 +57,14 @@ static void __session_rpc_close(struct ksmbd_session *sess,
 static void ksmbd_session_rpc_clear_list(struct ksmbd_session *sess)
 {
 	struct ksmbd_session_rpc *entry;
+	long index;
 
-	while (!list_empty(&sess->rpc_handle_list)) {
-		entry = list_entry(sess->rpc_handle_list.next,
-				   struct ksmbd_session_rpc,
-				   list);
-
-		list_del(&entry->list);
+	xa_for_each(&sess->rpc_handle_list, index, entry) {
+		xa_erase(&sess->rpc_handle_list, index);
 		__session_rpc_close(sess, entry);
 	}
+
+	xa_destroy(&sess->rpc_handle_list);
 }
 
 static int __rpc_method(char *rpc_name)
@@ -102,13 +100,13 @@ int ksmbd_session_rpc_open(struct ksmbd_session *sess, char *rpc_name)
 
 	entry = kzalloc(sizeof(struct ksmbd_session_rpc), GFP_KERNEL);
 	if (!entry)
-		return -EINVAL;
+		return -ENOMEM;
 
-	list_add(&entry->list, &sess->rpc_handle_list);
 	entry->method = method;
 	entry->id = ksmbd_ipc_id_alloc();
 	if (entry->id < 0)
 		goto free_entry;
+	xa_store(&sess->rpc_handle_list, entry->id, entry, GFP_KERNEL);
 
 	resp = ksmbd_rpc_open(sess, entry->id);
 	if (!resp)
@@ -117,9 +115,9 @@ int ksmbd_session_rpc_open(struct ksmbd_session *sess, char *rpc_name)
 	kvfree(resp);
 	return entry->id;
 free_id:
+	xa_erase(&sess->rpc_handle_list, entry->id);
 	ksmbd_rpc_id_free(entry->id);
 free_entry:
-	list_del(&entry->list);
 	kfree(entry);
 	return -EINVAL;
 }
@@ -128,24 +126,17 @@ void ksmbd_session_rpc_close(struct ksmbd_session *sess, int id)
 {
 	struct ksmbd_session_rpc *entry;
 
-	list_for_each_entry(entry, &sess->rpc_handle_list, list) {
-		if (entry->id == id) {
-			list_del(&entry->list);
-			__session_rpc_close(sess, entry);
-			break;
-		}
-	}
+	entry = xa_erase(&sess->rpc_handle_list, id);
+	if (entry)
+		__session_rpc_close(sess, entry);
 }
 
 int ksmbd_session_rpc_method(struct ksmbd_session *sess, int id)
 {
 	struct ksmbd_session_rpc *entry;
 
-	list_for_each_entry(entry, &sess->rpc_handle_list, list) {
-		if (entry->id == id)
-			return entry->method;
-	}
-	return 0;
+	entry = xa_load(&sess->rpc_handle_list, id);
+	return entry ? entry->method : 0;
 }
 
 void ksmbd_session_destroy(struct ksmbd_session *sess)
@@ -327,7 +318,7 @@ static struct ksmbd_session *__session_create(int protocol)
 	set_session_flag(sess, protocol);
 	xa_init(&sess->tree_conns);
 	xa_init(&sess->ksmbd_chann_list);
-	INIT_LIST_HEAD(&sess->rpc_handle_list);
+	xa_init(&sess->rpc_handle_list);
 	sess->sequence_number = 1;
 
 	ret = __init_smb2_session(sess);
diff --git a/fs/ksmbd/mgmt/user_session.h b/fs/ksmbd/mgmt/user_session.h
index 44a3c67b2bd9..b6a9e7a6aae4 100644
--- a/fs/ksmbd/mgmt/user_session.h
+++ b/fs/ksmbd/mgmt/user_session.h
@@ -52,7 +52,7 @@ struct ksmbd_session {
 	struct xarray			ksmbd_chann_list;
 	struct xarray			tree_conns;
 	struct ida			tree_conn_ida;
-	struct list_head		rpc_handle_list;
+	struct xarray			rpc_handle_list;
 
 	__u8				smb3encryptionkey[SMB3_ENC_DEC_KEY_SIZE];
 	__u8				smb3decryptionkey[SMB3_ENC_DEC_KEY_SIZE];
-- 
2.25.1

[PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct access on conn list

[Dawei Li]

Currently, racing protection on conn list is implemented as rwlock,
improve it by rcu primitive for its better performance.

Signed-off-by: Dawei Li <set_pte_at@outlook.com>
---
 fs/ksmbd/connection.c | 30 +++++++++++++++---------------
 fs/ksmbd/connection.h |  1 -
 fs/ksmbd/smb2pdu.c    | 14 +++++++-------
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
index fd0a288af299..36d1da273edd 100644
--- a/fs/ksmbd/connection.c
+++ b/fs/ksmbd/connection.c
@@ -20,7 +20,7 @@ static DEFINE_MUTEX(init_lock);
 static struct ksmbd_conn_ops default_conn_ops;
 
 LIST_HEAD(conn_list);
-DEFINE_RWLOCK(conn_list_lock);
+DEFINE_SPINLOCK(conn_list_lock);
 
 /**
  * ksmbd_conn_free() - free resources of the connection instance
@@ -32,9 +32,9 @@ DEFINE_RWLOCK(conn_list_lock);
  */
 void ksmbd_conn_free(struct ksmbd_conn *conn)
 {
-	write_lock(&conn_list_lock);
-	list_del(&conn->conns_list);
-	write_unlock(&conn_list_lock);
+	spin_lock(&conn_list_lock);
+	list_del_rcu(&conn->conns_list);
+	spin_unlock(&conn_list_lock);
 
 	xa_destroy(&conn->sessions);
 	kvfree(conn->request_buf);
@@ -84,9 +84,9 @@ struct ksmbd_conn *ksmbd_conn_alloc(void)
 	spin_lock_init(&conn->llist_lock);
 	INIT_LIST_HEAD(&conn->lock_list);
 
-	write_lock(&conn_list_lock);
-	list_add(&conn->conns_list, &conn_list);
-	write_unlock(&conn_list_lock);
+	spin_lock(&conn_list_lock);
+	list_add_rcu(&conn->conns_list, &conn_list);
+	spin_unlock(&conn_list_lock);
 	return conn;
 }
 
@@ -95,15 +95,15 @@ bool ksmbd_conn_lookup_dialect(struct ksmbd_conn *c)
 	struct ksmbd_conn *t;
 	bool ret = false;
 
-	read_lock(&conn_list_lock);
-	list_for_each_entry(t, &conn_list, conns_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(t, &conn_list, conns_list) {
 		if (memcmp(t->ClientGUID, c->ClientGUID, SMB2_CLIENT_GUID_SIZE))
 			continue;
 
 		ret = true;
 		break;
 	}
-	read_unlock(&conn_list_lock);
+	rcu_read_unlock();
 	return ret;
 }
 
@@ -402,8 +402,8 @@ static void stop_sessions(void)
 	struct ksmbd_transport *t;
 
 again:
-	read_lock(&conn_list_lock);
-	list_for_each_entry(conn, &conn_list, conns_list) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(conn, &conn_list, conns_list) {
 		struct task_struct *task;
 
 		t = conn->transport;
@@ -413,12 +413,12 @@ static void stop_sessions(void)
 				    task->comm, task_pid_nr(task));
 		conn->status = KSMBD_SESS_EXITING;
 		if (t->ops->shutdown) {
-			read_unlock(&conn_list_lock);
+			rcu_read_unlock();
 			t->ops->shutdown(t);
-			read_lock(&conn_list_lock);
+			rcu_read_lock();
 		}
 	}
-	read_unlock(&conn_list_lock);
+	rcu_read_unlock();
 
 	if (!list_empty(&conn_list)) {
 		schedule_timeout_interruptible(HZ / 10); /* 100ms */
diff --git a/fs/ksmbd/connection.h b/fs/ksmbd/connection.h
index 3643354a3fa7..e41f33a2da7c 100644
--- a/fs/ksmbd/connection.h
+++ b/fs/ksmbd/connection.h
@@ -139,7 +139,6 @@ struct ksmbd_transport {
 #define KSMBD_TCP_PEER_SOCKADDR(c)	((struct sockaddr *)&((c)->peer_addr))
 
 extern struct list_head conn_list;
-extern rwlock_t conn_list_lock;
 
 bool ksmbd_conn_alive(struct ksmbd_conn *conn);
 void ksmbd_conn_wait_idle(struct ksmbd_conn *conn);
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index 771e045c8d4a..c60cfb360f42 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -6915,8 +6915,8 @@ int smb2_lock(struct ksmbd_work *work)
 
 		nolock = 1;
 		/* check locks in connection list */
-		read_lock(&conn_list_lock);
-		list_for_each_entry(conn, &conn_list, conns_list) {
+		rcu_read_lock();
+		list_for_each_entry_rcu(conn, &conn_list, conns_list) {
 			spin_lock(&conn->llist_lock);
 			list_for_each_entry_safe(cmp_lock, tmp2, &conn->lock_list, clist) {
 				if (file_inode(cmp_lock->fl->fl_file) !=
@@ -6932,7 +6932,7 @@ int smb2_lock(struct ksmbd_work *work)
 						list_del(&cmp_lock->flist);
 						list_del(&cmp_lock->clist);
 						spin_unlock(&conn->llist_lock);
-						read_unlock(&conn_list_lock);
+						rcu_read_unlock();
 
 						locks_free_lock(cmp_lock->fl);
 						kfree(cmp_lock);
@@ -6954,7 +6954,7 @@ int smb2_lock(struct ksmbd_work *work)
 				    cmp_lock->start > smb_lock->start &&
 				    cmp_lock->start < smb_lock->end) {
 					spin_unlock(&conn->llist_lock);
-					read_unlock(&conn_list_lock);
+					rcu_read_unlock();
 					pr_err("previous lock conflict with zero byte lock range\n");
 					goto out;
 				}
@@ -6963,7 +6963,7 @@ int smb2_lock(struct ksmbd_work *work)
 				    smb_lock->start > cmp_lock->start &&
 				    smb_lock->start < cmp_lock->end) {
 					spin_unlock(&conn->llist_lock);
-					read_unlock(&conn_list_lock);
+					rcu_read_unlock();
 					pr_err("current lock conflict with zero byte lock range\n");
 					goto out;
 				}
@@ -6974,14 +6974,14 @@ int smb2_lock(struct ksmbd_work *work)
 				      cmp_lock->end >= smb_lock->end)) &&
 				    !cmp_lock->zero_len && !smb_lock->zero_len) {
 					spin_unlock(&conn->llist_lock);
-					read_unlock(&conn_list_lock);
+					rcu_read_unlock();
 					pr_err("Not allow lock operation on exclusive lock range\n");
 					goto out;
 				}
 			}
 			spin_unlock(&conn->llist_lock);
 		}
-		read_unlock(&conn_list_lock);
+		rcu_read_unlock();
 out_check_cl:
 		if (smb_lock->fl->fl_type == F_UNLCK && nolock) {
 			pr_err("Try to unlock nolocked range\n");
-- 
2.25.1

Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct access on conn list

[Sergey Senozhatsky]

On (23/01/15 18:32), Dawei Li wrote:
> Currently, racing protection on conn list is implemented as rwlock,
> improve it by rcu primitive for its better performance.

Why is this a problem and what proves that this patch improves/solves
it? Numbers/benchmarks/analysis?

Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct access on conn list

[Sergey Senozhatsky]

On (23/01/15 18:32), Dawei Li wrote:
> 
>  void ksmbd_conn_free(struct ksmbd_conn *conn)
>  {
> -	write_lock(&conn_list_lock);
> -	list_del(&conn->conns_list);
> -	write_unlock(&conn_list_lock);
> +	spin_lock(&conn_list_lock);
> +	list_del_rcu(&conn->conns_list);
> +	spin_unlock(&conn_list_lock);
>  
>  	xa_destroy(&conn->sessions);
>  	kvfree(conn->request_buf);

From a quick look this does not seem like a correct RCU usage. E.g.
where do you wait for grace periods and synchronize readers/writers?

Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct access on conn list

[Dawei Li]

Hi Sergey,

Thanks for reviewing,

On Mon, Jan 30, 2023 at 01:15:35PM +0900, Sergey Senozhatsky wrote:
> On (23/01/15 18:32), Dawei Li wrote:
> > 
> >  void ksmbd_conn_free(struct ksmbd_conn *conn)
> >  {
> > -	write_lock(&conn_list_lock);
> > -	list_del(&conn->conns_list);
> > -	write_unlock(&conn_list_lock);
> > +	spin_lock(&conn_list_lock);
> > +	list_del_rcu(&conn->conns_list);
> > +	spin_unlock(&conn_list_lock);
        synchronize_rcu(); 
> >  
> >  	xa_destroy(&conn->sessions);
> >  	kvfree(conn->request_buf);
> 
> From a quick look this does not seem like a correct RCU usage. E.g.
> where do you wait for grace periods and synchronize readers/writers?

Nice catch, I totally mess it up. Thanks!

At first glance, I assume synchronize_rcu() will do the job if sleeping
is OK?

Steve, Namjae,
Please drop this buggy patch from ksmbd-for-next.

Thanks,
     Dawei

Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct access on conn list

[Steve French]

On Mon, Jan 30, 2023 at 8:17 AM Dawei Li <set_pte_at@outlook.com> wrote:
...
> Steve, Namjae,
> Please drop this buggy patch from ksmbd-for-next.
>
> Thanks,
>      Dawei

Done


-- 
Thanks,

Steve

Re: [PATCH 3/6] ksmbd: replace rwlock with rcu for concurrenct access on conn list

[Sergey Senozhatsky]

Hi,

On (23/01/30 22:16), Dawei Li wrote:
> Hi Sergey,
> 
> Thanks for reviewing,
> 
> On Mon, Jan 30, 2023 at 01:15:35PM +0900, Sergey Senozhatsky wrote:
> > On (23/01/15 18:32), Dawei Li wrote:
> > > 
> > >  void ksmbd_conn_free(struct ksmbd_conn *conn)
> > >  {
> > > -	write_lock(&conn_list_lock);
> > > -	list_del(&conn->conns_list);
> > > -	write_unlock(&conn_list_lock);
> > > +	spin_lock(&conn_list_lock);
> > > +	list_del_rcu(&conn->conns_list);
> > > +	spin_unlock(&conn_list_lock);
>         synchronize_rcu(); 
> > >  
> > >  	xa_destroy(&conn->sessions);
> > >  	kvfree(conn->request_buf);
> > 
> > From a quick look this does not seem like a correct RCU usage. E.g.
> > where do you wait for grace periods and synchronize readers/writers?
> 
> Nice catch, I totally mess it up. Thanks!
> 
> At first glance, I assume synchronize_rcu() will do the job if sleeping
> is OK?

Yes, synchronize_rcu() will sleep (schedule()) and wait for grace
period to expire (and synchronize will all the RCU readers). RCU
is good for cases when writes are seldom, which may not be the case
with ksmb.

I really want to see benhcmarks, why do we want to remove the RW-lock.

[PATCH 4/6] ksmbd: Remove duplicated codes

[Dawei Li]

ksmbd_neg_token_init_mech_token() and ksmbd_neg_token_targ_resp_token()
share same implementation, unify them.

Signed-off-by: Dawei Li <set_pte_at@outlook.com>
---
 fs/ksmbd/asn1.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/ksmbd/asn1.c b/fs/ksmbd/asn1.c
index c03eba090368..c85e74fa78e5 100644
--- a/fs/ksmbd/asn1.c
+++ b/fs/ksmbd/asn1.c
@@ -208,7 +208,7 @@ int ksmbd_neg_token_init_mech_type(void *context, size_t hdrlen,
 	return 0;
 }
 
-int ksmbd_neg_token_init_mech_token(void *context, size_t hdrlen,
+static int ksmbd_neg_token_alloc(void *context, size_t hdrlen,
 				    unsigned char tag, const void *value,
 				    size_t vlen)
 {
@@ -223,17 +223,16 @@ int ksmbd_neg_token_init_mech_token(void *context, size_t hdrlen,
 	return 0;
 }
 
-int ksmbd_neg_token_targ_resp_token(void *context, size_t hdrlen,
+int ksmbd_neg_token_init_mech_token(void *context, size_t hdrlen,
 				    unsigned char tag, const void *value,
 				    size_t vlen)
 {
-	struct ksmbd_conn *conn = context;
-
-	conn->mechToken = kmalloc(vlen + 1, GFP_KERNEL);
-	if (!conn->mechToken)
-		return -ENOMEM;
+	return ksmbd_neg_token_alloc(context, hdrlen, tag, value, vlen);
+}
 
-	memcpy(conn->mechToken, value, vlen);
-	conn->mechToken[vlen] = '\0';
-	return 0;
+int ksmbd_neg_token_targ_resp_token(void *context, size_t hdrlen,
+				    unsigned char tag, const void *value,
+				    size_t vlen)
+{
+	return ksmbd_neg_token_alloc(context, hdrlen, tag, value, vlen);
 }
-- 
2.25.1

[PATCH 5/6] ksmbd: improve exception handling and avoid redundant sanity check in loop

[Dawei Li]

1. Sanity check on validity of hook is supposed to be static,
   move it from looping.

2. If exception occurs after kvmalloc(), kvfree() is supposed
   to reclaim memory to avoid mem leak.

Signed-off-by: Dawei Li <set_pte_at@outlook.com>
---
 fs/ksmbd/connection.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
index 36d1da273edd..b302de5db990 100644
--- a/fs/ksmbd/connection.c
+++ b/fs/ksmbd/connection.c
@@ -287,6 +287,12 @@ int ksmbd_conn_handler_loop(void *p)
 	mutex_init(&conn->srv_mutex);
 	__module_get(THIS_MODULE);
 
+	if (unlikely(!default_conn_ops.process_fn)) {
+		pr_err("No connection request callback\n");
+		module_put(THIS_MODULE);
+		return -EINVAL;
+	}
+
 	if (t->ops->prepare && t->ops->prepare(t))
 		goto out;
 
@@ -324,8 +330,10 @@ int ksmbd_conn_handler_loop(void *p)
 			break;
 
 		memcpy(conn->request_buf, hdr_buf, sizeof(hdr_buf));
-		if (!ksmbd_smb_request(conn))
+		if (!ksmbd_smb_request(conn)) {
+			pr_err("Invalid smb request\n");
 			break;
+		}
 
 		/*
 		 * We already read 4 bytes to find out PDU size, now
@@ -343,22 +351,18 @@ int ksmbd_conn_handler_loop(void *p)
 			continue;
 		}
 
-		if (!default_conn_ops.process_fn) {
-			pr_err("No connection request callback\n");
-			break;
-		}
-
 		if (default_conn_ops.process_fn(conn)) {
 			pr_err("Cannot handle request\n");
 			break;
 		}
 	}
 
+	kvfree(conn->request_buf);
+	conn->request_buf = NULL;
 out:
-	/* Wait till all reference dropped to the Server object*/
+	/* Wait till all reference dropped to the Server object */
 	wait_event(conn->r_count_q, atomic_read(&conn->r_count) == 0);
 
-
 	if (IS_ENABLED(CONFIG_UNICODE))
 		utf8_unload(conn->um);
 	unload_nls(conn->local_nls);
-- 
2.25.1

Re: [PATCH 5/6] ksmbd: improve exception handling and avoid redundant sanity check in loop

[Namjae Jeon]

2023-01-15 19:32 GMT+09:00, Dawei Li <set_pte_at@outlook.com>:
> 1. Sanity check on validity of hook is supposed to be static,
>    move it from looping.
>
> 2. If exception occurs after kvmalloc(), kvfree() is supposed
>    to reclaim memory to avoid mem leak.
>
> Signed-off-by: Dawei Li <set_pte_at@outlook.com>
> ---
>  fs/ksmbd/connection.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
> index 36d1da273edd..b302de5db990 100644
> --- a/fs/ksmbd/connection.c
> +++ b/fs/ksmbd/connection.c
> @@ -287,6 +287,12 @@ int ksmbd_conn_handler_loop(void *p)
>  	mutex_init(&conn->srv_mutex);
>  	__module_get(THIS_MODULE);
>
> +	if (unlikely(!default_conn_ops.process_fn)) {
> +		pr_err("No connection request callback\n");
> +		module_put(THIS_MODULE);
> +		return -EINVAL;
> +	}
Do you really need this check... ?
> +
>  	if (t->ops->prepare && t->ops->prepare(t))
>  		goto out;
>
> @@ -324,8 +330,10 @@ int ksmbd_conn_handler_loop(void *p)
>  			break;
>
>  		memcpy(conn->request_buf, hdr_buf, sizeof(hdr_buf));
> -		if (!ksmbd_smb_request(conn))
> +		if (!ksmbd_smb_request(conn)) {
> +			pr_err("Invalid smb request\n");
>  			break;
> +		}
>
>  		/*
>  		 * We already read 4 bytes to find out PDU size, now
> @@ -343,22 +351,18 @@ int ksmbd_conn_handler_loop(void *p)
>  			continue;
>  		}
>
> -		if (!default_conn_ops.process_fn) {
> -			pr_err("No connection request callback\n");
> -			break;
> -		}
> -
>  		if (default_conn_ops.process_fn(conn)) {
>  			pr_err("Cannot handle request\n");
>  			break;
>  		}
>  	}
>
> +	kvfree(conn->request_buf);
> +	conn->request_buf = NULL;
->request_buf is freed in ksmbd_conn_free(). ->request_buf should not
be freed here as the processing of the request may not be complete
here.

>  out:
> -	/* Wait till all reference dropped to the Server object*/
> +	/* Wait till all reference dropped to the Server object */
>  	wait_event(conn->r_count_q, atomic_read(&conn->r_count) == 0);
>
> -
>  	if (IS_ENABLED(CONFIG_UNICODE))
>  		utf8_unload(conn->um);
>  	unload_nls(conn->local_nls);
> --
> 2.25.1
>
>

[PATCH 6/6] ksmbd: fix typo, syncronous->synchronous

[Dawei Li]

syncronous->synchronous

Signed-off-by: Dawei Li <set_pte_at@outlook.com>
---
 fs/ksmbd/connection.c | 4 ++--
 fs/ksmbd/ksmbd_work.h | 2 +-
 fs/ksmbd/smb2pdu.c    | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c
index b302de5db990..0fcc20ce0852 100644
--- a/fs/ksmbd/connection.c
+++ b/fs/ksmbd/connection.c
@@ -114,7 +114,7 @@ void ksmbd_conn_enqueue_request(struct ksmbd_work *work)
 
 	if (conn->ops->get_cmd_val(work) != SMB2_CANCEL_HE) {
 		requests_queue = &conn->requests;
-		work->syncronous = true;
+		work->synchronous = true;
 	}
 
 	if (requests_queue) {
@@ -139,7 +139,7 @@ int ksmbd_conn_try_dequeue_request(struct ksmbd_work *work)
 	spin_lock(&conn->request_lock);
 	if (!work->multiRsp) {
 		list_del_init(&work->request_entry);
-		if (work->syncronous == false)
+		if (!work->synchronous)
 			list_del_init(&work->async_request_entry);
 		ret = 0;
 	}
diff --git a/fs/ksmbd/ksmbd_work.h b/fs/ksmbd/ksmbd_work.h
index 5ece58e40c97..3234f2cf6327 100644
--- a/fs/ksmbd/ksmbd_work.h
+++ b/fs/ksmbd/ksmbd_work.h
@@ -68,7 +68,7 @@ struct ksmbd_work {
 	/* Request is encrypted */
 	bool                            encrypted:1;
 	/* Is this SYNC or ASYNC ksmbd_work */
-	bool                            syncronous:1;
+	bool                            synchronous:1;
 	bool                            need_invalidate_rkey:1;
 
 	unsigned int                    remote_key;
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index c60cfb360f42..2ae153de4a9c 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -498,7 +498,7 @@ int init_smb2_rsp_hdr(struct ksmbd_work *work)
 	rsp_hdr->SessionId = rcv_hdr->SessionId;
 	memcpy(rsp_hdr->Signature, rcv_hdr->Signature, 16);
 
-	work->syncronous = true;
+	work->synchronous = true;
 	if (work->async_id) {
 		ksmbd_release_id(&conn->async_ida, work->async_id);
 		work->async_id = 0;
@@ -644,7 +644,7 @@ int setup_async_work(struct ksmbd_work *work, void (*fn)(void **), void **arg)
 		pr_err("Failed to alloc async message id\n");
 		return id;
 	}
-	work->syncronous = false;
+	work->synchronous = false;
 	work->async_id = id;
 	rsp_hdr->Id.AsyncId = cpu_to_le64(id);
 
-- 
2.25.1

Re: [PATCH 6/6] ksmbd: fix typo, syncronous->synchronous

[Sergey Senozhatsky]

On (23/01/15 18:32), Dawei Li wrote:
> 
> syncronous->synchronous
> 
> Signed-off-by: Dawei Li <set_pte_at@outlook.com>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>