Skip to contentSkip to conversation

bpf: Add support for XDP programs in DEVMAP entries

PATCH Superseded David Ahern 5 participants 18 messages May 28, 2020

Subsystems (9)
Networking Network drivers Kselftest BPF +5 more subsystems

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

David Ahern

author
#0 lore raw

v2 cover bpf: Add support for XDP programs in DEVMAP entries

Implementation of Daniel's proposal for allowing DEVMAP entries to be a device index, program fd pair.

Programs are run after XDP_REDIRECT and have access to both Rx device and Tx device.

v2
- moved dev_map_ext_val definition to uapi to formalize the API for devmap extensions; add bpf_ prefix to the prog_fd and prog_id entries
- changed devmap code to handle struct in a way that it can support future extensions
- fixed subject in libbpf patch

v1
- fixed prog put on invalid program - Toke
- changed write value from id to fd per Toke's comments about capabilities
- add test cases

David Ahern (5):
devmap: Formalize map value as a named struct
bpf: Add support to attach bpf program to a devmap entry
xdp: Add xdp_txq_info to xdp_buff
libbpf: Add SEC name for xdp programs attached to device map
selftest: Add tests for XDP programs in devmap entries

 include/linux/bpf.h                           |   5 +
 include/net/xdp.h                             |   5 +
 include/uapi/linux/bpf.h                      |  12 ++
 kernel/bpf/devmap.c                           | 119 +++++++++++++++---
 net/core/dev.c                                |  18 +++
 net/core/filter.c                             |  17 +++
 tools/include/uapi/linux/bpf.h                |  12 ++
 tools/lib/bpf/libbpf.c                        |   2 +
 .../bpf/prog_tests/xdp_devmap_attach.c        |  94 ++++++++++++++
 .../selftests/bpf/progs/test_xdp_devmap.c     |  19 +++
 .../selftests/bpf/progs/test_xdp_devmap2.c    |  19 +++
 .../bpf/progs/test_xdp_with_devmap.c          |  17 +++
 12 files changed, 322 insertions(+), 17 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap2.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c
Signature · 1 line
2.21.1 (Apple Git-122.3)
author
#1 lore raw

PATCH v2 1/5 devmap: Formalize map value as a named struct

From: David Ahern <[email protected]>

Add 'struct devmap_val' to the bpf uapi to formalize the expected values that can be passed in for a DEVMAP. Update devmap code to use the struct.

Signed-off-by David Ahern <[email protected]>
---
 include/uapi/linux/bpf.h       |  5 ++++
 kernel/bpf/devmap.c            | 43 ++++++++++++++++++++--------------
 tools/include/uapi/linux/bpf.h |  5 ++++
 3 files changed, 35 insertions(+), 18 deletions(-)
include/uapi/linux/bpf.h +5 -0
index 54b93f8b49b8..d27302ecaa9c 100644
@@ -3625,6 +3625,11 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 };
 
+/* DEVMAP values */
+struct devmap_val {
+	__u32 ifindex;   /* device index */
+};
+
 enum sk_action {
 	SK_DROP = 0,
 	SK_PASS,
kernel/bpf/devmap.c +25 -18

115 lines not on this page · raw message

tools/include/uapi/linux/bpf.h +5 -0
index 54b93f8b49b8..d27302ecaa9c 100644
@@ -3625,6 +3625,11 @@ struct xdp_md {
 	__u32 rx_queue_index;  /* rxq->queue_index  */
 };
 
+/* DEVMAP values */
+struct devmap_val {
+	__u32 ifindex;   /* device index */
+};
+
 enum sk_action {
 	SK_DROP = 0,
 	SK_PASS,
Signature · 1 line
2.21.1 (Apple Git-122.3)
#2 lore raw

Re: [PATCH v2 bpf-next 1/5] devmap: Formalize map value as a named struct

Quoted 19 lines · David Ahern

From: David Ahern <[email protected]>

Add 'struct devmap_val' to the bpf uapi to formalize the expected values that can be passed in for a DEVMAP. Update devmap code to use the struct.

Signed-off-by David Ahern <[email protected]>
---
 include/uapi/linux/bpf.h       |  5 ++++
 kernel/bpf/devmap.c            | 43 ++++++++++++++++++++--------------
 tools/include/uapi/linux/bpf.h |  5 ++++
 3 files changed, 35 insertions(+), 18 deletions(-)
include/uapi/linux/bpf.h +0 -0
index 54b93f8b49b8..d27302ecaa9c 100644

include/uapi/linux/bpf.h struct xdp_md { · line 3632

Quoted 8 lines
include/uapi/linux/bpf.h +5 -0
@@ -3625,6 +3625,11 @@ struct xdp_md {
        __u32 rx_queue_index;  /* rxq->queue_index  */
 };
 
+/* DEVMAP values */
+struct devmap_val {
+       __u32 ifindex;   /* device index */
+};
+

can DEVMAP be used outside of BPF ecosystem? If not, shouldn't this be struct bpf_devmap_val, to be consistent with the rest of the type names?

Quoted 16 lines
include/uapi/linux/bpf.h +0 -0
 enum sk_action {
        SK_DROP = 0,
        SK_PASS,
kernel/bpf/devmap.c +1 -0
index a51d9fb7a359..069a50113e26 100644
@@ -66,6 +66,7 @@ struct bpf_dtab_netdev {
        struct bpf_dtab *dtab;
        struct rcu_head rcu;
        unsigned int idx;
+       struct devmap_val val;
 };
 
 struct bpf_dtab {

kernel/bpf/devmap.c static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr) · line 114

Quoted 5 lines
kernel/bpf/devmap.c +1 -1
@@ -110,7 +111,8 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
 
        /* check sanity of attributes */
        if (attr->max_entries == 0 || attr->key_size != 4 ||
-           attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
+           attr->value_size > sizeof(struct devmap_val) ||

So is 0, 1, 2, 3, and after next patch 5, 6, and 7 all allowed as well? Isn't that a bit too permissive?

Quoted 3 lines
kernel/bpf/devmap.c +1 -0
+           attr->map_flags & ~DEV_CREATE_FLAG_MASK)
                return -EINVAL;

[...]

#3 lore raw

Re: [PATCH v2 bpf-next 1/5] devmap: Formalize map value as a named struct

Quoted 23 lines · Andrii Nakryiko
Quoted 22 lines · David Ahern

From: David Ahern <[email protected]>

Add 'struct devmap_val' to the bpf uapi to formalize the expected values that can be passed in for a DEVMAP. Update devmap code to use the struct.

Signed-off-by David Ahern <[email protected]>
---
 include/uapi/linux/bpf.h       |  5 ++++
 kernel/bpf/devmap.c            | 43 ++++++++++++++++++++--------------
 tools/include/uapi/linux/bpf.h |  5 ++++
 3 files changed, 35 insertions(+), 18 deletions(-)
include/uapi/linux/bpf.h +0 -0
index 54b93f8b49b8..d27302ecaa9c 100644
@@ -3625,6 +3625,11 @@ struct xdp_md {
        __u32 rx_queue_index;  /* rxq->queue_index  */
 };
Quoted 9 lines

include/uapi/linux/bpf.h struct xdp_md { · line 3632

Quoted 5 lines
include/uapi/linux/bpf.h +5 -0
+/* DEVMAP values */
+struct devmap_val {
+       __u32 ifindex;   /* device index */
+};
+

can DEVMAP be used outside of BPF ecosystem? If not, shouldn't this be struct bpf_devmap_val, to be consistent with the rest of the type names?

sure, added 'bpf_' to the name.

Quoted 18 lines
Quoted 18 lines
include/uapi/linux/bpf.h +0 -0
 enum sk_action {
        SK_DROP = 0,
        SK_PASS,
kernel/bpf/devmap.c +1 -0
index a51d9fb7a359..069a50113e26 100644
@@ -66,6 +66,7 @@ struct bpf_dtab_netdev {
        struct bpf_dtab *dtab;
        struct rcu_head rcu;
        unsigned int idx;
+       struct devmap_val val;
 };
 
 struct bpf_dtab {

@@ -110,7 +111,8 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
Quoted 7 lines

kernel/bpf/devmap.c static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr) · line 114

Quoted 4 lines
kernel/bpf/devmap.c +1 -1
        /* check sanity of attributes */
        if (attr->max_entries == 0 || attr->key_size != 4 ||
-           attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
+           attr->value_size > sizeof(struct devmap_val) ||

So is 0, 1, 2, 3, and after next patch 5, 6, and 7 all allowed as well? Isn't that a bit too permissive?

sure, I should check that it is at least 4-bytes - the existing size of the values. After that the struct can vary as user and kernel differ. The key is that newer userspace can not send down a higher value size than the kernel supports and older userspace can send fewer bytes (e.g., 4-byte ifindex only vs 8-byte ifindex + fd). I'll revert this to v1 where I check for specific known value sizes.

Quoted 7 lines
Quoted 4 lines
kernel/bpf/devmap.c +1 -0
+           attr->map_flags & ~DEV_CREATE_FLAG_MASK)
                return -EINVAL;

[...]

author
#4 lore raw

PATCH v2 2/5 bpf: Add support to attach bpf program to a devmap entry

In reply to David Ahern

From: David Ahern <[email protected]>

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

Quoted 1 line

4-byte values.

Block attach of BPF_XDP_DEVMAP programs to devices.

Signed-off-by David Ahern <[email protected]>
---
 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(-)
include/linux/bpf.h +5 -0
index efe8836b5c48..088751bc09aa 100644
@@ -1242,6 +1242,7 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
 		    struct net_device *dev_rx);
 int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
 			     struct bpf_prog *xdp_prog);
+bool dev_map_can_have_prog(struct bpf_map *map);
 
 struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key);
 void __cpu_map_flush(void);

@@ -1355,6 +1356,10 @@ static inline struct net_device  *__dev_map_hash_lookup_elem(struct bpf_map *map
 {
 	return NULL;
 }
+static inline bool dev_map_can_have_prog(struct bpf_map *map)
+{
+	return false;
+}
 
 static inline void __dev_flush(void)
 {
include/uapi/linux/bpf.h +5 -0
index d27302ecaa9c..2d9927b7a922 100644
@@ -224,6 +224,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_INET6_GETPEERNAME,
 	BPF_CGROUP_INET4_GETSOCKNAME,
 	BPF_CGROUP_INET6_GETSOCKNAME,
+	BPF_XDP_DEVMAP,
 	__MAX_BPF_ATTACH_TYPE
 };
 

@@ -3628,6 +3629,10 @@ struct xdp_md {
 /* DEVMAP values */
 struct devmap_val {
 	__u32 ifindex;   /* device index */
+	union {
+		int   bpf_prog_fd;  /* prog fd on map write */
+		__u32 bpf_prog_id;  /* prog id on map read */
+	};
 };
 
 enum sk_action {
kernel/bpf/devmap.c +77 -2

158 lines not on this page · raw message

net/core/dev.c +18 -0
index ae37586f6ee8..10684833f864 100644
@@ -5420,6 +5420,18 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
 	struct bpf_prog *new = xdp->prog;
 	int ret = 0;
 
+	if (new) {
+		u32 i;
+
+		/* generic XDP does not work with DEVMAPs that can
+		 * have a bpf_prog installed on an entry
+		 */
+		for (i = 0; i < new->aux->used_map_cnt; i++) {
+			if (dev_map_can_have_prog(new->aux->used_maps[i]))
+				return -EINVAL;
+		}
+	}
+
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
 		rcu_assign_pointer(dev->xdp_prog, new);

@@ -8835,6 +8847,12 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 			return -EINVAL;
 		}
 
+		if (prog->expected_attach_type == BPF_XDP_DEVMAP) {
+			NL_SET_ERR_MSG(extack, "BPF_XDP_DEVMAP programs can not be attached to a device");
+			bpf_prog_put(prog);
+			return -EINVAL;
+		}
+
 		/* prog->aux->id may be 0 for orphaned device-bound progs */
 		if (prog->aux->id && prog->aux->id == prog_id) {
 			bpf_prog_put(prog);
tools/include/uapi/linux/bpf.h +5 -0
index d27302ecaa9c..2d9927b7a922 100644
@@ -224,6 +224,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_INET6_GETPEERNAME,
 	BPF_CGROUP_INET4_GETSOCKNAME,
 	BPF_CGROUP_INET6_GETSOCKNAME,
+	BPF_XDP_DEVMAP,
 	__MAX_BPF_ATTACH_TYPE
 };
 

@@ -3628,6 +3629,10 @@ struct xdp_md {
 /* DEVMAP values */
 struct devmap_val {
 	__u32 ifindex;   /* device index */
+	union {
+		int   bpf_prog_fd;  /* prog fd on map write */
+		__u32 bpf_prog_id;  /* prog id on map read */
+	};
 };
 
 enum sk_action {
Signature · 1 line
2.21.1 (Apple Git-122.3)
#5 lore raw

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

Quoted 20 lines · David Ahern

From: David Ahern <[email protected]>

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

Quoted 6 lines
Quoted 1 line

4-byte values.

Block attach of BPF_XDP_DEVMAP programs to devices.

Signed-off-by David Ahern <[email protected]>

---

Please cc [email protected] in the future for patches related to BPF in general.

Quoted 7 lines
 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(-)

[...]

kernel/bpf/devmap.c static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp, · line 470

Quoted 11 lines
kernel/bpf/devmap.c +10 -0
+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 */
Quoted 4 lines
kernel/bpf/devmap.c +4 -0
+       case XDP_PASS:
+               break;
+       default:
+               bpf_warn_invalid_xdp_action(act);

kernel/bpf/devmap.c static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp, · line 484

Quoted 10 lines
kernel/bpf/devmap.c +10 -0
+               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?

Quoted 5 lines
kernel/bpf/devmap.c +5 -0
+       }
+
+       return xdp;
+}
+

[...]

#6 lore raw

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

Quoted 3 lines · Andrii Nakryiko

Please cc [email protected] in the future for patches related to BPF in general.

added to my script

Quoted 62 lines
Quoted 8 lines
 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(-)

[...]

kernel/bpf/devmap.c static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp, · line 470

Quoted 11 lines
kernel/bpf/devmap.c +10 -0
+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 */
Quoted 4 lines
kernel/bpf/devmap.c +4 -0
+       case XDP_PASS:
+               break;
+       default:
+               bpf_warn_invalid_xdp_action(act);

kernel/bpf/devmap.c static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp, · line 484

Quoted 10 lines
kernel/bpf/devmap.c +10 -0
+               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;
}
Quoted 2 lines

Wouldn't this be simpler?

Switched it to this which captures your intent with a more traditional return location.

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

        xdp_return_buff(xdp);
        return NULL;
#7 lore raw

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

Quoted 76 lines · David Ahern
Quoted 3 lines · Andrii Nakryiko

Please cc [email protected] in the future for patches related to BPF in general.

added to my script

Quoted 62 lines
Quoted 8 lines
 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(-)

[...]

kernel/bpf/devmap.c static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp, · line 470

Quoted 11 lines
kernel/bpf/devmap.c +10 -0
+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 */
Quoted 4 lines
kernel/bpf/devmap.c +4 -0
+       case XDP_PASS:
+               break;
+       default:
+               bpf_warn_invalid_xdp_action(act);

kernel/bpf/devmap.c static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp, · line 484

Quoted 10 lines
kernel/bpf/devmap.c +10 -0
+               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;
}
Quoted 2 lines

Wouldn't this be simpler?

Switched it to this which captures your intent with a more traditional return location.

Quoted 16 lines
        act = bpf_prog_run_xdp(xdp_prog, xdp);
        switch (act) {
        case XDP_PASS:
                return xdp;
        case XDP_DROP:
                break;
        default:
                bpf_warn_invalid_xdp_action(act);
                fallthrough;
        case XDP_ABORTED:
                trace_xdp_exception(dev, xdp_prog, act);
                break;
        }

        xdp_return_buff(xdp);
        return NULL;

looks good as well, thanks

#8 lore raw

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

In reply to David Ahern
Quoted 109 lines · David Ahern

From: David Ahern <[email protected]>

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

Quoted 1 line

4-byte values.

Block attach of BPF_XDP_DEVMAP programs to devices.

Signed-off-by David Ahern <[email protected]>
---
 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(-)
include/linux/bpf.h +5 -0
index efe8836b5c48..088751bc09aa 100644
@@ -1242,6 +1242,7 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
 		    struct net_device *dev_rx);
 int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
 			     struct bpf_prog *xdp_prog);
+bool dev_map_can_have_prog(struct bpf_map *map);
 
 struct bpf_cpu_map_entry *__cpu_map_lookup_elem(struct bpf_map *map, u32 key);
 void __cpu_map_flush(void);

@@ -1355,6 +1356,10 @@ static inline struct net_device  *__dev_map_hash_lookup_elem(struct bpf_map *map
 {
 	return NULL;
 }
+static inline bool dev_map_can_have_prog(struct bpf_map *map)
+{
+	return false;
+}
 
 static inline void __dev_flush(void)
 {
include/uapi/linux/bpf.h +5 -0
index d27302ecaa9c..2d9927b7a922 100644
@@ -224,6 +224,7 @@ enum bpf_attach_type {
 	BPF_CGROUP_INET6_GETPEERNAME,
 	BPF_CGROUP_INET4_GETSOCKNAME,
 	BPF_CGROUP_INET6_GETSOCKNAME,
+	BPF_XDP_DEVMAP,
 	__MAX_BPF_ATTACH_TYPE
 };
 

@@ -3628,6 +3629,10 @@ struct xdp_md {
 /* DEVMAP values */
 struct devmap_val {
 	__u32 ifindex;   /* device index */
+	union {
+		int   bpf_prog_fd;  /* prog fd on map write */
+		__u32 bpf_prog_id;  /* prog id on map read */
+	};
 };
 
 enum sk_action {
kernel/bpf/devmap.c +5 -0
index 069a50113e26..a628585a31e1 100644
@@ -64,6 +64,7 @@ struct bpf_dtab_netdev {
 	struct net_device *dev; /* must be first member, due to tracepoint */
 	struct hlist_node index_hlist;
 	struct bpf_dtab *dtab;
+	struct bpf_prog *xdp_prog;
 	struct rcu_head rcu;
 	unsigned int idx;
 	struct devmap_val val;

@@ -219,6 +220,8 @@ static void dev_map_free(struct bpf_map *map)
 
 			hlist_for_each_entry_safe(dev, next, head, index_hlist) {
 				hlist_del_rcu(&dev->index_hlist);
+				if (dev->xdp_prog)
+					bpf_prog_put(dev->xdp_prog);
 				dev_put(dev->dev);
 				kfree(dev);
 			}

@@ -233,6 +236,8 @@ static void dev_map_free(struct bpf_map *map)
 			if (!dev)
 				continue;
 
+			if (dev->xdp_prog)
+				bpf_prog_put(dev->xdp_prog);
 			dev_put(dev->dev);
 			kfree(dev);
 		}

kernel/bpf/devmap.c static int dev_map_hash_get_next_key(struct bpf_map *map, void *key, · line 331

Quoted 8 lines
kernel/bpf/devmap.c +5 -0
@@ -319,6 +324,16 @@ static int dev_map_hash_get_next_key(struct bpf_map *map, void *key,
 	return -ENOENT;
 }
 
+bool dev_map_can_have_prog(struct bpf_map *map)
+{
+	if ((map->map_type == BPF_MAP_TYPE_DEVMAP ||
+	     map->map_type == BPF_MAP_TYPE_DEVMAP_HASH) &&
+	    map->value_size != 4)

nit (since you've gotten rid of the magic sizes everywhere else) how about:

map->value_size != sizeof_field(struct devmap_val, ifindex)

author
#9 lore raw

PATCH v2 3/5 xdp: Add xdp_txq_info to xdp_buff

In reply to David Ahern

From: David Ahern <[email protected]>

Add xdp_txq_info as the Tx counterpart to xdp_rxq_info. At the moment only the device is added. Other fields (queue_index) can be added as use cases arise.

From a UAPI perspective, add egress_ifindex to xdp context for bpf programs to see the Tx device.

Update the verifier to only allow accesses to egress_ifindex by XDP programs with BPF_XDP_DEVMAP expected attach type.

Signed-off-by David Ahern <[email protected]>
---
 include/net/xdp.h              |  5 +++++
 include/uapi/linux/bpf.h       |  2 ++
 kernel/bpf/devmap.c            |  3 +++
 net/core/filter.c              | 17 +++++++++++++++++
 tools/include/uapi/linux/bpf.h |  2 ++
 5 files changed, 29 insertions(+)
include/net/xdp.h +5 -0
index 90f11760bd12..d54022959491 100644
@@ -61,12 +61,17 @@ struct xdp_rxq_info {
 	struct xdp_mem_info mem;
 } ____cacheline_aligned; /* perf critical, avoid false-sharing */
 
+struct xdp_txq_info {
+	struct net_device *dev;
+};
+
 struct xdp_buff {
 	void *data;
 	void *data_end;
 	void *data_meta;
 	void *data_hard_start;
 	struct xdp_rxq_info *rxq;
+	struct xdp_txq_info *txq;
 	u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
 };
include/uapi/linux/bpf.h +2 -0
index 2d9927b7a922..a0ebbbe066b2 100644
@@ -3624,6 +3624,8 @@ struct xdp_md {
 	/* Below access go through struct xdp_rxq_info */
 	__u32 ingress_ifindex; /* rxq->dev->ifindex */
 	__u32 rx_queue_index;  /* rxq->queue_index  */
+
+	__u32 egress_ifindex;  /* txq->dev->ifindex */
 };
 
 /* DEVMAP values */
kernel/bpf/devmap.c +3 -0
index a628585a31e1..73e4a6cd6c2b 100644
@@ -462,8 +462,11 @@ static struct xdp_buff *dev_map_run_prog(struct net_device *dev,
 					 struct xdp_buff *xdp,
 					 struct bpf_prog *xdp_prog)
 {
+	struct xdp_txq_info txq = { .dev = dev };
 	u32 act;
 
+	xdp->txq = &txq;
+
 	act = bpf_prog_run_xdp(xdp_prog, xdp);
 	switch (act) {
 	case XDP_DROP:
net/core/filter.c +17 -0
index a6fc23447f12..2e9dbfd8e60c 100644
@@ -7014,6 +7014,13 @@ static bool xdp_is_valid_access(int off, int size,
 				const struct bpf_prog *prog,
 				struct bpf_insn_access_aux *info)
 {
+	if (prog->expected_attach_type != BPF_XDP_DEVMAP) {
+		switch (off) {
+		case offsetof(struct xdp_md, egress_ifindex):
+			return false;
+		}
+	}
+
 	if (type == BPF_WRITE) {
 		if (bpf_prog_is_dev_bound(prog->aux)) {
 			switch (off) {

@@ -7967,6 +7974,16 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
 				      offsetof(struct xdp_rxq_info,
 					       queue_index));
 		break;
+	case offsetof(struct xdp_md, egress_ifindex):
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, txq),
+				      si->dst_reg, si->src_reg,
+				      offsetof(struct xdp_buff, txq));
+		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_txq_info, dev),
+				      si->dst_reg, si->dst_reg,
+				      offsetof(struct xdp_txq_info, dev));
+		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
+				      offsetof(struct net_device, ifindex));
+		break;
 	}
 
 	return insn - insn_buf;
tools/include/uapi/linux/bpf.h +2 -0
index 2d9927b7a922..a0ebbbe066b2 100644
@@ -3624,6 +3624,8 @@ struct xdp_md {
 	/* Below access go through struct xdp_rxq_info */
 	__u32 ingress_ifindex; /* rxq->dev->ifindex */
 	__u32 rx_queue_index;  /* rxq->queue_index  */
+
+	__u32 egress_ifindex;  /* txq->dev->ifindex */
 };
 
 /* DEVMAP values */
Signature · 1 line
2.21.1 (Apple Git-122.3)
author
#10 lore raw

PATCH v2 4/5 libbpf: Add SEC name for xdp programs attached to device map

In reply to David Ahern

Support SEC("xdp_dm*") as a short cut for loading the program with type BPF_PROG_TYPE_XDP and expected attach type BPF_XDP_DEVMAP.

Signed-off-by David Ahern <[email protected]>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)
tools/lib/bpf/libbpf.c +2 -0
index 5d60de6fd818..493909d5d3d3 100644
@@ -6657,6 +6657,8 @@ static const struct bpf_sec_def section_defs[] = {
 		.expected_attach_type = BPF_TRACE_ITER,
 		.is_attach_btf = true,
 		.attach_fn = attach_iter),
+	BPF_EAPROG_SEC("xdp_dm",		BPF_PROG_TYPE_XDP,
+						BPF_XDP_DEVMAP),
 	BPF_PROG_SEC("xdp",			BPF_PROG_TYPE_XDP),
 	BPF_PROG_SEC("perf_event",		BPF_PROG_TYPE_PERF_EVENT),
 	BPF_PROG_SEC("lwt_in",			BPF_PROG_TYPE_LWT_IN),
Signature · 1 line
2.21.1 (Apple Git-122.3)
#11 lore raw

Re: [PATCH v2 bpf-next 4/5] libbpf: Add SEC name for xdp programs attached to device map

Quoted 14 lines · David Ahern

Support SEC("xdp_dm*") as a short cut for loading the program with type BPF_PROG_TYPE_XDP and expected attach type BPF_XDP_DEVMAP.

Signed-off-by David Ahern <[email protected]>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)
tools/lib/bpf/libbpf.c +0 -0
index 5d60de6fd818..493909d5d3d3 100644

tools/lib/bpf/libbpf.c static const struct bpf_sec_def section_defs[] = { · line 6661

Quoted 5 lines
tools/lib/bpf/libbpf.c +2 -0
@@ -6657,6 +6657,8 @@ static const struct bpf_sec_def section_defs[] = {
                .expected_attach_type = BPF_TRACE_ITER,
                .is_attach_btf = true,
                .attach_fn = attach_iter),
+       BPF_EAPROG_SEC("xdp_dm",                BPF_PROG_TYPE_XDP,
+                                               BPF_XDP_DEVMAP),

naming is hard and subjective, but does "dm" really associate with DEVMAP to you, rather than "direct message" or "direct memory" or something along those line? Is there any harm to call this "xdp_devmap"? It's still short enough, IMO.

Quoted 6 lines
tools/lib/bpf/libbpf.c +0 -1
        BPF_PROG_SEC("xdp",                     BPF_PROG_TYPE_XDP),
        BPF_PROG_SEC("perf_event",              BPF_PROG_TYPE_PERF_EVENT),
        BPF_PROG_SEC("lwt_in",                  BPF_PROG_TYPE_LWT_IN),
--

2.21.1 (Apple Git-122.3)

#12 lore raw

Re: [PATCH v2 bpf-next 4/5] libbpf: Add SEC name for xdp programs attached to device map

Quoted 5 lines · Andrii Nakryiko
Quoted 5 lines
tools/lib/bpf/libbpf.c +0 -0
index 5d60de6fd818..493909d5d3d3 100644
@@ -6657,6 +6657,8 @@ static const struct bpf_sec_def section_defs[] = {
Quoted 8 lines

tools/lib/bpf/libbpf.c static const struct bpf_sec_def section_defs[] = { · line 6661

Quoted 5 lines
tools/lib/bpf/libbpf.c +2 -0
                .expected_attach_type = BPF_TRACE_ITER,
                .is_attach_btf = true,
                .attach_fn = attach_iter),
+       BPF_EAPROG_SEC("xdp_dm",                BPF_PROG_TYPE_XDP,
+                                               BPF_XDP_DEVMAP),

naming is hard and subjective, but does "dm" really associate with DEVMAP to you, rather than "direct message" or "direct memory" or

Yes it does b/c of the XDP context. Program name lengths being limited to 15 characters makes me shorten all prefixes to leave some usable characters for id'ing the program.

Quoted 3 lines

something along those line? Is there any harm to call this "xdp_devmap"? It's still short enough, IMO.

but for the SEC name, I switched it to xdp_devmap.

author
#13 lore raw

PATCH v2 5/5 selftest: Add tests for XDP programs in devmap entries

In reply to David Ahern

Add tests to verify ability to add an XDP program to a entry in a DEVMAP.

Add negative tests to show DEVMAP programs can not be attached to devices as a normal XDP program, and accesses to egress_ifindex require BPF_XDP_DEVMAP attach type.

Signed-off-by David Ahern <[email protected]>
---
 .../bpf/prog_tests/xdp_devmap_attach.c        | 94 +++++++++++++++++++
 .../selftests/bpf/progs/test_xdp_devmap.c     | 19 ++++
 .../selftests/bpf/progs/test_xdp_devmap2.c    | 19 ++++
 .../bpf/progs/test_xdp_with_devmap.c          | 17 ++++
 4 files changed, 149 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap2.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c
tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file +94 -0
new file mode 100644
index 000000000000..d81b2b366f39
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <uapi/linux/bpf.h>
+#include <linux/if_link.h>
+#include <test_progs.h>
+
+#define IFINDEX_LO 1
+
+void test_xdp_devmap_attach(void)
+{
+	struct bpf_prog_load_attr attr = {
+		.prog_type = BPF_PROG_TYPE_XDP,
+	};
+	struct bpf_object *obj, *dm_obj = NULL;
+	int err, dm_fd = -1, fd = -1, map_fd;
+	struct bpf_prog_info info = {};
+	struct devmap_val val = {
+		.ifindex = IFINDEX_LO,
+	};
+	__u32 id, len = sizeof(info);
+	__u32 duration = 0, idx = 0;
+
+	attr.file = "./test_xdp_with_devmap.o",
+	err = bpf_prog_load_xattr(&attr, &obj, &fd);
+	if (CHECK(err, "load of xdp program with 8-byte devmap",
+		  "err %d errno %d\n", err, errno))
+		return;
+
+	/* can not attach program with DEVMAPs that allow programs */
+	err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE);
+	CHECK(err == 0, "Generic attach of program with 8-byte devmap",
+	      "should have failed\n");
+
+	map_fd = bpf_object__find_map_fd_by_name(obj, "dm_ports");
+	if (CHECK(map_fd < 0, "Lookup dm_ports map",
+		  "err %d errno %d\n", err, errno))
+		goto out_close;
+
+	/*
+	 * basic test - load program to be installed in devmap;
+	 * also verifies access to ingress and egress ifindices.
+	 */
+	attr.expected_attach_type = BPF_XDP_DEVMAP;
+	attr.file = "./test_xdp_devmap.o";
+	err = bpf_prog_load_xattr(&attr, &dm_obj, &dm_fd);
+	if (CHECK(err, "Load of BPF_XDP_DEVMAP program",
+		  "err %d errno %d\n", err, errno))
+		goto out_close;
+
+	err = bpf_obj_get_info_by_fd(dm_fd, &info, &len);
+	if (CHECK_FAIL(err))
+		goto out_close;
+
+	val.bpf_prog_fd = dm_fd;
+	err = bpf_map_update_elem(map_fd, &idx, &val, 0);
+	CHECK(err, "Add program to devmap entry",
+	      "err %d errno %d\n", err, errno);
+
+	err = bpf_map_lookup_elem(map_fd, &id, &val);
+	CHECK(err, "Read devmap entry",
+	      "err %d errno %d\n", err, errno);
+	CHECK(info.id != val.bpf_prog_id, "Expected program id in devmap entry",
+	      "expected %u read %u\n", info.id, val.bpf_prog_id);
+
+	/* can not attach BPF_XDP_DEVMAP program to a device */
+	err = bpf_set_link_xdp_fd(IFINDEX_LO, dm_fd, XDP_FLAGS_SKB_MODE);
+	CHECK(err == 0, "Attach of BPF_XDP_DEVMAP program",
+	      "should have failed\n");
+
+	bpf_object__close(dm_obj);
+
+	/* Load of BPF_XDP_DEVMAP as XDP should fail because of egress index */
+	attr.expected_attach_type = 0;
+	err = bpf_prog_load_xattr(&attr, &dm_obj, &dm_fd);
+	if (CHECK(err == 0,
+		  "Load of XDP program accessing egress ifindex without attach type",
+		  "should have failed\n"))
+		bpf_object__close(dm_obj);
+
+	attr.file = "./xdp_dummy.o";
+	err = bpf_prog_load_xattr(&attr, &dm_obj, &dm_fd);
+	if (CHECK_FAIL(err))
+		goto out_close;
+
+	val.ifindex = 1;
+	val.bpf_prog_fd = dm_fd;
+	err = bpf_map_update_elem(map_fd, &idx, &val, 0);
+	CHECK(err == 0, "Add non-BPF_XDP_DEVMAP program to devmap entry",
+	      "should have failed\n");
+
+	bpf_object__close(dm_obj);
+
+out_close:
+	bpf_object__close(obj);
+}
tools/testing/selftests/bpf/progs/test_xdp_devmap.c new file +19 -0
new file mode 100644
index 000000000000..64fc2c3cae01
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* program inserted into devmap entry */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("xdp_devmap_log")
+int xdpdm_devlog(struct xdp_md *ctx)
+{
+	char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n";
+	void *data_end = (void *)(long)ctx->data_end;
+	void *data = (void *)(long)ctx->data;
+	unsigned int len = data_end - data;
+
+	bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len);
+
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
tools/testing/selftests/bpf/progs/test_xdp_devmap2.c new file +19 -0
new file mode 100644
index 000000000000..64fc2c3cae01
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* program inserted into devmap entry */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("xdp_devmap_log")
+int xdpdm_devlog(struct xdp_md *ctx)
+{
+	char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n";
+	void *data_end = (void *)(long)ctx->data_end;
+	void *data = (void *)(long)ctx->data;
+	unsigned int len = data_end - data;
+
+	bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len);
+
+	return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c new file +17 -0
new file mode 100644
index 000000000000..815cd59b4866
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct bpf_map_def SEC("maps") dm_ports = {
+	.type = BPF_MAP_TYPE_DEVMAP,
+	.key_size = sizeof(__u32),
+	.value_size = sizeof(struct devmap_val),
+	.max_entries = 4,
+};
+
+SEC("xdp_devmap")
+int xdp_with_devmap(struct xdp_md *ctx)
+{
+	return bpf_redirect_map(&dm_ports, 1, 0);
+}
Signature · 1 line
2.21.1 (Apple Git-122.3)
#14 lore raw

Re: [PATCH v2 bpf-next 5/5] selftest: Add tests for XDP programs in devmap entries

Quoted 39 lines · David Ahern

Add tests to verify ability to add an XDP program to a entry in a DEVMAP.

Add negative tests to show DEVMAP programs can not be attached to devices as a normal XDP program, and accesses to egress_ifindex require BPF_XDP_DEVMAP attach type.

Signed-off-by David Ahern <[email protected]>
---
 .../bpf/prog_tests/xdp_devmap_attach.c        | 94 +++++++++++++++++++
 .../selftests/bpf/progs/test_xdp_devmap.c     | 19 ++++
 .../selftests/bpf/progs/test_xdp_devmap2.c    | 19 ++++
 .../bpf/progs/test_xdp_with_devmap.c          | 17 ++++
 4 files changed, 149 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_devmap2.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c
tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file +13 -0
new file mode 100644
index 000000000000..d81b2b366f39
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <uapi/linux/bpf.h>
+#include <linux/if_link.h>
+#include <test_progs.h>
+
+#define IFINDEX_LO 1
+
+void test_xdp_devmap_attach(void)
+{
+       struct bpf_prog_load_attr attr = {
+               .prog_type = BPF_PROG_TYPE_XDP,
+       };
+       struct bpf_object *obj, *dm_obj = NULL;

tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c line 23

Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file +10 -0
+       int err, dm_fd = -1, fd = -1, map_fd;
+       struct bpf_prog_info info = {};
+       struct devmap_val val = {
+               .ifindex = IFINDEX_LO,
+       };
+       __u32 id, len = sizeof(info);
+       __u32 duration = 0, idx = 0;
+
+       attr.file = "./test_xdp_with_devmap.o",
+       err = bpf_prog_load_xattr(&attr, &obj, &fd);

please use skeletons instead of loading .o files.

Quoted 4 lines
tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file +4 -0
+       if (CHECK(err, "load of xdp program with 8-byte devmap",
+                 "err %d errno %d\n", err, errno))
+               return;
+

[...]

Quoted 35 lines
tools/testing/selftests/bpf/progs/test_xdp_devmap.c new file +19 -0
new file mode 100644
index 000000000000..64fc2c3cae01
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* program inserted into devmap entry */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("xdp_devmap_log")
+int xdpdm_devlog(struct xdp_md *ctx)
+{
+       char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n";
+       void *data_end = (void *)(long)ctx->data_end;
+       void *data = (void *)(long)ctx->data;
+       unsigned int len = data_end - data;
+
+       bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len);
+
+       return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
tools/testing/selftests/bpf/progs/test_xdp_devmap2.c new file +4 -0
new file mode 100644
index 000000000000..64fc2c3cae01
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* program inserted into devmap entry */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>

tools/testing/selftests/bpf/progs/test_xdp_devmap2.c line 14

Quoted 10 lines
tools/testing/selftests/bpf/progs/test_xdp_devmap2.c new file +10 -0
+
+SEC("xdp_devmap_log")
+int xdpdm_devlog(struct xdp_md *ctx)
+{
+       char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n";
+       void *data_end = (void *)(long)ctx->data_end;
+       void *data = (void *)(long)ctx->data;
+       unsigned int len = data_end - data;
+
+       bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len);

instead of just printing ifindexes, why not return them through global variable and validate in a test?

Quoted 13 lines
tools/testing/selftests/bpf/progs/test_xdp_devmap2.c new file +5 -0
+
+       return XDP_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c new file +2 -0
new file mode 100644
index 000000000000..815cd59b4866
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+

tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c line 12

Quoted 10 lines
tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c new file +10 -0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct bpf_map_def SEC("maps") dm_ports = {
+       .type = BPF_MAP_TYPE_DEVMAP,
+       .key_size = sizeof(__u32),
+       .value_size = sizeof(struct devmap_val),
+       .max_entries = 4,
+};
+

This is an old syntax for maps, all the selftests were converted to BTF-defined maps. Please update.

Quoted 8 lines
tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c new file +5 -1
+SEC("xdp_devmap")
+int xdp_with_devmap(struct xdp_md *ctx)
+{
+       return bpf_redirect_map(&dm_ports, 1, 0);
+}
--

2.21.1 (Apple Git-122.3)

#15 lore raw

Re: [PATCH v2 bpf-next 5/5] selftest: Add tests for XDP programs in devmap entries

Quoted 13 lines · On Thu, 28 May 2020 00:08:34 -0700 Andrii Nakryiko <[email protected]>…
Quoted 13 lines
tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c new file +7 -0
new file mode 100644
index 000000000000..815cd59b4866
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+struct bpf_map_def SEC("maps") dm_ports = {
+       .type = BPF_MAP_TYPE_DEVMAP,
Quoted 8 lines

tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c line 12

Quoted 5 lines
tools/testing/selftests/bpf/progs/test_xdp_with_devmap.c new file +5 -0
+       .key_size = sizeof(__u32),
+       .value_size = sizeof(struct devmap_val),
+       .max_entries = 4,
+};
+

This is an old syntax for maps, all the selftests were converted to BTF-defined maps. Please update.

LOL - The map type BPF_MAP_TYPE_DEVMAP does not support BTF for key and value, which it what I've been trying to point out... (and yes, I do have code that makes it work in my tree.).

That said, you should use the new SEC(".maps") definitions, but you need to use some tricks to avoid a BTF-ID getting generated. Let me help you with something that should work:

/* DEVMAP values */

struct devmap_val {
	__u32 ifindex;   /* device index */
};
struct {
	__uint(type, BPF_MAP_TYPE_DEVMAP);
	__uint(key_size, sizeof(u32));
	__uint(value_size, sizeof(struct devmap_val));
	__uint(max_entries, 4);
} dm_ports SEC(".maps");

Notice by setting key_size and value_size, instead of the "__type", then a BTF-ID will be generated for this map.
Normally with proper BTF it should look like:

struct {
	__uint(type, BPF_MAP_TYPE_DEVMAP);
	__type(key, u32);
	__type(value, struct devmap_val);
	__uint(max_entries, 4);
} dm_ports_with_BTF SEC(".maps");
Signature · 4 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
#16 lore raw

Re: [PATCH v2 bpf-next 5/5] selftest: Add tests for XDP programs in devmap entries

Quoted 20 lines · Jesper Dangaard Brouer

That said, you should use the new SEC(".maps") definitions, but you need to use some tricks to avoid a BTF-ID getting generated. Let me help you with something that should work:

/* DEVMAP values */

struct devmap_val {
	__u32 ifindex;   /* device index */
};
struct {
	__uint(type, BPF_MAP_TYPE_DEVMAP);
	__uint(key_size, sizeof(u32));
	__uint(value_size, sizeof(struct devmap_val));
	__uint(max_entries, 4);
} dm_ports SEC(".maps");

Notice by setting key_size and value_size, instead of the "__type", then a BTF-ID will be generated for this map.
Normally with proper BTF it should look like:

Quoted 8 lines
struct {
	__uint(type, BPF_MAP_TYPE_DEVMAP);
	__type(key, u32);
	__type(value, struct devmap_val);
	__uint(max_entries, 4);
} dm_ports_with_BTF SEC(".maps");

Thanks for the tip.

#17 lore raw

Re: [PATCH v2 bpf-next 5/5] selftest: Add tests for XDP programs in devmap entries

In reply to Andrii Nakryiko
Quoted 23 lines · Andrii Nakryiko
Quoted 23 lines
tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file +17 -0
new file mode 100644
index 000000000000..d81b2b366f39
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <uapi/linux/bpf.h>
+#include <linux/if_link.h>
+#include <test_progs.h>
+
+#define IFINDEX_LO 1
+
+void test_xdp_devmap_attach(void)
+{
+       struct bpf_prog_load_attr attr = {
+               .prog_type = BPF_PROG_TYPE_XDP,
+       };
+       struct bpf_object *obj, *dm_obj = NULL;
+       int err, dm_fd = -1, fd = -1, map_fd;
+       struct bpf_prog_info info = {};
+       struct devmap_val val = {
+               .ifindex = IFINDEX_LO,
Quoted 8 lines

tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c line 23

Quoted 6 lines
tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file +6 -0
+       };
+       __u32 id, len = sizeof(info);
+       __u32 duration = 0, idx = 0;
+
+       attr.file = "./test_xdp_with_devmap.o",
+       err = bpf_prog_load_xattr(&attr, &obj, &fd);

please use skeletons instead of loading .o files.

I will look into it.

Quoted 24 lines
Quoted 5 lines
tools/testing/selftests/bpf/prog_tests/xdp_devmap_attach.c new file +4 -0
+       if (CHECK(err, "load of xdp program with 8-byte devmap",
+                 "err %d errno %d\n", err, errno))
+               return;
+

[...]

Quoted 15 lines
tools/testing/selftests/bpf/progs/test_xdp_devmap2.c new file +9 -0
new file mode 100644
index 000000000000..64fc2c3cae01
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+/* program inserted into devmap entry */
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("xdp_devmap_log")
+int xdpdm_devlog(struct xdp_md *ctx)
+{
+       char fmt[] = "devmap redirect: dev %u -> dev %u len %u\n";
Quoted 8 lines

tools/testing/selftests/bpf/progs/test_xdp_devmap2.c line 14

Quoted 5 lines
tools/testing/selftests/bpf/progs/test_xdp_devmap2.c new file +5 -0
+       void *data_end = (void *)(long)ctx->data_end;
+       void *data = (void *)(long)ctx->data;
+       unsigned int len = data_end - data;
+
+       bpf_trace_printk(fmt, sizeof(fmt), ctx->ingress_ifindex, ctx->egress_ifindex, len);

instead of just printing ifindexes, why not return them through global variable and validate in a test?

The point of this program to access the egress_ifindex with the expected attached type NOT set to BPF_XDP_DEVMAP to FAIL the verifier checks.