Re: [PATCH net-next v18 08/15] quic: add path management

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] quic_udp_sock_create() in net/quic/path.c (line 100) always
  takes an active netns reference with get_net_track(net,…

--

quic: add path management

This patch adds struct quic_path_group to manage the active and alternate
QUIC paths. Each path is bound to a UDP tunnel socket, which is wrapped in
a refcounted struct quic_udp_sock and stored in a global hash table. The
patch also adds connection migration helpers and basic PLPMTUD state
handling.

> diff --git a/net/quic/path.c b/net/quic/path.c
> new file mode 100644
> index 0000000000000..e24968376f6b1
> --- /dev/null
> +++ b/net/quic/path.c
> @@ -0,0 +1,596 @@

[ ... ]

> +static void quic_udp_sock_put_work(struct work_struct *work)
> +{

[ ... ]

> +	head = quic_udp_sock_head(sock_net(sk), ntohs(us->addr.v4.sin_port));
> +	mutex_lock(&head->lock);
> +	__hlist_del(&us->node);
> +	mutex_unlock(&head->lock);
> +
> +	put_net_track(sock_net(sk), &us->ns_tracker);
> +	sock_put(sk);
> +	kfree(us);
> +}
> +
> +static struct quic_udp_sock *quic_udp_sock_create(struct sock *sk,
> +						  union quic_addr *a)
> +{

[ ... ]

> +	refcount_set(&us->refcnt, 1);
> +	us->sk = sock->sk;
> +	memcpy(&us->addr, a, sizeof(*a));
> +	us->bind_ifindex = udp_conf.bind_ifindex;
> +	get_net_track(net, &us->ns_tracker, GFP_KERNEL);

[Severity: Medium]
If the QUIC socket doing the bind is a kernel socket, can this active
netns reference keep the namespace alive forever?

quic_udp_sock_create() calls get_net_track() for every quic_udp_sock. The
only matching put_net_track() is in quic_udp_sock_put_work(). That work
runs only after the last path using the tunnel is unbound, which happens
through quic_destroy_sock()->quic_path_unbind() when the QUIC socket is
closed.

Kernel sockets are deliberately kept from holding an active netns
reference. sk_alloc() does:

	sk->sk_net_refcnt = kern ? 0 : 1;
	if (likely(sk->sk_net_refcnt)) {
		get_net_track(net, &sk->ns_tracker, priority);
		sock_inuse_add(net, 1);
	} else {
		net_passive_inc(net);
		...

The UDP tunnel socket created by udp_sock_create() follows that rule, but
the reference held by quic_udp_sock does not.

Consider an in-kernel user that creates a QUIC socket with
sock_create_kern() in a non-init netns. It then closes that socket from
its pernet ->exit handler, which is the usual pattern for netns-scoped
kernel sockets:

  quic_path_bind()->quic_udp_sock_create()
    get_net_track()            /* active netns ref taken */

  netns teardown:
    active netns count never drops to zero
      cleanup_net() and pernet ->exit never run
        kernel QUIC socket is never closed
          quic_udp_sock_put_work() never runs
            put_net_track() never runs

In that case, wouldn't the namespace and everything it owns be leaked?

No in-tree code creates IPPROTO_QUIC kernel sockets yet, either at this
commit or at the end of the series. So this depends on a future
in-kernel user. The code is still the same at the end of the series.

For user sockets, the extra active reference adds nothing while the
socket is alive, because the socket already holds one. Would a passive
reference (net_passive_inc()/net_passive_dec()) be enough to keep struct
net valid for sock_net() and quic_udp_sock_head() in the deferred work?
Another option would be to follow the owning socket's sk_net_refcnt
semantics.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794241654.git.lucien.xin%40gmail.com