Re: [PATCH net 1/4] rxrpc: Fix memory leaks in rxkad_verify_response()

[Simon Horman <horms@kernel.org>]

On Mon, Apr 20, 2026 at 03:58:54PM +0100, David Howells wrote:
> Fix rxkad_verify_response() to free ticket by using a __free() construct
> rather than explicitly freeing it.
> 
> Also fix rxkad_verify_response() to free the server key by using a __free()
> construct.
> 
> Fixes: 57af281e5389 ("rxrpc: Tidy up abort generation infrastructure")
> Fixes: ec832bd06d6f ("rxrpc: Don't retain the server key in the connection")
> Closes: https://sashiko.dev/#/patchset/20260408121252.2249051-1-dhowells%40redhat.com
> Signed-off-by: David Howells <dhowells@redhat.com>

...

> index eb7f2769d2b1..0acdc46f42c2 100644
> --- a/net/rxrpc/rxkad.c
> +++ b/net/rxrpc/rxkad.c

...

> @@ -1160,16 +1159,15 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
>  	}
>  
>  	ret = -ENOMEM;
> -	response = kzalloc_obj(struct rxkad_response, GFP_NOFS);
> +	struct rxkad_response *response __free(kfree) =
> +		kzalloc_obj(struct rxkad_response, GFP_NOFS);
>  	if (!response)
>  		goto temporary_error;
>  

Hi David,

This goto, combined with the use of __free in the declaration
of ticket below results in a compile error for x86_64 allmodconfig
with clang 21.1.8.

  net/rxrpc/rxkad.c:1165:3: error: cannot jump from this goto statement to its label
   1165 |                 goto temporary_error;
        |                 ^
  net/rxrpc/rxkad.c:1192:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
   1192 |         void *ticket __free(kfree) = kmalloc(ticket_len, GFP_NOFS);
        |               ^

Moreover, the use of this construct is discouraged in Networking code:

  1.7.3. Using device-managed and cleanup.h constructs¶

  Netdev remains skeptical about promises of all “auto-cleanup” APIs,
  including even devm_ helpers, historically. They are not the preferred
  style of implementation, merely an acceptable one.

  Use of guard() is discouraged within any function longer than 20 lines,
  scoped_guard() is considered more readable. Using normal lock/unlock is
  still (weakly) preferred.

  Low level cleanup constructs (such as __free()) can be used when building
  APIs and helpers, especially scoped iterators. However, direct use of
  __free() within networking core and drivers is discouraged. Similar
  guidance applies to declaring variables mid-function.

  https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

And to round things out, Sashiko also points out problems with
the use of __free() in this patch.

...

>  
>  	/* extract the kerberos ticket and decrypt and decode it */
>  	ret = -ENOMEM;
> -	ticket = kmalloc(ticket_len, GFP_NOFS);
> +	void *ticket __free(kfree) = kmalloc(ticket_len, GFP_NOFS);
>  	if (!ticket)
> -		goto temporary_error_free_resp;
> +		goto temporary_error;

...

>  temporary_error:
>  	/* Ignore the response packet if we got a temporary error such as
>  	 * ENOMEM.  We just want to send the challenge again.  Note that we
>  	 * also come out this way if the ticket decryption fails.
>  	 */
> -	key_put(server_key);
>  	return ret;
>  }
>  
> 

-- 
pw-bot: changes-requested