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@xxxxxxxxxxx>:
> 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@xxxxxxxxxxx>
> ---
>  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
>
>