netdev - Re: [PATCH RFC v4 bpf-next 09/11] tun: Support xdp in the Tx path for xdp

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:   Tue, 3 Mar 2020 10:08:12 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     David Ahern <dahern@...italocean.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, kuba@...nel.org,
        prashantbhole.linux@...il.com, jasowang@...hat.com,
        toke@...hat.com, mst@...hat.com, toshiaki.makita1@...il.com,
        daniel@...earbox.net, john.fastabend@...il.com, ast@...nel.org,
        kafai@...com, songliubraving@...com, yhs@...com, andriin@...com,
        dsahern@...il.com, brouer@...hat.com
Subject: Re: [PATCH RFC v4 bpf-next 09/11] tun: Support xdp in the Tx path
 for xdp_frames

On Mon, 2 Mar 2020 21:27:08 -0700
David Ahern <dahern@...italocean.com> wrote:

> On 3/2/20 11:30 AM, Alexei Starovoitov wrote:
> > On Wed, Feb 26, 2020 at 08:20:11PM -0700, David Ahern wrote:  
> >> +
> >> +		act = bpf_prog_run_xdp(xdp_prog, &xdp);
> >> +		switch (act) {
> >> +		case XDP_TX:    /* for Tx path, XDP_TX == XDP_PASS */
> >> +			act = XDP_PASS;
> >> +			break;
> >> +		case XDP_PASS:
> >> +			break;
> >> +		case XDP_REDIRECT:
> >> +			/* fall through */
> >> +		default:
> >> +			bpf_warn_invalid_xdp_action(act);
> >> +			/* fall through */
> >> +		case XDP_ABORTED:
> >> +			trace_xdp_exception(tun->dev, xdp_prog, act);
> >> +			/* fall through */
> >> +		case XDP_DROP:
> >> +			break;
> >> +		}  
> > 
> > patch 8 has very similar switch. Can you share the code?  
> 
> Most likely; I'll take a look.
> 
> > 
> > I'm worried that XDP_TX is a silent alias to XDP_PASS.
> > What were the reasons to go with this approach?  
> 
> As I stated in the cover letter:
> 
> "XDP_TX on Rx means send the packet out the device it arrived
> on; given that, XDP_Tx for the Tx path is treated as equivalent to
> XDP_PASS - ie., continue on the Tx path."

I'm not really buying this.  IHMO XDP_PASS should mean continue on the
Tx path, as this is a tx/egress XDP hook.  I don't see a reason to
basically "remove" the XDP_TX return code at this point.

> > imo it's less error prone and extensible to warn on XDP_TX.
> > Which will mean that both XDP_TX and XDP_REDICT are not supported
> > for egress atm.  

I agree.

I more see "XDP_TX" as a mirror facility... maybe there is a use-case
for bouncing packets back in the TX/Egress hook? That is future work,
but not reason disable the option now.


> I personally don't care either way; I was going with the simplest
> concept from a user perspective.
> 
> > 
> > Patches 8 and 9 cover tun only. I'd like to see egress hook to be
> > implemented in at least one physical NIC. Pick any hw. Something
> > that handles real frames. Adding this hook to virtual NIC is easy,
> > but it doesn't demonstrate design trade-offs one would need to
> > think through by adding egress hook to physical nic. That's why I
> > think it's mandatory to have it as part of the patch set.
> > 
> > Patch 11 exposes egress to samples/bpf. It's nice, but without
> > selftests it's no go. All new features must be exercised as part of
> > selftests/bpf.  
> 
> Patches that exercise the rtnetlink uapi are fairly easy to do on
> single node; anything traffic related requires multiple nodes or
> namespace level capabilities.  Unless I am missing something that is
> why all current XDP tests ride on top of veth; veth changes are not
> part of this set.
> 
> So to be clear you are saying that all new XDP features require
> patches to a h/w nic, veth and whatever the author really cares about
> before new features like this go in?

I would say yes. XDP is founded for physical HW NICs, and we need to
show it makes sense for physical HW NICs.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer