netdev - Re: [PATCH bpf v2] xsk: fix memory leak and packet loss in Tx skb path

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: <a6bee4f6-10a9-abbd-1b90-bf4a7c82dacc@iogearbox.net>
Date:   Wed, 15 Jul 2020 20:36:11 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Magnus Karlsson <magnus.karlsson@...il.com>
Cc:     Magnus Karlsson <magnus.karlsson@...el.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        A.Zema@...convsystems.com
Subject: Re: [PATCH bpf v2] xsk: fix memory leak and packet loss in Tx skb
 path

On 7/11/20 9:39 AM, Magnus Karlsson wrote:
> On Sat, Jul 11, 2020 at 1:28 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>> On 7/10/20 8:45 AM, 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, missed to free the skb, and
>>> returned EBUSY to the application. Fix this memory leak and
>>> unnecessary packet loss, by not discarding the packet in the Tx ring,
>>> freeing the allocated skb, 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.
>>>
>>> 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>
>>> ---
>>> The v1 of this patch was called "xsk: do not discard packet when
>>> QUEUE_STATE_FROZEN".
>>> ---
>>>    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) {
>>> +                     /* QUEUE_STATE_FROZEN, tell application to
>>> +                      * retry sending the packet
>>> +                      */
>>> +                     skb->destructor = NULL;
>>> +                     kfree_skb(skb);
>>> +                     err = -EAGAIN;
>>> +                     goto out;
>>
>> Hmm, I'm probably missing something or I should blame my current lack of coffee,
>> but I'll ask anyway.. What is the relation here to the kfree_skb{,_list}() in
>> dev_direct_xmit() when we have NETDEV_TX_BUSY condition? Wouldn't the patch above
>> double-free with NETDEV_TX_BUSY?
> 
> I think you are correct even without coffee :-). I misinterpreted the
> following piece of code in dev_direct_xmit():
> 
> if (!dev_xmit_complete(ret))
>       kfree_skb(skb);
> 
> If the skb was NOT consumed by the transmit, then it goes and frees
> the skb. NETDEV_TX_BUSY as a return value will make
> dev_xmit_complete() return false which triggers the freeing of the
> skb. So if I now understand dev_direct_xmit() correctly, it will
> always consume the skb, even when NETDEV_TX_BUSY is returned. And this
> is what I would like to avoid. If the skb is freed, the destructor is
> triggered and it will complete the packet to user-space, which is the
> same thing as dropping it, which is what I want to avoid in the first
> place since it is completely unnecessary.
> 
> So what would be the best way to solve this? Prefer to share the code
> with AF_PACKET if possible. Introduce a boolean function parameter to
> indicate if it should be freed in this case? Other ideas? Here are the
> users of dev_direct_xmit():

Another option could be looking at pktgen which mangles skb->users to keep
the skb alive.

Thanks,
Daniel