Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <TYCP286MB23233D324DE28E57E376228ECAC09@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
References: <20230115103209.146002-1-set_pte_at@outlook.com> <TYCP286MB23233D324DE28E57E376228ECAC09@TYCP286MB2323.JPNP286.PROD.OUTLOOK.COM>
From:   Namjae Jeon <linkinjeon@kernel.org>
Date:   Mon, 16 Jan 2023 23:38:54 +0900
Message-ID: <CAKYAXd93UvZP0DVrvqFMY7nF0KmU+HMXDcOW2ESDm2ikoq8ubA@mail.gmail.com>
Subject: Re: [PATCH 5/6] ksmbd: improve exception handling and avoid redundant
 sanity check in loop
To:     Dawei Li <set_pte_at@outlook.com>
Cc:     sfrench@samba.org, senozhatsky@chromium.org, tom@talpey.com,
        hyc.lee@gmail.com, linux-cifs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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