From: Lorenz Bauer <lmb@xxxxxxxxxxxxx>
Date: Tue, 11 Jul 2023 17:15:06 +0100
> 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?
Ah sorry, I assumed it would not happen, but if we can put unbound
TCP/UDP socket into a map and select it, then yes, it hits the warning.
Let's say we can select a non-RCU sk in bpf_sk_assign() and then the
socket is converted to RCU by bind(udp_sk) or listen(tcp_sk).
The sk_is_refcounted() in bpf_sk_assign() returns true and sk_refcnt
is incremented. Then, I think of two scenarios:
1) RCU conversion is done before sk_is_refcounted() in skb_steal_sock().
-> *refcounted is false
2) RCU conversion is done after skb_steal_sock().
-> *refcounted is true
In both cases, we need to decrement the refcnt that is bumped up
by bpf_sk_assign(). The sock_put() in the v1 series does not catch
the former case.
How should we track it ?
>
> 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.
If the issue above happened, I think it could be a blocker.
Thanks!
