Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210305092534.13121-1-baijiaju1990@gmail.com>
In-Reply-To: <20210305092534.13121-1-baijiaju1990@gmail.com>
From:   Magnus Karlsson <magnus.karlsson@gmail.com>
Date:   Fri, 5 Mar 2021 13:07:47 +0100
Message-ID: <CAJ8uoz3dkGsnMQ5wnFmyyFVfkMrz8Z2pqPZ+frFXj=Sy72xpcw@mail.gmail.com>
Subject: Re: [PATCH] net: xdp: fix error return code of xsk_generic_xmit()
To:     Jia-Ju Bai <baijiaju1990@gmail.com>
Cc:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@intel.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        KP Singh <kpsingh@kernel.org>,
        Network Development <netdev@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Mar 5, 2021 at 10:28 AM Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>
> When err is zero but xskq_prod_reserve() fails, no error return code of
> xsk_generic_xmit() is assigned.
> To fix this bug, err is assigned with the return value of
> xskq_prod_reserve(), and then err is checked.

This error is ignored by design. This so that the zero-copy path and
the copy/skb path will return the same value (success in this case)
when the completion ring does not have a spare entry we can put the
future completion in. The problem lies with the zero-copy path that is
asynchronous, in contrast to the skb path that is synchronous. The
zero-copy path cannot return an error when this happens as this
reservation in the completion ring is performed by the driver that
might concurrently run on another core without any way to feed this
back to the syscall that does not wait for the driver to execute in
any case. Introducing a return value for this condition right now for
the skb case, might break existing applications.

Though it would be really good if you could submit a small patch to
bpf-next that adds a comment explaining this to avoid any future
confusion. Something along the lines of: /* The error code of
xskq_prod_reserve is ignored so that skb mode will mimic the same
behavior as zero-copy mode that does not signal an error in this case
as it cannot. */. You could put it right after the if statement.

Thank you: Magnus

> The spinlock is only used to protect the call to xskq_prod_reserve().
>
> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>  net/xdp/xsk.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 4faabd1ecfd1..f1c1db07dd07 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -484,8 +484,14 @@ static int xsk_generic_xmit(struct sock *sk)
>                  * if there is space in it. This avoids having to implement
>                  * any buffering in the Tx path.
>                  */
> +               if (unlikely(err)) {
> +                       kfree_skb(skb);
> +                       goto out;
> +               }
> +
>                 spin_lock_irqsave(&xs->pool->cq_lock, flags);
> -               if (unlikely(err) || xskq_prod_reserve(xs->pool->cq)) {
> +               err = xskq_prod_reserve(xs->pool->cq);
> +               if (unlikely(err)) {
>                         spin_unlock_irqrestore(&xs->pool->cq_lock, flags);
>                         kfree_skb(skb);
>                         goto out;
> --
> 2.17.1
>