netdev - Re: [PATCH v3 bpf-next 6/8] xdp: Add a flag for disabling napi_direct of xdp_return_frame in xdp_mem

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]

Date:   Mon, 23 Jul 2018 20:38:14 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
Cc:     Toshiaki Makita <toshiaki.makita1@...il.com>,
        netdev@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: [PATCH v3 bpf-next 6/8] xdp: Add a flag for disabling
 napi_direct of xdp_return_frame in xdp_mem_info

On Tue, 24 Jul 2018 11:43:11 +0900, Toshiaki Makita wrote:
> On 2018/07/24 10:22, Jakub Kicinski wrote:
> > On Mon, 23 Jul 2018 00:13:06 +0900, Toshiaki Makita wrote:  
> >> From: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
> >>
> >> We need some mechanism to disable napi_direct on calling
> >> xdp_return_frame_rx_napi() from some context.
> >> When veth gets support of XDP_REDIRECT, it will redirects packets which
> >> are redirected from other devices. On redirection veth will reuse
> >> xdp_mem_info of the redirection source device to make return_frame work.
> >> But in this case .ndo_xdp_xmit() called from veth redirection uses
> >> xdp_mem_info which is not guarded by NAPI, because the .ndo_xdp_xmit is
> >> not called directly from the rxq which owns the xdp_mem_info.
> >>
> >> This approach introduces a flag in xdp_mem_info to indicate that
> >> napi_direct should be disabled even when _rx_napi variant is used.
> >>
> >> Signed-off-by: Toshiaki Makita <makita.toshiaki@....ntt.co.jp>  
> > 
> > To be clear - you will modify flags of the original source device if it
> > ever redirected a frame to a software device like veth?  Seems a bit
> > heavy handed.  The xdp_return_frame_rx_napi() is only really used on
> > error paths, but still..  Also as you note the original NAPI can run
> > concurrently with your veth dest one, but also with NAPIs of other veth
> > devices, so the non-atomic xdp.rxq->mem.flags |= XDP_MEM_RF_NO_DIRECT;
> > makes me worried.  
> 
> xdp_mem_info is copied in xdp_frame in convert_to_xdp_frame() so the
> field is local to the frame. Changing flags affects only the frame.
> xdp.rxq is local to NAPI thread, so no worries about atomicity.

Ah, right!  mem_info used to be just 8B, now it would be 12B.
Alternatively we could perhaps add this info to struct redirect_info,
through xdp_do_redirect() to avoid the per-frame cost.  I'm not sure
that's better.

> > Would you mind elaborating why not handle the RX completely in the NAPI
> > context of the original device?  
> 
> Originally it was difficult to implement .ndo_xdp_xmit() and
> .ndo_xdp_flush() model without creating NAPI in veth. Now it is changed
> so I'm not sure how difficult it is at this point.
> But in any case I want to avoid stack inflation by veth NAPI. (Imagine
> some misconfiguration like calling XDP_TX on both side of veth...)

True :/