On Tue, Jul 4, 2023 at 2:46 PM Lorenz Bauer <lmb@xxxxxxxxxxxxx> wrote:
>
> +static inline
> +struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
> + const struct in6_addr *saddr, const __be16 sport,
> + const struct in6_addr *daddr, const __be16 dport,
> + bool *refcounted, inet6_ehashfn_t *ehashfn)
> +{
> + struct sock *sk, *reuse_sk;
> + bool prefetched;
> +
> + sk = skb_steal_sock(skb, refcounted, &prefetched);
> + if (!sk)
> + return NULL;
> +
> + if (!prefetched)
> + return sk;
> +
> + if (sk->sk_protocol == IPPROTO_TCP) {
> + if (sk->sk_state != TCP_LISTEN)
> + return sk;
> + } else if (sk->sk_protocol == IPPROTO_UDP) {
> + if (sk->sk_state != TCP_CLOSE)
> + return sk;
> + } else {
> + return sk;
> + }
> +
> + reuse_sk = inet6_lookup_reuseport(net, sk, skb, doff,
> + saddr, sport, daddr, ntohs(dport),
> + ehashfn);
> + if (!reuse_sk)
> + return sk;
> +
> + /* We've chosen a new reuseport sock which is never refcounted. This
> + * implies that sk also isn't refcounted.
> + */
> + WARN_ON_ONCE(*refcounted);
> +
> + return reuse_sk;
> +}
Hi Kuniyuki,
Continuing the conversation from v5 of the patch set, you wrote:
In inet6?_steal_sock(), we call inet6?_lookup_reuseport() only for
sk that was a TCP listener or UDP non-connected socket until just before
the sk_state checks. Then, we know *refcounted should be false for such
sockets even before inet6?_lookup_reuseport().
This makes sense for me in the TCP listener case. I understand UDP
less, so I'll have to rely on your input. I tried to convince myself
that all UDP sockets in TCP_CLOSE have SOCK_RCU_FREE set. However, the
only place I see sock_set_flag(sk, SOCK_RCU_FREE) in the UDP case is
in udp_lib_get_port(). That in turn seems to be called during bind.
So, what if BPF does bpf_sk_assign() of an unbound and unconnected
socket? Wouldn't that trigger the warning?
To maybe sidestep this question: do you think the location of the
WARN_ON_ONCE has to prevent this patch set from going in? I've been
noodling at it for quite a while already and it would be good to see
it land.
Thanks
Lorenz
