On Wed, Jul 15, 2020 at 8:36 PM Daniel Borkmann <dan...@iogearbox.net> wrote:
>
> On 7/11/20 9:39 AM, Magnus Karlsson wrote:
> > On Sat, Jul 11, 2020 at 1:28 AM Daniel Borkmann <dan...@iogearbox.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.karls...@intel.com>
> >>> Reported-by: Arkadiusz Zema <a.z...@falconvsystems.com>
> >>> Suggested-by: Arkadiusz Zema <a.z...@falconvsystems.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. Will take a look at that and give it a try.
/Magnus
> Thanks,
> Daniel
