Eric Dumazet wrote:
>
>
> On 3/31/21 4:32 AM, Cong Wang wrote:
> > From: Cong Wang <[email protected]>
> >
> > Currently sockmap calls into each protocol to update the struct
> > proto and replace it. This certainly won't work when the protocol
> > is implemented as a module, for example, AF_UNIX.
> >
> > Introduce a new ops sk->sk_prot->psock_update_sk_prot(), so each
> > protocol can implement its own way to replace the struct proto.
> > This also helps get rid of symbol dependencies on CONFIG_INET.
>
> [...]
>
>
> >
> > -struct proto *tcp_bpf_get_proto(struct sock *sk, struct sk_psock *psock)
> > +int tcp_bpf_update_proto(struct sock *sk, bool restore)
> > {
> > + struct sk_psock *psock = sk_psock(sk);
>
> I do not think RCU is held here ?
Hi, thanks for looking at this.
>
> sk_psock() is using rcu_dereference_sk_user_data()
First caller of this is here,
sock_{hash|map}_update_common <- has a WARN_ON_ONCE(!rcu_read_lock_held);
sock_map_link()
sock_map_init_proto()
psock_update_sk_prot(sk, false)
And the other does this,
sk_psock_put()
sk_psock_drop()
sk_psock_restore_proto
psock_update_sk_prot(sk, true)
But we can get here through many callers and it sure doesn't look like its
all safe. For example one case,
.sendmsg
tcp_bpf_sendmsg
psock = sk_psock_get(sk)
sk_psock_put(sk, psock) <- this doesn't have the RCU held
>
> > int family = sk->sk_family == AF_INET6 ? TCP_BPF_IPV6 : TCP_BPF_IPV4;
> > int config = psock->progs.msg_parser ? TCP_BPF_TX : TCP_BPF_BASE;
> >
>
> Same issue in udp_bpf_update_proto() of course.
>
Yep.
Either we revert the patch or we can fix it to pass the psock through.
Passing the psock works because we have a reference on it and it wont
go away. I don't have any other good ideas off-hand.
Thanks Eric! I'm a bit surprised we didn't get an RCU splat from the
tests though.
.John
