diff mbox series

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

Message ID TYCP286MB23233D324DE28E57E376228ECAC09@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM
State New
Headers
Series ksmbd: Minor performance improvement & code cleanup |

Commit Message

Dawei Li Jan. 15, 2023, 10:32 a.m. UTC 
  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(-)

Comments

Namjae Jeon Jan. 16, 2023, 2:38 p.m. UTC | #1 
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
>
>
diff mbox series

Patch

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);