On Thu, May 20, 2021 at 05:51:17PM +0900, Kuniyuki Iwashima wrote:
> From: Martin KaFai Lau <kafai@xxxxxx>
> Date: Wed, 19 May 2021 23:26:48 -0700
> > On Mon, May 17, 2021 at 09:22:50AM +0900, Kuniyuki Iwashima wrote:
> >
> > > +static int reuseport_resurrect(struct sock *sk, struct sock_reuseport *old_reuse,
> > > + struct sock_reuseport *reuse, bool bind_inany)
> > > +{
> > > + if (old_reuse == reuse) {
> > > + /* If sk was in the same reuseport group, just pop sk out of
> > > + * the closed section and push sk into the listening section.
> > > + */
> > > + __reuseport_detach_closed_sock(sk, old_reuse);
> > > + __reuseport_add_sock(sk, old_reuse);
> > > + return 0;
> > > + }
> > > +
> > > + if (!reuse) {
> > > + /* In bind()/listen() path, we cannot carry over the eBPF prog
> > > + * for the shutdown()ed socket. In setsockopt() path, we should
> > > + * not change the eBPF prog of listening sockets by attaching a
> > > + * prog to the shutdown()ed socket. Thus, we will allocate a new
> > > + * reuseport group and detach sk from the old group.
> > > + */
> > For the reuseport_attach_prog() path, I think it needs to consider
> > the reuse->num_closed_socks != 0 case also and that should belong
> > to the resurrect case. For example, when
> > sk_unhashed(sk) but sk->sk_reuseport == 0.
>
> In the path, reuseport_resurrect() is called from reuseport_alloc() only
> if reuse->num_closed_socks != 0.
>
>
> > @@ -92,6 +117,14 @@ int reuseport_alloc(struct sock *sk, bool bind_inany)
> > reuse = rcu_dereference_protected(sk->sk_reuseport_cb,
> > lockdep_is_held(&reuseport_lock));
> > if (reuse) {
> > + if (reuse->num_closed_socks) {
>
> But, should this be
>
> if (sk->sk_state == TCP_CLOSE && reuse->num_closed_socks)
>
> because we need not allocate a new group when we attach a bpf prog to
> listeners?
The reuseport_alloc() is fine as is. No need to change.
I should have copied reuseport_attach_prog() in the last reply and
commented there instead.
I meant reuseport_attach_prog() needs a change. In reuseport_attach_prog(),
iiuc, currently passing the "else if (!rcu_access_pointer(sk->sk_reuseport_cb))"
check implies the sk was (and still is) hashed with sk_reuseport enabled
because the current behavior would have set sk_reuseport_cb to NULL during
unhash but it is no longer true now. For example, this will break:
1. shutdown(lsk); /* lsk was bound with sk_reuseport enabled */
2. setsockopt(lsk, ..., SO_REUSEPORT, &zero, ...); /* disable sk_reuseport */
3. setsockopt(lsk, ..., SO_ATTACH_REUSEPORT_EBPF, &prog_fd, ...);
^---- /* This will work now because sk_reuseport_cb is not NULL.
* However, it shouldn't be allowed.
*/
I am thinking something like this (uncompiled code):
int reuseport_attach_prog(struct sock *sk, struct bpf_prog *prog)
{
struct sock_reuseport *reuse;
struct bpf_prog *old_prog;
if (sk_unhashed(sk)) {
int err;
if (!sk->sk_reuseport)
return -EINVAL;
err = reuseport_alloc(sk, false);
if (err)
return err;
} else if (!rcu_access_pointer(sk->sk_reuseport_cb)) {
/* The socket wasn't bound with SO_REUSEPORT */
return -EINVAL;
}
/* ... */
}
WDYT?
