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

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net/smc: release the internal TCP sock on IPPROTO_SMC socket creation failure

This adds a .destroy hook to smc_inet_prot and smc_inet6_prot that calls
smc_clcsock_release(), so the internal TCP sock created by
smc_inet_init_sock() is freed when socket creation fails after proto->init
has run.  It also stores NULL into smc->clcsock at the top of
smc_inet_init_sock().

A couple of questions below.

> diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c
> index a94084b4a498..b94a19499986 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,

[ ... ]

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

[Severity: Low]
Is the premise of this comment (and of the matching changelog paragraph,
"Also initialize clcsock to NULL when setting the sock up: the smc_sock
slab is SLAB_TYPESAFE_BY_RCU, so recycled objects are not zeroed") correct?

sk_alloc() always adds __GFP_ZERO, and sk_prot_alloc() then clears the
whole protocol object:

net/core/sock.c:sk_alloc() {
	sk = sk_prot_alloc(prot, priority | __GFP_ZERO, family);
	...
}

net/core/sock.c:sk_prot_alloc() {
	sk = kmem_cache_alloc(slab, priority & ~__GFP_ZERO);
	if (!sk)
		return sk;
	if (want_init_on_alloc(priority))
		sk_prot_clear_nulls(sk, prot->obj_size);
	...
}

sk_prot_clear_nulls() memsets everything except sk_node.next, and
smc_sock.clcsock sits well past sk_node, so isn't clcsock already NULL on
entry to .init even for a recycled SLAB_TYPESAFE_BY_RCU object, making this
store dead code?

The second sentence of the comment also seems hard to satisfy: the mutex
that smc_clcsock_release() takes is only initialized later, inside
smc_sk_init():

net/smc/af_smc.c:smc_sk_init() {
	...
	mutex_init(&smc->clcsock_release_lock);
	...
}

So if .destroy really could run before .init completed, wouldn't
smc_clcsock_release() lock an uninitialized mutex regardless of this NULL
store?  Could the comment and changelog paragraph be dropped or reworded to
match what the code actually guarantees?

>  
>  	/* 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));
> +}

[Severity: High]
Can this free the internal TCP socket while the SMC sock is still on the
SMC diag hash list?

sk_common_release() runs .destroy before .unhash:

net/core/sock.c:sk_common_release() {
	if (sk->sk_prot->destroy)
		sk->sk_prot->destroy(sk);
	...
	sk->sk_prot->unhash(sk);
	...
}

and the sk was already inserted into smc_v4_hashinfo/smc_v6_hashinfo by
smc_sk_init() -> sk->sk_prot->hash(sk) == smc_hash_sk() before
smc_create_clcsk() ran, so when smc_inet_destroy_sock() calls
smc_clcsock_release() the sock is still visible to the diag dumper.

The reader side walks that same hashinfo holding only the hashinfo rwlock,
and does an unlocked check-then-dereference of smc->clcsock:

net/smc/smc_diag.c:smc_diag_dump_proto() {
	read_lock(&prot->h.smc_hash->lock);
	head = &prot->h.smc_hash->ht;
	...
	sk_for_each(sk, head) {
	...
}

net/smc/smc_diag.c:smc_diag_msg_common_fill() {
	...
	if (!smc->clcsock)
		return;
	r->id.idiag_sport = htons(smc->clcsock->sk->sk_num);
	r->id.idiag_dport = smc->clcsock->sk->sk_dport;
	...
}

while the writer holds only clcsock_release_lock:

net/smc/smc_close.c:smc_clcsock_release() {
	mutex_lock(&smc->clcsock_release_lock);
	if (smc->clcsock) {
		tcp = smc->clcsock;
		smc->clcsock = NULL;
		sock_release(tcp);
	}
	...
}

With an empty lockset intersection, can a concurrent AF_SMC diag dump (for
example "ss --smc", which has no capability check in
smc_diag_handler_dump()) re-read smc->clcsock as NULL after the NULL check
and oops, or dereference the just-released struct socket / tcp_sock and copy
freed memory into the netlink reply?

The existing teardown path unhashes first and only then drops the clcsock:

net/smc/af_smc.c:__smc_release() {
	sk->sk_prot->unhash(sk);

	if (sk->sk_state == SMC_CLOSED) {
		if (smc->clcsock) {
			release_sock(sk);
			smc_clcsock_release(smc);
	...
}

Would it be better to keep that ordering here, for instance by unhashing
before releasing the clcsock in the destroy path, or by taking
clcsock_release_lock in the diag reader?

The lockless clcsock read in smc_diag is itself older than this patch (see
also smc_close_active_abort() and smc_close_passive_work()), but on the
socket-creation-failure path nothing was freed while the sk was hashed
before this change, since the clcsock was simply leaked.

>  
>  int __init smc_inet_init(void)
>  {
>  	int rc;
-- 
This is an AI-generated review.