From: Lorenz Bauer lmb@isovalent.com Date: Tue, 11 Jul 2023 17:15:06 +0100
On Tue, Jul 4, 2023 at 2:46 PM Lorenz Bauer lmb@isovalent.com 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!