On Mon, Nov 16, 2020 at 11:29 PM CET, John Fastabend wrote:
> If the skb_verdict_prog redirects an skb knowingly to itself, fix your
> BPF program this is not optimal and an abuse of the API please use
> SK_PASS. That said there may be cases, such as socket load balancing,
> where picking the socket is hashed based or otherwise picks the same
> socket it was received on in some rare cases. If this happens we don't
> want to confuse userspace giving them an EAGAIN error if we can avoid
> it.
>
> To avoid double accounting in these cases. At the moment even if the
> skb has already been charged against the sockets rcvbuf and forward
> alloc we check it again and do set_owner_r() causing it to be orphaned
> and recharged. For one this is useless work, but more importantly we
> can have a case where the skb could be put on the ingress queue, but
> because we are under memory pressure we return EAGAIN. The trouble
> here is the skb has already been accounted for so any rcvbuf checks
> include the memory associated with the packet already. This rolls
> up and can result in unecessary EAGAIN errors in userspace read()
> calls.
>
> Fix by doing an unlikely check and skipping checks if skb->sk == sk.
>
> Fixes: 51199405f9672 ("bpf: skb_verdict, support SK_PASS on RX BPF path")
> Signed-off-by: John Fastabend <john.fastab...@gmail.com>
> ---
>  net/core/skmsg.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 9aed5a2c7c5b..514bc9f6f8ae 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -442,11 +442,19 @@ static int sk_psock_skb_ingress_enqueue(struct sk_buff 
> *skb,
>       return copied;
>  }
>  
> +static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff 
> *skb);
> +
>  static int sk_psock_skb_ingress(struct sk_psock *psock, struct sk_buff *skb)
>  {
>       struct sock *sk = psock->sk;
>       struct sk_msg *msg;
>  
> +     /* If we are receiving on the same sock skb->sk is already assigned,
> +      * skip memory accounting and owner transition seeing it already set
> +      * correctly.
> +      */
> +     if (unlikely(skb->sk == sk))
> +             return sk_psock_skb_ingress_self(psock, skb);
>       msg = sk_psock_create_ingress_msg(sk, skb);
>       if (!msg)
>               return -EAGAIN;

Reviewed-by: Jakub Sitnicki <ja...@cloudflare.com>

Reply via email to