| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201208090236.86926-1-kuniyu@amazon.co.jp>
Date: Tue, 8 Dec 2020 18:02:36 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.co.jp>
To: <kafai@...com>
CC: <ast@...nel.org>, <benh@...zon.com>, <bpf@...r.kernel.org>,
<daniel@...earbox.net>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <kuni1840@...il.com>,
<kuniyu@...zon.co.jp>, <linux-kernel@...r.kernel.org>,
<netdev@...r.kernel.org>
Subject: Re: [PATCH v1 bpf-next 03/11] tcp: Migrate TCP_ESTABLISHED/TCP_SYN_RECV sockets in accept queues.
From: Martin KaFai Lau <kafai@...com>
Date: Tue, 8 Dec 2020 00:13:28 -0800
> On Tue, Dec 08, 2020 at 03:27:14PM +0900, Kuniyuki Iwashima wrote:
> > From: Martin KaFai Lau <kafai@...com>
> > Date: Mon, 7 Dec 2020 12:14:38 -0800
> > > On Sun, Dec 06, 2020 at 01:03:07AM +0900, Kuniyuki Iwashima wrote:
> > > > From: Martin KaFai Lau <kafai@...com>
> > > > Date: Fri, 4 Dec 2020 17:42:41 -0800
> > > > > On Tue, Dec 01, 2020 at 11:44:10PM +0900, Kuniyuki Iwashima wrote:
> > > > > [ ... ]
> > > > > > diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
> > > > > > index fd133516ac0e..60d7c1f28809 100644
> > > > > > --- a/net/core/sock_reuseport.c
> > > > > > +++ b/net/core/sock_reuseport.c
> > > > > > @@ -216,9 +216,11 @@ int reuseport_add_sock(struct sock *sk, struct sock *sk2, bool bind_inany)
> > > > > > }
> > > > > > EXPORT_SYMBOL(reuseport_add_sock);
> > > > > >
> > > > > > -void reuseport_detach_sock(struct sock *sk)
> > > > > > +struct sock *reuseport_detach_sock(struct sock *sk)
> > > > > > {
> > > > > > struct sock_reuseport *reuse;
> > > > > > + struct bpf_prog *prog;
> > > > > > + struct sock *nsk = NULL;
> > > > > > int i;
> > > > > >
> > > > > > spin_lock_bh(&reuseport_lock);
> > > > > > @@ -242,8 +244,12 @@ void reuseport_detach_sock(struct sock *sk)
> > > > > >
> > > > > > reuse->num_socks--;
> > > > > > reuse->socks[i] = reuse->socks[reuse->num_socks];
> > > > > > + prog = rcu_dereference(reuse->prog);
> > > > > Is it under rcu_read_lock() here?
> > > >
> > > > reuseport_lock is locked in this function, and we do not modify the prog,
> > > > but is rcu_dereference_protected() preferable?
> > > >
> > > > ---8<---
> > > > prog = rcu_dereference_protected(reuse->prog,
> > > > lockdep_is_held(&reuseport_lock));
> > > > ---8<---
> > > It is not only reuse->prog. Other things also require rcu_read_lock(),
> > > e.g. please take a look at __htab_map_lookup_elem().
> > >
> > > The TCP_LISTEN sk (selected by bpf to be the target of the migration)
> > > is also protected by rcu.
> >
> > Thank you, I will use rcu_read_lock() and rcu_dereference() in v3 patchset.
> >
> >
> > > I am surprised there is no WARNING in the test.
> > > Do you have the needed DEBUG_LOCK* config enabled?
> >
> > Yes, DEBUG_LOCK* was 'y', but rcu_dereference() without rcu_read_lock()
> > does not show warnings...
> I would at least expect the "WARN_ON_ONCE(!rcu_read_lock_held() ...)"
> from __htab_map_lookup_elem() should fire in your test
> example in the last patch.
>
> It is better to check the config before sending v3.
It seems ok, but I will check it again.
---8<---
[ec2-user@...10-0-0-124 bpf-next]$ cat .config | grep DEBUG_LOCK
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_DEBUG_LOCKDEP=y
CONFIG_DEBUG_LOCKING_API_SELFTESTS=y
---8<---
> > > > > > diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> > > > > > index 1451aa9712b0..b27241ea96bd 100644
> > > > > > --- a/net/ipv4/inet_connection_sock.c
> > > > > > +++ b/net/ipv4/inet_connection_sock.c
> > > > > > @@ -992,6 +992,36 @@ struct sock *inet_csk_reqsk_queue_add(struct sock *sk,
> > > > > > }
> > > > > > EXPORT_SYMBOL(inet_csk_reqsk_queue_add);
> > > > > >
> > > > > > +void inet_csk_reqsk_queue_migrate(struct sock *sk, struct sock *nsk)
> > > > > > +{
> > > > > > + struct request_sock_queue *old_accept_queue, *new_accept_queue;
> > > > > > +
> > > > > > + old_accept_queue = &inet_csk(sk)->icsk_accept_queue;
> > > > > > + new_accept_queue = &inet_csk(nsk)->icsk_accept_queue;
> > > > > > +
> > > > > > + spin_lock(&old_accept_queue->rskq_lock);
> > > > > > + spin_lock(&new_accept_queue->rskq_lock);
> > > > > I am also not very thrilled on this double spin_lock.
> > > > > Can this be done in (or like) inet_csk_listen_stop() instead?
> > > >
> > > > It will be possible to migrate sockets in inet_csk_listen_stop(), but I
> > > > think it is better to do it just after reuseport_detach_sock() becuase we
> > > > can select a different listener (almost) every time at a lower cost by
> > > > selecting the moved socket and pass it to inet_csk_reqsk_queue_migrate()
> > > > easily.
> > > I don't see the "lower cost" point. Please elaborate.
> >
> > In reuseport_select_sock(), we pass sk_hash of the request socket to
> > reciprocal_scale() and generate a random index for socks[] to select
> > a different listener every time.
> > On the other hand, we do not have request sockets in unhash path and
> > sk_hash of the listener is always 0, so we have to generate a random number
> > in another way. In reuseport_detach_sock(), we can use the index of the
> > moved socket, but we do not have it in inet_csk_listen_stop(), so we have
> > to generate a random number in inet_csk_listen_stop().
> > I think it is at lower cost to use the index of the moved socket.
> Generate a random number is not a big deal for the migration code path.
>
> Also, I really still failed to see a particular way that the kernel
> pick will help in the migration case. The kernel has no clue
> on how to select the right process to migrate to without
> a proper policy signal from the user. They are all as bad as
> a random pick. I am not sure this migration feature is
> even useful if there is no bpf prog attached to define the policy.
I think most applications start new listeners before closing listeners, in
this case, selecting the moved socket as the new listener works well.
> That said, if it is still desired to do a random pick by kernel when
> there is no bpf prog, it probably makes sense to guard it in a sysctl as
> suggested in another reply. To keep it simple, I would also keep this
> kernel-pick consistent instead of request socket is doing something
> different from the unhash path.
Then, is this way better to keep kernel-pick consistent?
1. call reuseport_select_migrated_sock() without sk_hash from any path
2. generate a random number in reuseport_select_migrated_sock()
3. pass it to __reuseport_select_sock() only for select-by-hash
(4. pass 0 as sk_hash to bpf_run_sk_reuseport not to use it)
5. do migration per queue in inet_csk_listen_stop() or per request in
receive path.
I understand it is beautiful to keep consistensy, but also think
the kernel-pick with heuristic performs better than random-pick.
Powered by blists - more mailing lists