RE: [PATCH] bpf, sockmap: Do not read sk_receive_queue in tcp_bpf_recvmsg if strparser enabled

["liujian (CE)" <liujian56@huawei.com>]

> -----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)
> >
> > [...]
>