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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJ8uoz2_m+-s4UXuChu9Edk99BS7NK=0cRFGFB4+z9KsHiDTMg@mail.gmail.com>
Date:   Thu, 9 Jul 2020 19:10:13 +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 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.

/Magnus

>
> > Fixes: 35fcde7f8deb ("xsk: support for Tx")
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> > Reported-by: Arkadiusz Zema <A.Zema@...convsystems.com>
> > Suggested-by: Arkadiusz Zema <A.Zema@...convsystems.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
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ