Re: [PATCH v6 bpf-next 03/11] tcp: Keep TCP_CLOSE sockets in the reuseport group.

On Thu, May 20, 2021 at 05:51:17PM +0900, Kuniyuki Iwashima wrote:

From:   Martin KaFai Lau <redacted>
Date:   Wed, 19 May 2021 23:26:48 -0700

quoted

On Mon, May 17, 2021 at 09:22:50AM +0900, Kuniyuki Iwashima wrote:

quoted

+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.

quoted

@@ -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?