On Thu, Jul 09, 2020 at 11:45:51AM +0200, Magnus Karlsson wrote:
> In the skb Tx path, transmission of a packet is performed with
> dev_direct_xmit(). When QUEUE_STATE_FROZEN is set in the transmit
> routines, it returns NETDEV_TX_BUSY signifying that it was not
> possible to send the packet now, please try later. Unfortunately, the
> xsk transmit code discarded the packet and returned EBUSY to the
> application. Fix this unnecessary packet loss, by not discarding the
> packet and return EAGAIN. As EAGAIN is returned to the application, it
> can then retry the send operation and the packet will finally be sent
> as we will likely not be in the QUEUE_STATE_FROZEN state anymore. So
> EAGAIN tells the application that the packet was not discarded from
> the Tx ring and that it needs to call send() again. EBUSY, on the
> other hand, signifies that the packet was not sent and discarded from
> the Tx ring. The application needs to put the packet on the Tx ring
> again if it wants it to be sent.
Doesn't the original code leak the skb if NETDEV_TX_BUSY is returned?
I'm not seeing where it was released. The new code looks correct.
> Fixes: 35fcde7f8deb ("xsk: support for Tx")
> Signed-off-by: Magnus Karlsson <magnus.karls...@intel.com>
> Reported-by: Arkadiusz Zema <a.z...@falconvsystems.com>
> Suggested-by: Arkadiusz Zema <a.z...@falconvsystems.com>
> ---
> net/xdp/xsk.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 3700266..5304250 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -376,13 +376,22 @@ static int xsk_generic_xmit(struct sock *sk)
> skb->destructor = xsk_destruct_skb;
>
> err = dev_direct_xmit(skb, xs->queue_id);
> - xskq_cons_release(xs->tx);
> /* Ignore NET_XMIT_CN as packet might have been sent */
> - if (err == NET_XMIT_DROP || err == NETDEV_TX_BUSY) {
> + if (err == NET_XMIT_DROP) {
> /* SKB completed but not sent */
> + xskq_cons_release(xs->tx);
> err = -EBUSY;
> goto out;
> + } else if (err == NETDEV_TX_BUSY) {
Should be "if (err == ..." here, no else.
> + /* QUEUE_STATE_FROZEN, tell application to
> + * retry sending the packet
> + */
> + skb->destructor = NULL;
> + kfree_skb(skb);
> + err = -EAGAIN;
> + goto out;
> }
> + xskq_cons_release(xs->tx);
>
> sent_frame = true;
> }
> --
> 2.7.4
>
