Re: [PATCH net 2/2] connector/cn_proc: release socket state and listener count on close

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch makes cn_proc_release() (drivers/connector/cn_proc.c)
  the single place that cleans up when a NETLINK_CONNECTOR socket closes.

--

connector/cn_proc: release socket state and listener count on close

This moves the NETLINK_CONNECTOR release cleanup into a new
cn_proc_release() helper, which now runs for every socket on close. The
helper frees sk->sk_user_data and decrements proc_event_num_listeners
when the stored mcast_op is PROC_CN_MCAST_LISTEN. The aim is to fix the
proc_input leak and the listener count imbalance.

> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index 4f613c0648ae4..a4b62998095ea 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -387,6 +387,20 @@ static void cn_proc_ack(int err, int rcvd_seq, int rcvd_ack)
>  	send_msg(msg);
>  }
>  
> +/* Called when a NETLINK_CONNECTOR socket is released. */
> +void cn_proc_release(struct sock *sk)
> +{
> +	struct proc_input *input = sk->sk_user_data;
> +
> +	if (!input)
> +		return;
> +
> +	if (input->mcast_op == PROC_CN_MCAST_LISTEN)
> +		atomic_dec(&proc_event_num_listeners);
> +	kfree(input);
> +	sk->sk_user_data = NULL;
> +}

[Severity: High]
This cleanup assumes two things. Each socket owns at most one
proc_input, and proc_event_num_listeners holds exactly one increment for
each socket whose stored mcast_op is LISTEN. Does cn_proc_mcast_ctl()
actually guarantee either of these?

Nothing on the send path serialises it per socket. In
netlink_sendmsg()->netlink_unicast()->netlink_unicast_kernel(),
nlk->netlink_rcv() is called with no lock_sock(). cn_call_callback()
holds queue_lock only for the lookup:

drivers/connector/connector.c:cn_call_callback() {
    ...
	spin_unlock_bh(&dev->cbdev->queue_lock);

	if (cbq != NULL) {
		cbq->callback(msg, nsp);
    ...
}

As a result, two threads sharing one fd can run this section of
cn_proc_mcast_ctl() at the same time:

drivers/connector/cn_proc.c:cn_proc_mcast_ctl() {
    ...
		if (sk->sk_user_data == NULL) {
			sk->sk_user_data = kzalloc_obj(struct proc_input);
			...
			initial = 1;
		} else {
			prev_mc_op =
			((struct proc_input *)(sk->sk_user_data))->mcast_op;
		}
		...
		((struct proc_input *)(sk->sk_user_data))->mcast_op = mc_op;
	...
	case PROC_CN_MCAST_LISTEN:
		if (initial || (prev_mc_op != PROC_CN_MCAST_LISTEN))
			atomic_inc(&proc_event_num_listeners);
		break;
	case PROC_CN_MCAST_IGNORE:
		if (!initial && (prev_mc_op != PROC_CN_MCAST_IGNORE))
			atomic_dec(&proc_event_num_listeners);
    ...
}

Can this still leak the proc_input? If both threads see sk_user_data as
NULL, each calls kzalloc and the second store overwrites the first
pointer. cn_proc_release() frees only the pointer that survives. A loop
of socket(), two concurrent control messages and close() would then
still leak kmalloc-8, much like the OOM reproducer in the commit
message.

Can the count still be left too high? Both threads can take the
increment path, for example when both are initial LISTENs, or when the
second thread reads the freshly published zeroed mcast_op (0, which is
not PROC_CN_MCAST_LISTEN) before the first thread stores LISTEN.
cn_proc_release() decrements only once.

There is also an interleaving where the new release-time decrement
seems to push the count too low:

thread A (LISTEN, fresh socket)
    sk->sk_user_data = kzalloc_obj(...)   /* mcast_op == 0 */
    initial = 1

thread B (IGNORE)
    prev_mc_op = 0
    atomic_dec(&proc_event_num_listeners)
    mcast_op = PROC_CN_MCAST_IGNORE

thread A
    mcast_op = PROC_CN_MCAST_LISTEN
    atomic_inc(&proc_event_num_listeners)

close()
    cn_proc_release()
        atomic_dec(&proc_event_num_listeners)

Before this patch that sequence left the counter balanced. With this
patch, each round lowers proc_event_num_listeners by one.

Once the count drops below 1, proc_fork_connector() and the other
proc_*_connector() hooks, as well as cn_proc_ack(), return early. Would
that stop process events for every listener on the system? The
reachability checks are cn_bind() for CN_IDX_PROC and the init user/pid
namespace test in cn_proc_mcast_ctl(), so an unprivileged user can
trigger this.

Would it make sense to serialise cn_proc_mcast_ctl() per socket, for
example with lock_sock(sk) or a mutex around the sk_user_data install
and the mcast_op transition plus counter update? Another option is a
cmpxchg() install of sk_user_data together with an xchg() of mcast_op,
using the old value to pick the inc or dec.

>  /**
>   * cn_proc_mcast_ctl
>   * @msg: message sent from userspace via the connector

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102130922.75629-1-cenzhang%40linux.microsoft.com