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:   Mon, 2 Mar 2020 10:30:41 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     David Ahern <dsahern@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        prashantbhole.linux@...il.com, jasowang@...hat.com,
        brouer@...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, David Ahern <dahern@...italocean.com>
Subject: Re: [PATCH RFC v4 bpf-next 09/11] tun: Support xdp in the Tx path
 for xdp_frames

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?

I'm worried that XDP_TX is a silent alias to XDP_PASS.
What were the reasons to go with this approach?
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.

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.

re: patch 3. I agree with Toke and Jesper that union in uapi is unnecessary.
Just drop it. xdp_md is a virtual data structure. It's not allocated anywhere.
There is no need to save non-existent memory.