| Message ID | 20210719214834.125484-2-john.fastabend@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | sockmap fixes picked up by stress tests | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 9 maintainers not CCed: yhs@fb.com kpsingh@kernel.org andrii@kernel.org kafai@fb.com ast@kernel.org lmb@cloudflare.com songliubraving@fb.com davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Mon, Jul 19, 2021 at 11:48 PM CEST, John Fastabend wrote: > We don't want strparser to run and pass skbs into skmsg handlers when > the psock is null. We just sk_drop them in this case. When removing > a live socket from map it means extra drops that we do not need to > incur. Move the zap below strparser close to avoid this condition. > > This way we stop the stream parser first stopping it from processing > packets and then delete the psock. > > Fixes: a136678c0bdbb ("bpf: sk_msg, zap ingress queue on psock down") > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > --- To confirm my understanding - the extra drops can happen because currently we are racing to clear SK_PSOCK_TX_ENABLED flag in sk_psock_drop with sk_psock_verdict_apply, which checks the flag before pushing skb onto psock->ingress_skb queue (or possibly straight into psock->ingress_msg queue on no redirect).
Jakub Sitnicki wrote: > On Mon, Jul 19, 2021 at 11:48 PM CEST, John Fastabend wrote: > > We don't want strparser to run and pass skbs into skmsg handlers when > > the psock is null. We just sk_drop them in this case. When removing > > a live socket from map it means extra drops that we do not need to > > incur. Move the zap below strparser close to avoid this condition. > > > > This way we stop the stream parser first stopping it from processing > > packets and then delete the psock. > > > > Fixes: a136678c0bdbb ("bpf: sk_msg, zap ingress queue on psock down") > > Signed-off-by: John Fastabend <john.fastabend@gmail.com> > > --- > > To confirm my understanding - the extra drops can happen because > currently we are racing to clear SK_PSOCK_TX_ENABLED flag in > sk_psock_drop with sk_psock_verdict_apply, which checks the flag before > pushing skb onto psock->ingress_skb queue (or possibly straight into > psock->ingress_msg queue on no redirect). Correct. If strparser hands a skb to the sk_psock_* handlers then before they enqueue the packet the flag is checked and the packet will be dropped in this case. I noticed this while testing. So rather than letting packets get into sk_psock_* handlers this patch just stops the strparser.
diff --git a/net/core/skmsg.c b/net/core/skmsg.c index 15d71288e741..28115ef742e8 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -773,8 +773,6 @@ static void sk_psock_destroy(struct work_struct *work) void sk_psock_drop(struct sock *sk, struct sk_psock *psock) { - sk_psock_stop(psock, false); - write_lock_bh(&sk->sk_callback_lock); sk_psock_restore_proto(sk, psock); rcu_assign_sk_user_data(sk, NULL); @@ -784,6 +782,8 @@ void sk_psock_drop(struct sock *sk, struct sk_psock *psock) sk_psock_stop_verdict(sk, psock); write_unlock_bh(&sk->sk_callback_lock); + sk_psock_stop(psock, false); + INIT_RCU_WORK(&psock->rwork, sk_psock_destroy); queue_rcu_work(system_wq, &psock->rwork); }
We don't want strparser to run and pass skbs into skmsg handlers when the psock is null. We just sk_drop them in this case. When removing a live socket from map it means extra drops that we do not need to incur. Move the zap below strparser close to avoid this condition. This way we stop the stream parser first stopping it from processing packets and then delete the psock. Fixes: a136678c0bdbb ("bpf: sk_msg, zap ingress queue on psock down") Signed-off-by: John Fastabend <john.fastabend@gmail.com> --- net/core/skmsg.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)