netdev - Re: [PATCH v1] net/handshake: Fix handshake_req_destroy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 7 Feb 2024 10:21:46 +0900
From: Hannes Reinecke <hare@...e.de>
To: Chuck Lever <cel@...nel.org>, kernel-tls-handshake@...ts.linux.dev,
 netdev@...r.kernel.org
Cc: Chuck Lever <chuck.lever@...cle.com>
Subject: Re: [PATCH v1] net/handshake: Fix handshake_req_destroy_test1

On 2/7/24 03:16, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@...cle.com>
> 
> Recently, handshake_req_destroy_test1 started failing:
> 
> Expected handshake_req_destroy_test == req, but
>      handshake_req_destroy_test == 0000000000000000
>      req == 0000000060f99b40
> not ok 11 req_destroy works
> 
> This is because "sock_release(sock)" was replaced with "fput(filp)"
> to address a memory leak. Note that sock_release() is synchronous
> but fput() usually delays the final close and clean-up.
> 
> The delay is not consequential in the other cases that were changed
> but handshake_req_destroy_test1 is testing that handshake_req_cancel()
> followed by closing the file actually does call the ->hp_destroy
> method. Thus the PTR_EQ test at the end has to be sure that the
> final close is complete before it checks the pointer.
> 
> We cannot use a completion here because if ->hp_destroy is never
> called (ie, there is an API bug) then the test will hang.
> 
> Reported by: Guenter Roeck <linux@...ck-us.net>
> Closes: https://lore.kernel.org/netdev/ZcKDd1to4MPANCrn@tissot.1015granger.net/T/#mac5c6299f86799f1c71776f3a07f9c566c7c3c40
> Fixes: 4a0f07d71b04 ("net/handshake: Fix memory leak in __sock_create() and sock_alloc_file()")
> Signed-off-by: Chuck Lever <chuck.lever@...cle.com>
> ---
>   net/handshake/handshake-test.c |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/handshake/handshake-test.c b/net/handshake/handshake-test.c
> index 16ed7bfd29e4..34fd1d9b2db8 100644
> --- a/net/handshake/handshake-test.c
> +++ b/net/handshake/handshake-test.c
> @@ -471,7 +471,10 @@ static void handshake_req_destroy_test1(struct kunit *test)
>   	handshake_req_cancel(sock->sk);
>   
>   	/* Act */
> -	fput(filp);
> +	/* Ensure the close/release/put process has run to
> +	 * completion before checking the result.
> +	 */
> +	__fput_sync(filp);
>   
>   	/* Assert */
>   	KUNIT_EXPECT_PTR_EQ(test, handshake_req_destroy_test, req);
> 
> 
> 
Reviewed-by: Hannes Reinecke <hare@...e.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@...e.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich