| Message ID | 20211015080142.43424-1-liujian56@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bpf, sockmap: Do not read sk_receive_queue in tcp_bpf_recvmsg if strparser enabled | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 17 of 17 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 56 this patch: 56 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/checkpatch | warning | WARNING: line length of 92 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 56 this patch: 56 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Fri, Oct 15, 2021 at 10:01 AM CEST, Liu Jian wrote: > If the strparser function of sk is turned on, all received data needs to > be processed by strparser first. > > Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") > Signed-off-by: Liu Jian <liujian56@huawei.com> > --- [...] > net/core/skmsg.c | 5 +++++ > net/ipv4/tcp_bpf.c | 9 ++++++--- > 3 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h > index 94e2a1f6e58d..25e92dff04aa 100644 > --- a/include/linux/skmsg.h > +++ b/include/linux/skmsg.h > @@ -390,6 +390,7 @@ void sk_psock_stop(struct sk_psock *psock, bool wait); > int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock); > void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock); > void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock); > +bool sk_psock_strparser_started(struct sock *sk); > #else > static inline int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock) > { > @@ -403,6 +404,11 @@ static inline void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) > static inline void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock) > { > } > + > +static inline bool sk_psock_strparser_started(struct sock *sk) > +{ > + return false; > +} > #endif > > void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock); > diff --git a/net/core/skmsg.c b/net/core/skmsg.c > index e85b7f8491b9..dd64ef854f3e 100644 > --- a/net/core/skmsg.c > +++ b/net/core/skmsg.c > @@ -1105,6 +1105,11 @@ void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) > sk->sk_write_space = sk_psock_write_space; > } > > +bool sk_psock_strparser_started(struct sock *sk) > +{ > + return sk->sk_data_ready == sk_psock_strp_data_ready; What if kTLS is configured on the socket? I think this check won't work then. > +} > + > void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock) > { > if (!psock->saved_data_ready) [...]
Jakub Sitnicki wrote: > On Fri, Oct 15, 2021 at 10:01 AM CEST, Liu Jian wrote: > > If the strparser function of sk is turned on, all received data needs to > > be processed by strparser first. > > > > Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > --- > > [...] > > > net/core/skmsg.c | 5 +++++ > > net/ipv4/tcp_bpf.c | 9 ++++++--- > > 3 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h > > index 94e2a1f6e58d..25e92dff04aa 100644 > > --- a/include/linux/skmsg.h > > +++ b/include/linux/skmsg.h > > @@ -390,6 +390,7 @@ void sk_psock_stop(struct sk_psock *psock, bool wait); > > int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock); > > void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock); > > void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock); > > +bool sk_psock_strparser_started(struct sock *sk); > > #else > > static inline int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock) > > { > > @@ -403,6 +404,11 @@ static inline void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) > > static inline void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock) > > { > > } > > + > > +static inline bool sk_psock_strparser_started(struct sock *sk) > > +{ > > + return false; > > +} > > #endif > > > > void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock); > > diff --git a/net/core/skmsg.c b/net/core/skmsg.c > > index e85b7f8491b9..dd64ef854f3e 100644 > > --- a/net/core/skmsg.c > > +++ b/net/core/skmsg.c > > @@ -1105,6 +1105,11 @@ void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) > > sk->sk_write_space = sk_psock_write_space; > > } > > > > +bool sk_psock_strparser_started(struct sock *sk) > > +{ > > + return sk->sk_data_ready == sk_psock_strp_data_ready; > > What if kTLS is configured on the socket? I think this check won't work then. Liu, did you see this. I think its a bit cleaner, avoids the extra parser check in hotpath, and should solve the issue? https://patchwork.kernel.org/project/netdevbpf/patch/20211011191647.418704-3-john.fastabend@gmail.com/ I think it should also address Jakub's concern. Thanks, John > > > +} > > + > > void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock) > > { > > if (!psock->saved_data_ready) > > [...]
> -----Original Message----- > From: John Fastabend [mailto:john.fastabend@gmail.com] > Sent: Monday, October 18, 2021 9:54 PM > To: Jakub Sitnicki <jakub@cloudflare.com>; liujian (CE) > <liujian56@huawei.com> > Cc: john.fastabend@gmail.com; daniel@iogearbox.net; lmb@cloudflare.com; > edumazet@google.com; davem@davemloft.net; kuba@kernel.org; > yoshfuji@linux-ipv6.org; dsahern@kernel.org; ast@kernel.org; > andrii@kernel.org; kafai@fb.com; songliubraving@fb.com; yhs@fb.com; > kpsingh@kernel.org; netdev@vger.kernel.org; bpf@vger.kernel.org > Subject: Re: [PATCH] bpf, sockmap: Do not read sk_receive_queue in > tcp_bpf_recvmsg if strparser enabled > > Jakub Sitnicki wrote: > > On Fri, Oct 15, 2021 at 10:01 AM CEST, Liu Jian wrote: > > > If the strparser function of sk is turned on, all received data > > > needs to be processed by strparser first. > > > > > > Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg > > > interface") > > > Signed-off-by: Liu Jian <liujian56@huawei.com> > > > --- > > > > [...] > > > > > net/core/skmsg.c | 5 +++++ > > > net/ipv4/tcp_bpf.c | 9 ++++++--- > > > 3 files changed, 17 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h index > > > 94e2a1f6e58d..25e92dff04aa 100644 > > > --- a/include/linux/skmsg.h > > > +++ b/include/linux/skmsg.h > > > @@ -390,6 +390,7 @@ void sk_psock_stop(struct sk_psock *psock, bool > > > wait); int sk_psock_init_strp(struct sock *sk, struct sk_psock > > > *psock); void sk_psock_start_strp(struct sock *sk, struct sk_psock > > > *psock); void sk_psock_stop_strp(struct sock *sk, struct sk_psock > > > *psock); > > > +bool sk_psock_strparser_started(struct sock *sk); > > > #else > > > static inline int sk_psock_init_strp(struct sock *sk, struct > > > sk_psock *psock) { @@ -403,6 +404,11 @@ static inline void > > > sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) static > > > inline void sk_psock_stop_strp(struct sock *sk, struct sk_psock > > > *psock) { } > > > + > > > +static inline bool sk_psock_strparser_started(struct sock *sk) { > > > + return false; > > > +} > > > #endif > > > > > > void sk_psock_start_verdict(struct sock *sk, struct sk_psock > > > *psock); diff --git a/net/core/skmsg.c b/net/core/skmsg.c index > > > e85b7f8491b9..dd64ef854f3e 100644 > > > --- a/net/core/skmsg.c > > > +++ b/net/core/skmsg.c > > > @@ -1105,6 +1105,11 @@ void sk_psock_start_strp(struct sock *sk, > struct sk_psock *psock) > > > sk->sk_write_space = sk_psock_write_space; } > > > > > > +bool sk_psock_strparser_started(struct sock *sk) { > > > + return sk->sk_data_ready == sk_psock_strp_data_ready; > > > > What if kTLS is configured on the socket? I think this check won't work then. > > Liu, did you see this. I think its a bit cleaner, avoids the extra parser check in > hotpath, and should solve the issue? > > https://patchwork.kernel.org/project/netdevbpf/patch/20211011191647.418 > 704-3-john.fastabend@gmail.com/ > > I think it should also address Jakub's concern. > I am sorry, I did not see the patch before. I think it can solve my issue, please ignore my patch. Thanks. > Thanks, > John > > > > > > +} > > > + > > > void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock) { > > > if (!psock->saved_data_ready) > > > > [...] >
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h index 94e2a1f6e58d..25e92dff04aa 100644 --- a/include/linux/skmsg.h +++ b/include/linux/skmsg.h @@ -390,6 +390,7 @@ void sk_psock_stop(struct sk_psock *psock, bool wait); int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock); void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock); void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock); +bool sk_psock_strparser_started(struct sock *sk); #else static inline int sk_psock_init_strp(struct sock *sk, struct sk_psock *psock) { @@ -403,6 +404,11 @@ static inline void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) static inline void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock) { } + +static inline bool sk_psock_strparser_started(struct sock *sk) +{ + return false; +} #endif void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock); diff --git a/net/core/skmsg.c b/net/core/skmsg.c index e85b7f8491b9..dd64ef854f3e 100644 --- a/net/core/skmsg.c +++ b/net/core/skmsg.c @@ -1105,6 +1105,11 @@ void sk_psock_start_strp(struct sock *sk, struct sk_psock *psock) sk->sk_write_space = sk_psock_write_space; } +bool sk_psock_strparser_started(struct sock *sk) +{ + return sk->sk_data_ready == sk_psock_strp_data_ready; +} + void sk_psock_stop_strp(struct sock *sk, struct sk_psock *psock) { if (!psock->saved_data_ready) diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c index 9d068153c316..17129b343966 100644 --- a/net/ipv4/tcp_bpf.c +++ b/net/ipv4/tcp_bpf.c @@ -198,6 +198,7 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, if (unlikely(!psock)) return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len); if (!skb_queue_empty(&sk->sk_receive_queue) && + !sk_psock_strparser_started(sk) && sk_psock_queue_empty(psock)) { sk_psock_put(sk, psock); return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len); @@ -214,9 +215,11 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, if (data) { if (!sk_psock_queue_empty(psock)) goto msg_bytes_ready; - release_sock(sk); - sk_psock_put(sk, psock); - return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len); + if (!sk_psock_strparser_started(sk)) { + release_sock(sk); + sk_psock_put(sk, psock); + return tcp_recvmsg(sk, msg, len, nonblock, flags, addr_len); + } } copied = -EAGAIN; }
If the strparser function of sk is turned on, all received data needs to be processed by strparser first. Fixes: 604326b41a6fb ("bpf, sockmap: convert to generic sk_msg interface") Signed-off-by: Liu Jian <liujian56@huawei.com> --- include/linux/skmsg.h | 6 ++++++ net/core/skmsg.c | 5 +++++ net/ipv4/tcp_bpf.c | 9 ++++++--- 3 files changed, 17 insertions(+), 3 deletions(-)