Re: [PATCH v2 bpf-next 2/5] bpf: Add support to attach bpf program to a devmap entry

On Wed, May 27, 2020 at 5:17 PM David Ahern [off-list ref] wrote:

From: David Ahern <redacted>

Add BPF_XDP_DEVMAP attach type for use with programs associated with a
DEVMAP entry.

Allow DEVMAPs to associate a program with a device entry by adding
a bpf_prog_fd to 'struct devmap_val'. Values read show the program
id, so the fd and id are a union.

The program associated with the fd must have type XDP with expected
attach type BPF_XDP_DEVMAP. When a program is associated with a device
index, the program is run on an XDP_REDIRECT and before the buffer is
added to the per-cpu queue. At this point rxq data is still valid; the
next patch adds tx device information allowing the prorgam to see both
ingress and egress device indices.

XDP generic is skb based and XDP programs do not work with skb's. Block
the use case by walking maps used by a program that is to be attached
via xdpgeneric and fail if any of them are DEVMAP / DEVMAP_HASH with
 > 4-byte values.

Block attach of BPF_XDP_DEVMAP programs to devices.

Signed-off-by: David Ahern <redacted>
---

Please cc bpf@vger.kernel.org in the future for patches related to BPF
in general.

 include/linux/bpf.h            |  5 +++
 include/uapi/linux/bpf.h       |  5 +++
 kernel/bpf/devmap.c            | 79 +++++++++++++++++++++++++++++++++-
 net/core/dev.c                 | 18 ++++++++
 tools/include/uapi/linux/bpf.h |  5 +++
 5 files changed, 110 insertions(+), 2 deletions(-)

[...]

+static struct xdp_buff *dev_map_run_prog(struct net_device *dev,
+                                        struct xdp_buff *xdp,
+                                        struct bpf_prog *xdp_prog)
+{
+       u32 act;
+
+       act = bpf_prog_run_xdp(xdp_prog, xdp);
+       switch (act) {
+       case XDP_DROP:
+               fallthrough;

nit: I don't think fallthrough is necessary for cases like:

case XDP_DROP:
case XDP_PASS:
    /* do something */

+       case XDP_PASS:
+               break;
+       default:
+               bpf_warn_invalid_xdp_action(act);
+               fallthrough;
+       case XDP_ABORTED:
+               trace_xdp_exception(dev, xdp_prog, act);
+               act = XDP_DROP;
+               break;
+       }
+
+       if (act == XDP_DROP) {
+               xdp_return_buff(xdp);
+               xdp = NULL;

hm.. if you move XDP_DROP case to after XDP_ABORTED and do fallthrough
from XDP_ABORTED, you won't even need to override act and it will just
handle all the cases, no?

switch (act) {
case XDP_PASS:
    return xdp;
default:
    bpf_warn_invalid_xdp_action(act);
    fallthrough;
case XDP_ABORTED:
    trace_xdp_exception(dev, xdp_prog, act);
    fallthrough;
case XDP_DROP:
    xdp_return_buff(xdp);
    return NULL;
}

Wouldn't this be simpler?

+       }
+
+       return xdp;
+}
+

[...]