Re: [PATCH] net/smc: release the internal TCP sock on IPPROTO_SMC socket creation failure

On 12/08/26 12:45 pm, Chuyf26 wrote:
> IPPROTO_SMC sockets wrap an internal TCP sock ("clcsock"), which is
> created by smc_inet_init_sock() via smc_create_clcsk() from the
> proto->init hook of inet_create()/inet6_create(). When socket
> creation fails after proto->init has succeeded - for example when a
> cgroup BPF program attached to BPF_CGROUP_INET_SOCK_CREATE denies the
> socket - inet_create() calls sk_common_release(), which only invokes
> sk_prot->destroy if it is set. Neither smc_inet_prot nor
> smc_inet6_prot defines .destroy, and the sock destructor smc_destruct()
> returns early unless sk_state is SMC_CLOSED (it is SMC_INIT here), so
> the internal TCP sock is never released.
> 
> As a result, every failing socket(AF_INET, SOCK_STREAM, IPPROTO_SMC)
> call leaks one tcp_sock. Any unprivileged task able to attach a
> deny-all BPF_CGROUP_INET_SOCK_CREATE program to its own cgroup (or a
> task confined by an LSM policy) can grow kernel memory unboundedly.
> 
> Reproduced on v7.2-rc7: with a deny-all BPF_CGROUP_INET_SOCK_CREATE
> program attached, a loop of socket(AF_INET, SOCK_STREAM, IPPROTO_SMC)
> calls fails with EPERM and kmemleak reports one unreferenced tcp_sock
> object per call.
> 
> Add a .destroy hook to both IPPROTO_SMC protos that releases the
> clcsock via smc_clcsock_release(). That helper is safe here: it takes
> the clcsock_release_lock initialized by smc_sk_init() and skips a NULL
> clcsock, which is what smc_create_clcsk() leaves behind when creating
> the TCP sock fails. Also initialize clcsock to NULL at the start of
> smc_inet_init_sock(): the smc_sock slab is SLAB_TYPESAFE_BY_RCU, so
> recycled objects are not zeroed and stale memory must not be handed to
> sock_release().
> 
> The same behaviour is visible without any debugging option: on a
> plain kernel the slabinfo "TCP" active_objs count grows by one per
> failing socket() call and never shrinks.
> 
> With the fix, the same reproducer leaves no unreferenced objects in
> kmemleak and the "TCP" slabinfo count stays flat, and regular
> IPPROTO_SMC socket create/close cycles are unaffected
> (sk_common_release() from inet_release() also routes through the new
> .destroy, where the already-NULL clcsock is a no-op).
> 

The fix looks good, but I think the commit message is longer than needed
and spends too much space on reproducer details and internal call path
narration.
I would trim the detailed reproducer/results text and most of the
internal call path explanation, and keep it focused on the leak, the
failure path, and why adding .destroy plus clcsock = NULL fixes the issue.
If you want to keep the reproducer and validation details, please move
those below `...` instead of keeping them in the main commit message body.

Thank you,
Sidraya
> Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC")
> Reported-by: Abaci <[email protected]>
> Assisted-by: abaci:qwen3.8-max
> Signed-off-by: Chuyf26 <[email protected]>
> ---
>  net/smc/smc_inet.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> index a94084b..b94a194 100644
> --- a/net/smc/smc_inet.c
> +++ b/net/smc/smc_inet.c
> @@ -15,13 +15,16 @@
>  
>  #include "smc_inet.h"
>  #include "smc.h"
> +#include "smc_close.h"
>  
>  static int smc_inet_init_sock(struct sock *sk);
> +static void smc_inet_destroy_sock(struct sock *sk);
>  
>  static struct proto smc_inet_prot = {
>  	.name		= "INET_SMC",
>  	.owner		= THIS_MODULE,
>  	.init		= smc_inet_init_sock,
> +	.destroy	= smc_inet_destroy_sock,
>  	.hash		= smc_hash_sk,
>  	.unhash		= smc_unhash_sk,
>  	.release_cb	= smc_release_cb,
> @@ -68,6 +71,7 @@ static struct proto smc_inet6_prot = {
>  	.name		= "INET6_SMC",
>  	.owner		= THIS_MODULE,
>  	.init		= smc_inet_init_sock,
> +	.destroy	= smc_inet_destroy_sock,
>  	.hash		= smc_hash_sk,
>  	.unhash		= smc_unhash_sk,
>  	.release_cb	= smc_release_cb,
> @@ -109,6 +113,14 @@ static struct inet_protosw smc_inet6_protosw = {
>  static int smc_inet_init_sock(struct sock *sk)
>  {
>  	struct net *net = sock_net(sk);
> +	struct smc_sock *smc = smc_sk(sk);
> +
> +	/*
> +	 * The smc_sock slab is SLAB_TYPESAFE_BY_RCU and recycled objects
> +	 * are not zeroed. .destroy may run even if .init never completed,
> +	 * so make sure smc_clcsock_release() sees a valid clcsock.
> +	 */
> +	smc->clcsock = NULL;
>  
>  	/* init common smc sock */
>  	smc_sk_init(net, sk, IPPROTO_SMC);
> @@ -116,6 +128,17 @@ static int smc_inet_init_sock(struct sock *sk)
>  	return smc_create_clcsk(net, sk, sk->sk_family);
>  }
>  
> +static void smc_inet_destroy_sock(struct sock *sk)
> +{
> +	/*
> +	 * If inet_create()/inet6_create() fail after .init has created the
> +	 * internal TCP sock (e.g. rejected by a cgroup BPF program),
> +	 * sk_common_release() ends up here. Release the TCP sock, otherwise
> +	 * it leaks on every failed IPPROTO_SMC socket() call.
> +	 */
> +	smc_clcsock_release(smc_sk(sk));
> +}
> +
>  int __init smc_inet_init(void)
>  {
>  	int rc;