Re: [PATCH bpf 07/11] bpf: sockmap incorrectly handling copied_seq

[John Fastabend <john.fastabend@gmail.com>]

Eric Dumazet wrote:
> On Tue, Mar 21, 2023 at 2:52 PM John Fastabend <john.fastabend@gmail.com> wrote:
> >
> > The read_skb() logic is incrementing the tcp->copied_seq which is used for
> > among other things calculating how many outstanding bytes can be read by
> > the application. This results in application errors, if the application
> > does an ioctl(FIONREAD) we return zero because this is calculated from
> > the copied_seq value.
> >
> > To fix this we move tcp->copied_seq accounting into the recv handler so
> > that we update these when the recvmsg() hook is called and data is in
> > fact copied into user buffers. This gives an accurate FIONREAD value
> > as expected and improves ACK handling. Before we were calling the
> > tcp_rcv_space_adjust() which would update 'number of bytes copied to
> > user in last RTT' which is wrong for programs returning SK_PASS. The
> > bytes are only copied to the user when recvmsg is handled.
> >
> > Doing the fix for recvmsg is straightforward, but fixing redirect and
> > SK_DROP pkts is a bit tricker. Build a tcp_psock_eat() helper and then
> > call this from skmsg handlers. This fixes another issue where a broken
> > socket with a BPF program doing a resubmit could hang the receiver. This
> > happened because although read_skb() consumed the skb through sock_drop()
> > it did not update the copied_seq. Now if a single reccv socket is
> > redirecting to many sockets (for example for lb) the receiver sk will be
> > hung even though we might expect it to continue. The hang comes from
> > not updating the copied_seq numbers and memory pressure resulting from
> > that.
> >
> > We have a slight layer problem of calling tcp_eat_skb even if its not
> > a TCP socket. To fix we could refactor and create per type receiver
> > handlers. I decided this is more work than we want in the fix and we
> > already have some small tweaks depending on caller that use the
> > helper skb_bpf_strparser(). So we extend that a bit and always set
> > the strparser bit when it is in use and then we can gate the
> > seq_copied updates on this.
> >
> > Fixes: 04919bed948dc ("tcp: Introduce tcp_read_skb()")
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> >  include/net/tcp.h  |  3 +++
> >  net/core/skmsg.c   |  7 +++++--
> >  net/ipv4/tcp.c     | 10 +---------
> >  net/ipv4/tcp_bpf.c | 28 +++++++++++++++++++++++++++-
> >  4 files changed, 36 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > index db9f828e9d1e..674044b8bdaf 100644
> > --- a/include/net/tcp.h
> > +++ b/include/net/tcp.h
> > @@ -1467,6 +1467,8 @@ static inline void tcp_adjust_rcv_ssthresh(struct sock *sk)
> >  }
> >
> >  void tcp_cleanup_rbuf(struct sock *sk, int copied);
> > +void __tcp_cleanup_rbuf(struct sock *sk, int copied);
> > +
> >
> >  /* We provision sk_rcvbuf around 200% of sk_rcvlowat.
> >   * If 87.5 % (7/8) of the space has been consumed, we want to override
> > @@ -2321,6 +2323,7 @@ struct sk_psock;
> >  struct proto *tcp_bpf_get_proto(struct sock *sk, struct sk_psock *psock);
> >  int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore);
> >  void tcp_bpf_clone(const struct sock *sk, struct sock *newsk);
> > +void tcp_eat_skb(struct sock *sk, struct sk_buff *skb);
> >  #endif /* CONFIG_BPF_SYSCALL */
> >
> >  int tcp_bpf_sendmsg_redir(struct sock *sk, bool ingress,
> > diff --git a/net/core/skmsg.c b/net/core/skmsg.c

[...]

> >  EXPORT_SYMBOL(tcp_read_skb);
> > diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> > index b1ba58be0c5a..c0e5680dccc0 100644
> > --- a/net/ipv4/tcp_bpf.c
> > +++ b/net/ipv4/tcp_bpf.c
> > @@ -11,6 +11,24 @@
> >  #include <net/inet_common.h>
> >  #include <net/tls.h>
> >
> > +void tcp_eat_skb(struct sock *sk, struct sk_buff *skb)
> > +{
> > +       struct tcp_sock *tcp;
> > +       int copied;
> > +
> > +       if (!skb || !skb->len || !sk_is_tcp(sk))
> > +               return;
> > +
> > +       if (skb_bpf_strparser(skb))
> > +               return;
> > +
> > +       tcp = tcp_sk(sk);
> > +       copied = tcp->copied_seq + skb->len;
> > +       WRITE_ONCE(tcp->copied_seq, skb->len);
>
> It seems your tests are unable to catch this bug :/

Its because the tests are returning SK_PASS and this logic
is never called. I'll add a test that checks FIONREAD and
does SK_DROP.

Thanks.