netdev - Re: [PATCH bpf] xsk: do not discard packet when QUEUE_STATE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 9 Jul 2020 21:39:42 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
Cc:     Magnus Karlsson <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        A.Zema@...convsystems.com
Subject: Re: [PATCH bpf] xsk: do not discard packet when QUEUE_STATE_FROZEN

On Thu, Jul 9, 2020 at 9:34 PM Jonathan Lemon <jonathan.lemon@...il.com> wrote:
>
> On Thu, Jul 09, 2020 at 09:30:42PM +0200, Magnus Karlsson wrote:
> > On Thu, Jul 9, 2020 at 7:10 PM Magnus Karlsson
> > <magnus.karlsson@...il.com> wrote:
> > >
> > > On Thu, Jul 9, 2020 at 7:06 PM Jonathan Lemon <jonathan.lemon@...il.com> wrote:
> > > >
> > > > 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.
> > >
> > > You are correct. Should also have mentioned that in the commit message.
> >
> > Jonathan,
> >
> > Some context here. The bug report from Arkadiusz started out with the
> > unnecessary packet loss. While fixing it, I discovered that it was
> > actually leaking memory too. If you want, I can send a v2 that has a
> > commit message that mentions both problems? Let me know what you
> > prefer.
>
> I think it would be best to mention both problems for the benefit of
> future readers.

You will get a v2 tomorrow.

/Magnus

> --
> Jonathan