[PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[David Ahern]

Implementation of Daniel's proposal for allowing DEVMAP entries to be
a device index, program id pair. Daniel suggested an fd to specify the
program, but that seems odd to me that you insert the value as an fd, but
read it back as an id since the fd can be closed.

David Ahern (4):
  bpf: Handle 8-byte values in DEVMAP and DEVMAP_HASH
  bpf: Add support to attach bpf program to a devmap
  xdp: Add xdp_txq_info to xdp_buff
  bpftool: Add SEC name for xdp programs attached to device map

 include/linux/bpf.h            |   5 ++
 include/net/xdp.h              |   5 ++
 include/uapi/linux/bpf.h       |   3 +
 kernel/bpf/devmap.c            | 132 +++++++++++++++++++++++++++++----
 net/core/dev.c                 |  18 +++++
 net/core/filter.c              |  17 +++++
 tools/include/uapi/linux/bpf.h |   3 +
 tools/lib/bpf/libbpf.c         |   2 +
 8 files changed, 170 insertions(+), 15 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[PATCH RFC bpf-next 1/4] bpf: Handle 8-byte values in DEVMAP and DEVMAP_HASH

[David Ahern]

Add support to DEVMAP and DEVMAP_HASH to support 8-byte values as a
<device index, program id> pair. To do this, a new struct is needed in
bpf_dtab_netdev to hold the values to return on lookup.

Signed-off-by: David Ahern <dsahern@kernel.org>
---
 kernel/bpf/devmap.c | 53 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index a51d9fb7a359..2c01ce434306 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -60,12 +60,19 @@ struct xdp_dev_bulk_queue {
 	unsigned int count;
 };
 
+/* devmap value can be dev index or dev index + prog id */
+struct dev_map_ext_val {
+	u32 ifindex;	/* must be first for compat with 4-byte values */
+	u32 prog_id;
+};
+
 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 rcu_head rcu;
 	unsigned int idx;
+	struct dev_map_ext_val val;
 };
 
 struct bpf_dtab {
@@ -108,9 +115,13 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
 	u64 cost = 0;
 	int err;
 
-	/* check sanity of attributes */
+	/* check sanity of attributes. 2 value sizes supported:
+	 * 4 bytes: ifindex
+	 * 8 bytes: ifindex + prog id
+	 */
 	if (attr->max_entries == 0 || attr->key_size != 4 ||
-	    attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
+	    (attr->value_size != 4 && attr->value_size != 8) ||
+	    attr->map_flags & ~DEV_CREATE_FLAG_MASK)
 		return -EINVAL;
 
 	/* Lookup returns a pointer straight to dev->ifindex, so make sure the
@@ -472,18 +483,15 @@ int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
 static void *dev_map_lookup_elem(struct bpf_map *map, void *key)
 {
 	struct bpf_dtab_netdev *obj = __dev_map_lookup_elem(map, *(u32 *)key);
-	struct net_device *dev = obj ? obj->dev : NULL;
 
-	return dev ? &dev->ifindex : NULL;
+	return obj ? &obj->val : NULL;
 }
 
 static void *dev_map_hash_lookup_elem(struct bpf_map *map, void *key)
 {
 	struct bpf_dtab_netdev *obj = __dev_map_hash_lookup_elem(map,
 								*(u32 *)key);
-	struct net_device *dev = obj ? obj->dev : NULL;
-
-	return dev ? &dev->ifindex : NULL;
+	return obj ? &obj->val : NULL;
 }
 
 static void __dev_map_entry_free(struct rcu_head *rcu)
@@ -552,15 +560,18 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
 		return ERR_PTR(-ENOMEM);
 
 	dev->dev = dev_get_by_index(net, ifindex);
-	if (!dev->dev) {
-		kfree(dev);
-		return ERR_PTR(-EINVAL);
-	}
+	if (!dev->dev)
+		goto err_out;
 
 	dev->idx = idx;
 	dev->dtab = dtab;
 
+	dev->val.ifindex = ifindex;
+
 	return dev;
+err_out:
+	kfree(dev);
+	return ERR_PTR(-EINVAL);
 }
 
 static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
@@ -568,8 +579,16 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
 {
 	struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
 	struct bpf_dtab_netdev *dev, *old_dev;
-	u32 ifindex = *(u32 *)value;
 	u32 i = *(u32 *)key;
+	u32 ifindex;
+
+	if (map->value_size == 4) {
+		ifindex = *(u32 *)value;
+	} else {
+		struct dev_map_ext_val *val = value;
+
+		ifindex = val->ifindex;
+	}
 
 	if (unlikely(map_flags > BPF_EXIST))
 		return -EINVAL;
@@ -609,10 +628,18 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
 {
 	struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
 	struct bpf_dtab_netdev *dev, *old_dev;
-	u32 ifindex = *(u32 *)value;
 	u32 idx = *(u32 *)key;
 	unsigned long flags;
 	int err = -EEXIST;
+	u32 ifindex;
+
+	if (map->value_size == 4) {
+		ifindex = *(u32 *)value;
+	} else {
+		struct dev_map_ext_val *val = value;
+
+		ifindex = val->ifindex;
+	}
 
 	if (unlikely(map_flags > BPF_EXIST || !ifindex))
 		return -EINVAL;
-- 
2.21.1 (Apple Git-122.3)

[PATCH RFC bpf-next 2/4] bpf: Add support to attach bpf program to a devmap

[David Ahern]

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

DEVMAPs can associate a program with a device entry by setting the
value to <index,id> pair. The program associated with the id 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
8-bytes values.

Signed-off-by: David Ahern <dsahern@kernel.org>
---
 include/linux/bpf.h            |  5 +++
 include/uapi/linux/bpf.h       |  1 +
 kernel/bpf/devmap.c            | 82 +++++++++++++++++++++++++++++++---
 net/core/dev.c                 | 18 ++++++++
 tools/include/uapi/linux/bpf.h |  1 +
 5 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index efe8836b5c48..088751bc09aa 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -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)
 {
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 97e1fd19ff58..8c2c0d0c9a0e 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -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
 };
 
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 2c01ce434306..06f4c746fa7c 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -70,6 +70,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 dev_map_ext_val val;
@@ -228,6 +229,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);
 			}
@@ -242,6 +245,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);
 		}
@@ -328,6 +333,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 == 8)
+		return true;
+
+	return false;
+}
+
 static int bq_xmit_all(struct xdp_dev_bulk_queue *bq, u32 flags)
 {
 	struct net_device *dev = bq->dev;
@@ -452,6 +467,39 @@ static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp,
 	return bq_enqueue(dev, xdpf, dev_rx);
 }
 
+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;
+	case XDP_PASS:
+		break;
+	case XDP_TX:
+		fallthrough;
+	case XDP_REDIRECT:
+		fallthrough;
+	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;
+	}
+
+	return xdp;
+}
+
 int dev_xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp,
 		    struct net_device *dev_rx)
 {
@@ -463,6 +511,11 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
 {
 	struct net_device *dev = dst->dev;
 
+	if (dst->xdp_prog) {
+		xdp = dev_map_run_prog(dev, xdp, dst->xdp_prog);
+		if (!xdp)
+			return 0;
+	}
 	return __xdp_enqueue(dev, xdp, dev_rx);
 }
 
@@ -499,6 +552,8 @@ static void __dev_map_entry_free(struct rcu_head *rcu)
 	struct bpf_dtab_netdev *dev;
 
 	dev = container_of(rcu, struct bpf_dtab_netdev, rcu);
+	if (dev->xdp_prog)
+		bpf_prog_put(dev->xdp_prog);
 	dev_put(dev->dev);
 	kfree(dev);
 }
@@ -549,9 +604,10 @@ static int dev_map_hash_delete_elem(struct bpf_map *map, void *key)
 
 static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
 						    struct bpf_dtab *dtab,
-						    u32 ifindex,
+						    u32 ifindex, u32 prog_id,
 						    unsigned int idx)
 {
+	struct bpf_prog *prog = NULL;
 	struct bpf_dtab_netdev *dev;
 
 	dev = kmalloc_node(sizeof(*dev), GFP_ATOMIC | __GFP_NOWARN,
@@ -563,12 +619,23 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
 	if (!dev->dev)
 		goto err_out;
 
+	if (prog_id) {
+		prog = bpf_prog_by_id(prog_id);
+		if (IS_ERR(prog) || prog->type != BPF_PROG_TYPE_XDP ||
+		    prog->expected_attach_type != BPF_XDP_DEVMAP)
+			goto err_dev;
+	}
+
 	dev->idx = idx;
 	dev->dtab = dtab;
 
+	dev->xdp_prog = prog;
 	dev->val.ifindex = ifindex;
+	dev->val.prog_id = prog_id;
 
 	return dev;
+err_dev:
+	dev_put(dev->dev);
 err_out:
 	kfree(dev);
 	return ERR_PTR(-EINVAL);
@@ -580,7 +647,7 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
 	struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
 	struct bpf_dtab_netdev *dev, *old_dev;
 	u32 i = *(u32 *)key;
-	u32 ifindex;
+	u32 ifindex, id = 0;
 
 	if (map->value_size == 4) {
 		ifindex = *(u32 *)value;
@@ -588,6 +655,7 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
 		struct dev_map_ext_val *val = value;
 
 		ifindex = val->ifindex;
+		id = val->prog_id;
 	}
 
 	if (unlikely(map_flags > BPF_EXIST))
@@ -599,8 +667,11 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
 
 	if (!ifindex) {
 		dev = NULL;
+		/* can not specify id if ifindex is 0 */
+		if (id)
+			return -EINVAL;
 	} else {
-		dev = __dev_map_alloc_node(net, dtab, ifindex, i);
+		dev = __dev_map_alloc_node(net, dtab, ifindex, id, i);
 		if (IS_ERR(dev))
 			return PTR_ERR(dev);
 	}
@@ -630,8 +701,8 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
 	struct bpf_dtab_netdev *dev, *old_dev;
 	u32 idx = *(u32 *)key;
 	unsigned long flags;
+	u32 ifindex, id = 0;
 	int err = -EEXIST;
-	u32 ifindex;
 
 	if (map->value_size == 4) {
 		ifindex = *(u32 *)value;
@@ -639,6 +710,7 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
 		struct dev_map_ext_val *val = value;
 
 		ifindex = val->ifindex;
+		id = val->prog_id;
 	}
 
 	if (unlikely(map_flags > BPF_EXIST || !ifindex))
@@ -650,7 +722,7 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
 	if (old_dev && (map_flags & BPF_NOEXIST))
 		goto out_err;
 
-	dev = __dev_map_alloc_node(net, dtab, ifindex, idx);
+	dev = __dev_map_alloc_node(net, dtab, ifindex, id, idx);
 	if (IS_ERR(dev)) {
 		err = PTR_ERR(dev);
 		goto out_err;
diff --git a/net/core/dev.c b/net/core/dev.c
index f36bd3b21997..2571a8976cd4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5410,6 +5410,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
+		 */
+		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);
@@ -8825,6 +8837,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);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 97e1fd19ff58..8c2c0d0c9a0e 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -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
 };
 
-- 
2.21.1 (Apple Git-122.3)

[PATCH RFC bpf-next 3/4] xdp: Add xdp_txq_info to xdp_buff

[David Ahern]

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.

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 <dsahern@kernel.org>
---
 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(+)

diff --git a/include/net/xdp.h b/include/net/xdp.h
index 3094fccf5a88..fd60ab3d401f 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -65,6 +65,10 @@ 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;
@@ -72,6 +76,7 @@ struct xdp_buff {
 	void *data_hard_start;
 	unsigned long handle;
 	struct xdp_rxq_info *rxq;
+	struct xdp_txq_info *txq;
 	u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
 };
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 8c2c0d0c9a0e..264de1484a66 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -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 */
 };
 
 enum sk_action {
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index 06f4c746fa7c..240c1aeca7ef 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -471,8 +471,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:
diff --git a/net/core/filter.c b/net/core/filter.c
index bd2853d23b50..199e02a30381 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6994,6 +6994,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) {
@@ -7942,6 +7949,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;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 8c2c0d0c9a0e..264de1484a66 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -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 */
 };
 
 enum sk_action {
-- 
2.21.1 (Apple Git-122.3)

[PATCH RFC bpf-next 4/4] bpftool: Add SEC name for xdp programs attached to device map

[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 <dsahern@kernel.org>
---
 tools/lib/bpf/libbpf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fa04cbe547ed..563827b260e8 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -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),
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[Jesper Dangaard Brouer]

On Thu, 21 May 2020 19:05:22 -0600
David Ahern <dsahern@kernel.org> wrote:

> Implementation of Daniel's proposal for allowing DEVMAP entries to be
> a device index, program id pair. Daniel suggested an fd to specify the
> program, but that seems odd to me that you insert the value as an fd, but
> read it back as an id since the fd can be closed.

Great that you are working on this :-)

Lorenzo (cc lorenzo@kernel.org) is working on a similar approach for
cpumap, please coordinate/Cc him on these patches.

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

Re: [PATCH RFC bpf-next 1/4] bpf: Handle 8-byte values in DEVMAP and DEVMAP_HASH

[Jesper Dangaard Brouer]

On Thu, 21 May 2020 19:05:23 -0600
David Ahern <dsahern@kernel.org> wrote:

> Add support to DEVMAP and DEVMAP_HASH to support 8-byte values as a
> <device index, program id> pair. To do this, a new struct is needed in
> bpf_dtab_netdev to hold the values to return on lookup.

I would like to see us leverage BTF instead of checking on the size
attr->value_size. E.g do the sanity check based on BTF.
Given I don't know the exact details on how this should be done, I will
look into it... I already promised Lorenzo, as we have already
discussed this on IRC.

So, you can Lorenzo can go ahead with this approach, and test the
use-case. And I'll try to figure out if-and-how we can leverage BTF
here.  Input from BTF experts will be much appreciated.


> Signed-off-by: David Ahern <dsahern@kernel.org>
> ---
>  kernel/bpf/devmap.c | 53 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index a51d9fb7a359..2c01ce434306 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -60,12 +60,19 @@ struct xdp_dev_bulk_queue {
>  	unsigned int count;
>  };
>  
> +/* devmap value can be dev index or dev index + prog id */
> +struct dev_map_ext_val {
> +	u32 ifindex;	/* must be first for compat with 4-byte values */
> +	u32 prog_id;
> +};
> +
>  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 rcu_head rcu;
>  	unsigned int idx;
> +	struct dev_map_ext_val val;
>  };
>  
>  struct bpf_dtab {
> @@ -108,9 +115,13 @@ static int dev_map_init_map(struct bpf_dtab *dtab, union bpf_attr *attr)
>  	u64 cost = 0;
>  	int err;
>  
> -	/* check sanity of attributes */
> +	/* check sanity of attributes. 2 value sizes supported:
> +	 * 4 bytes: ifindex
> +	 * 8 bytes: ifindex + prog id
> +	 */
>  	if (attr->max_entries == 0 || attr->key_size != 4 ||
> -	    attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
> +	    (attr->value_size != 4 && attr->value_size != 8) ||
> +	    attr->map_flags & ~DEV_CREATE_FLAG_MASK)
>  		return -EINVAL;
>  
>  	/* Lookup returns a pointer straight to dev->ifindex, so make sure the
[...]

>  static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
> @@ -568,8 +579,16 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
>  {
>  	struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
>  	struct bpf_dtab_netdev *dev, *old_dev;
> -	u32 ifindex = *(u32 *)value;
>  	u32 i = *(u32 *)key;
> +	u32 ifindex;
> +
> +	if (map->value_size == 4) {
> +		ifindex = *(u32 *)value;
> +	} else {
> +		struct dev_map_ext_val *val = value;
> +
> +		ifindex = val->ifindex;
> +	}
>  
>  	if (unlikely(map_flags > BPF_EXIST))
>  		return -EINVAL;
> @@ -609,10 +628,18 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
>  {
>  	struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
>  	struct bpf_dtab_netdev *dev, *old_dev;
> -	u32 ifindex = *(u32 *)value;
>  	u32 idx = *(u32 *)key;
>  	unsigned long flags;
>  	int err = -EEXIST;
> +	u32 ifindex;
> +
> +	if (map->value_size == 4) {
> +		ifindex = *(u32 *)value;
> +	} else {
> +		struct dev_map_ext_val *val = value;
> +
> +		ifindex = val->ifindex;
> +	}
>  
>  	if (unlikely(map_flags > BPF_EXIST || !ifindex))
>  		return -EINVAL;



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

Re: [PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[Toke Høiland-Jørgensen]

David Ahern <dsahern@kernel.org> writes:

> Implementation of Daniel's proposal for allowing DEVMAP entries to be
> a device index, program id pair. Daniel suggested an fd to specify the
> program, but that seems odd to me that you insert the value as an fd, but
> read it back as an id since the fd can be closed.

While I can be sympathetic to the argument that it seems odd, every
other API uses FD for insert and returns ID, so why make it different
here? Also, the choice has privilege implications, since the CAP_BPF
series explicitly makes going from ID->FD a more privileged operation
than just querying the ID.

-Toke

Re: [PATCH RFC bpf-next 2/4] bpf: Add support to attach bpf program to a devmap

[Toke Høiland-Jørgensen]

David Ahern <dsahern@kernel.org> writes:

> Add BPF_XDP_DEVMAP attach type for use with programs associated with a
> DEVMAP.
>
> DEVMAPs can associate a program with a device entry by setting the
> value to <index,id> pair. The program associated with the id 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
> 8-bytes values.
>
> Signed-off-by: David Ahern <dsahern@kernel.org>
> ---
>  include/linux/bpf.h            |  5 +++
>  include/uapi/linux/bpf.h       |  1 +
>  kernel/bpf/devmap.c            | 82 +++++++++++++++++++++++++++++++---
>  net/core/dev.c                 | 18 ++++++++
>  tools/include/uapi/linux/bpf.h |  1 +
>  5 files changed, 102 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index efe8836b5c48..088751bc09aa 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -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)
>  {
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 97e1fd19ff58..8c2c0d0c9a0e 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -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
>  };
>  
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 2c01ce434306..06f4c746fa7c 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -70,6 +70,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 dev_map_ext_val val;
> @@ -228,6 +229,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);
>  			}
> @@ -242,6 +245,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);
>  		}
> @@ -328,6 +333,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 == 8)
> +		return true;
> +
> +	return false;
> +}
> +
>  static int bq_xmit_all(struct xdp_dev_bulk_queue *bq, u32 flags)
>  {
>  	struct net_device *dev = bq->dev;
> @@ -452,6 +467,39 @@ static inline int __xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp,
>  	return bq_enqueue(dev, xdpf, dev_rx);
>  }
>  
> +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;
> +	case XDP_PASS:
> +		break;
> +	case XDP_TX:
> +		fallthrough;
> +	case XDP_REDIRECT:
> +		fallthrough;
> +	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;
> +	}
> +
> +	return xdp;
> +}
> +
>  int dev_xdp_enqueue(struct net_device *dev, struct xdp_buff *xdp,
>  		    struct net_device *dev_rx)
>  {
> @@ -463,6 +511,11 @@ int dev_map_enqueue(struct bpf_dtab_netdev *dst, struct xdp_buff *xdp,
>  {
>  	struct net_device *dev = dst->dev;
>  
> +	if (dst->xdp_prog) {
> +		xdp = dev_map_run_prog(dev, xdp, dst->xdp_prog);
> +		if (!xdp)
> +			return 0;
> +	}
>  	return __xdp_enqueue(dev, xdp, dev_rx);
>  }
>  
> @@ -499,6 +552,8 @@ static void __dev_map_entry_free(struct rcu_head *rcu)
>  	struct bpf_dtab_netdev *dev;
>  
>  	dev = container_of(rcu, struct bpf_dtab_netdev, rcu);
> +	if (dev->xdp_prog)
> +		bpf_prog_put(dev->xdp_prog);
>  	dev_put(dev->dev);
>  	kfree(dev);
>  }
> @@ -549,9 +604,10 @@ static int dev_map_hash_delete_elem(struct bpf_map *map, void *key)
>  
>  static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
>  						    struct bpf_dtab *dtab,
> -						    u32 ifindex,
> +						    u32 ifindex, u32 prog_id,
>  						    unsigned int idx)
>  {
> +	struct bpf_prog *prog = NULL;
>  	struct bpf_dtab_netdev *dev;
>  
>  	dev = kmalloc_node(sizeof(*dev), GFP_ATOMIC | __GFP_NOWARN,
> @@ -563,12 +619,23 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
>  	if (!dev->dev)
>  		goto err_out;
>  
> +	if (prog_id) {
> +		prog = bpf_prog_by_id(prog_id);
> +		if (IS_ERR(prog) || prog->type != BPF_PROG_TYPE_XDP ||
> +		    prog->expected_attach_type != BPF_XDP_DEVMAP)

If the second or third condition fails here you never bpf_prog_put() the
prog ref.

> +			goto err_dev;
> +	}
> +
>  	dev->idx = idx;
>  	dev->dtab = dtab;
>  
> +	dev->xdp_prog = prog;
>  	dev->val.ifindex = ifindex;
> +	dev->val.prog_id = prog_id;
>  
>  	return dev;
> +err_dev:
> +	dev_put(dev->dev);
>  err_out:
>  	kfree(dev);
>  	return ERR_PTR(-EINVAL);
> @@ -580,7 +647,7 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
>  	struct bpf_dtab *dtab = container_of(map, struct bpf_dtab, map);
>  	struct bpf_dtab_netdev *dev, *old_dev;
>  	u32 i = *(u32 *)key;
> -	u32 ifindex;
> +	u32 ifindex, id = 0;
>  
>  	if (map->value_size == 4) {
>  		ifindex = *(u32 *)value;
> @@ -588,6 +655,7 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
>  		struct dev_map_ext_val *val = value;
>  
>  		ifindex = val->ifindex;
> +		id = val->prog_id;
>  	}
>  
>  	if (unlikely(map_flags > BPF_EXIST))
> @@ -599,8 +667,11 @@ static int __dev_map_update_elem(struct net *net, struct bpf_map *map,
>  
>  	if (!ifindex) {
>  		dev = NULL;
> +		/* can not specify id if ifindex is 0 */
> +		if (id)
> +			return -EINVAL;
>  	} else {
> -		dev = __dev_map_alloc_node(net, dtab, ifindex, i);
> +		dev = __dev_map_alloc_node(net, dtab, ifindex, id, i);
>  		if (IS_ERR(dev))
>  			return PTR_ERR(dev);
>  	}
> @@ -630,8 +701,8 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
>  	struct bpf_dtab_netdev *dev, *old_dev;
>  	u32 idx = *(u32 *)key;
>  	unsigned long flags;
> +	u32 ifindex, id = 0;
>  	int err = -EEXIST;
> -	u32 ifindex;
>  
>  	if (map->value_size == 4) {
>  		ifindex = *(u32 *)value;
> @@ -639,6 +710,7 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
>  		struct dev_map_ext_val *val = value;
>  
>  		ifindex = val->ifindex;
> +		id = val->prog_id;
>  	}
>  
>  	if (unlikely(map_flags > BPF_EXIST || !ifindex))
> @@ -650,7 +722,7 @@ static int __dev_map_hash_update_elem(struct net *net, struct bpf_map *map,
>  	if (old_dev && (map_flags & BPF_NOEXIST))
>  		goto out_err;
>  
> -	dev = __dev_map_alloc_node(net, dtab, ifindex, idx);
> +	dev = __dev_map_alloc_node(net, dtab, ifindex, id, idx);
>  	if (IS_ERR(dev)) {
>  		err = PTR_ERR(dev);
>  		goto out_err;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index f36bd3b21997..2571a8976cd4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5410,6 +5410,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
> +		 */
> +		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);
> @@ -8825,6 +8837,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);
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 97e1fd19ff58..8c2c0d0c9a0e 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -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
>  };
>  
> -- 
> 2.21.1 (Apple Git-122.3)

Re: [PATCH RFC bpf-next 3/4] xdp: Add xdp_txq_info to xdp_buff

[Toke Høiland-Jørgensen]

David Ahern <dsahern@kernel.org> writes:

> 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.
>
> 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 <dsahern@kernel.org>

Nit: seems a bit odd to order the patches like this, I'd have expected
this patch to go before #2.

-Toke

Re: [PATCH RFC bpf-next 1/4] bpf: Handle 8-byte values in DEVMAP and DEVMAP_HASH

[Jesper Dangaard Brouer]

On Fri, 22 May 2020 14:08:05 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> On Thu, 21 May 2020 19:05:23 -0600
> David Ahern <dsahern@kernel.org> wrote:
> 
> > Add support to DEVMAP and DEVMAP_HASH to support 8-byte values as a
> > <device index, program id> pair. To do this, a new struct is needed in
> > bpf_dtab_netdev to hold the values to return on lookup.  
> 
> I would like to see us leverage BTF instead of checking on the size
> attr->value_size. E.g do the sanity check based on BTF.
> Given I don't know the exact details on how this should be done, I will
> look into it... I already promised Lorenzo, as we have already
> discussed this on IRC.
> 
> So, you can Lorenzo can go ahead with this approach, and test the
> use-case. And I'll try to figure out if-and-how we can leverage BTF
> here.  Input from BTF experts will be much appreciated.

Published my current notes here:
 https://github.com/xdp-project/xdp-project/blob/BTF01-notes.public/areas/core/BTF_01_notes.org

And created PR that people can GitHub "subscribe" to, if you are interested:
 https://github.com/xdp-project/xdp-project/pull/36

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

Re: [PATCH RFC bpf-next 2/4] bpf: Add support to attach bpf program to a devmap

[David Ahern]

On 5/22/20 10:02 AM, Toke Høiland-Jørgensen wrote:
>> @@ -563,12 +619,23 @@ static struct bpf_dtab_netdev *__dev_map_alloc_node(struct net *net,
>>  	if (!dev->dev)
>>  		goto err_out;
>>  
>> +	if (prog_id) {
>> +		prog = bpf_prog_by_id(prog_id);
>> +		if (IS_ERR(prog) || prog->type != BPF_PROG_TYPE_XDP ||
>> +		    prog->expected_attach_type != BPF_XDP_DEVMAP)
> 
> If the second or third condition fails here you never bpf_prog_put() the
> prog ref.
> 

good catch. Thanks, will fix.

Re: [PATCH RFC bpf-next 3/4] xdp: Add xdp_txq_info to xdp_buff

[David Ahern]

On 5/22/20 10:04 AM, Toke Høiland-Jørgensen wrote:
> David Ahern <dsahern@kernel.org> writes:
> 
>> 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.
>>
>> 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 <dsahern@kernel.org>
> 
> Nit: seems a bit odd to order the patches like this, I'd have expected
> this patch to go before #2.
> 

This patch depends on patch 2, so it needs to follow it.

Re: [PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[David Ahern]

On 5/22/20 9:59 AM, Toke Høiland-Jørgensen wrote:
> David Ahern <dsahern@kernel.org> writes:
> 
>> Implementation of Daniel's proposal for allowing DEVMAP entries to be
>> a device index, program id pair. Daniel suggested an fd to specify the
>> program, but that seems odd to me that you insert the value as an fd, but
>> read it back as an id since the fd can be closed.
> 
> While I can be sympathetic to the argument that it seems odd, every
> other API uses FD for insert and returns ID, so why make it different
> here? Also, the choice has privilege implications, since the CAP_BPF
> series explicitly makes going from ID->FD a more privileged operation
> than just querying the ID.
> 

I do not like the model where the kernel changes the value the user
pushed down.

Re: [PATCH RFC bpf-next 1/4] bpf: Handle 8-byte values in DEVMAP and DEVMAP_HASH

[David Ahern]

On 5/22/20 10:04 AM, Jesper Dangaard Brouer wrote:
> On Fri, 22 May 2020 14:08:05 +0200
> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> 
>> On Thu, 21 May 2020 19:05:23 -0600
>> David Ahern <dsahern@kernel.org> wrote:
>>
>>> Add support to DEVMAP and DEVMAP_HASH to support 8-byte values as a
>>> <device index, program id> pair. To do this, a new struct is needed in
>>> bpf_dtab_netdev to hold the values to return on lookup.  
>>
>> I would like to see us leverage BTF instead of checking on the size
>> attr->value_size. E.g do the sanity check based on BTF.
>> Given I don't know the exact details on how this should be done, I will
>> look into it... I already promised Lorenzo, as we have already
>> discussed this on IRC.
>>
>> So, you can Lorenzo can go ahead with this approach, and test the
>> use-case. And I'll try to figure out if-and-how we can leverage BTF
>> here.  Input from BTF experts will be much appreciated.
> 
> Published my current notes here:
>  https://github.com/xdp-project/xdp-project/blob/BTF01-notes.public/areas/core/BTF_01_notes.org
> 
> And created PR that people can GitHub "subscribe" to, if you are interested:
>  https://github.com/xdp-project/xdp-project/pull/36
> 

thanks for compiling some notes.

Fundamentally, I do not see how this can work for something like the
program id where the kernel needs to know not just the type (u32) but
that it should take the id and do a lookup - convert the id into a
bpf_prog reference.

Re: [PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[Toke Høiland-Jørgensen]

David Ahern <dsahern@gmail.com> writes:

> On 5/22/20 9:59 AM, Toke Høiland-Jørgensen wrote:
>> David Ahern <dsahern@kernel.org> writes:
>> 
>>> Implementation of Daniel's proposal for allowing DEVMAP entries to be
>>> a device index, program id pair. Daniel suggested an fd to specify the
>>> program, but that seems odd to me that you insert the value as an fd, but
>>> read it back as an id since the fd can be closed.
>> 
>> While I can be sympathetic to the argument that it seems odd, every
>> other API uses FD for insert and returns ID, so why make it different
>> here? Also, the choice has privilege implications, since the CAP_BPF
>> series explicitly makes going from ID->FD a more privileged operation
>> than just querying the ID.
>> 
>
> I do not like the model where the kernel changes the value the user
> pushed down.

Yet it's what we do in every other interface where a user needs to
supply a program, including in prog array maps. So let's not create a
new inconsistent interface here...

-Toke

Re: [PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[Jesper Dangaard Brouer]

On Mon, 25 May 2020 14:15:32 +0200
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> David Ahern <dsahern@gmail.com> writes:
> 
> > On 5/22/20 9:59 AM, Toke Høiland-Jørgensen wrote:  
> >> David Ahern <dsahern@kernel.org> writes:
> >>   
> >>> Implementation of Daniel's proposal for allowing DEVMAP entries to be
> >>> a device index, program id pair. Daniel suggested an fd to specify the
> >>> program, but that seems odd to me that you insert the value as an fd, but
> >>> read it back as an id since the fd can be closed.  
> >> 
> >> While I can be sympathetic to the argument that it seems odd, every
> >> other API uses FD for insert and returns ID, so why make it different
> >> here? Also, the choice has privilege implications, since the CAP_BPF
> >> series explicitly makes going from ID->FD a more privileged operation
> >> than just querying the ID.

Sorry, I don't follow.
Can someone explain why is inserting an ID is a privilege problem?

   
> >
> > I do not like the model where the kernel changes the value the user
> > pushed down.  
> 
> Yet it's what we do in every other interface where a user needs to
> supply a program, including in prog array maps. So let's not create a
> new inconsistent interface here...

I sympathize with Ahern on this.  It seems very weird to insert/write
one value-type, but read another value-type.

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

Re: [PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[Toke Høiland-Jørgensen]

Jesper Dangaard Brouer <brouer@redhat.com> writes:

> On Mon, 25 May 2020 14:15:32 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>> David Ahern <dsahern@gmail.com> writes:
>> 
>> > On 5/22/20 9:59 AM, Toke Høiland-Jørgensen wrote:  
>> >> David Ahern <dsahern@kernel.org> writes:
>> >>   
>> >>> Implementation of Daniel's proposal for allowing DEVMAP entries to be
>> >>> a device index, program id pair. Daniel suggested an fd to specify the
>> >>> program, but that seems odd to me that you insert the value as an fd, but
>> >>> read it back as an id since the fd can be closed.  
>> >> 
>> >> While I can be sympathetic to the argument that it seems odd, every
>> >> other API uses FD for insert and returns ID, so why make it different
>> >> here? Also, the choice has privilege implications, since the CAP_BPF
>> >> series explicitly makes going from ID->FD a more privileged operation
>> >> than just querying the ID.
>
> Sorry, I don't follow.
> Can someone explain why is inserting an ID is a privilege problem?

See description here:
https://lore.kernel.org/bpf/20200513230355.7858-1-alexei.starovoitov@gmail.com/

Specifically, this part:

> Consolidating all CAP checks at load time makes security model similar to
> open() syscall. Once the user got an FD it can do everything with it.
> read/write/poll don't check permissions. The same way when bpf_prog_load
> command returns an FD the user can do everything (including attaching,
> detaching, and bpf_test_run).
> 
> The important design decision is to allow ID->FD transition for
> CAP_SYS_ADMIN only. What it means that user processes can run
> with CAP_BPF and CAP_NET_ADMIN and they will not be able to affect each
> other unless they pass FDs via scm_rights or via pinning in bpffs.


>> > I do not like the model where the kernel changes the value the user
>> > pushed down.  
>> 
>> Yet it's what we do in every other interface where a user needs to
>> supply a program, including in prog array maps. So let's not create a
>> new inconsistent interface here...
>
> I sympathize with Ahern on this.  It seems very weird to insert/write
> one value-type, but read another value-type.

Yeah, I do kinda agree that it's a bit weird. But it's what we do
everywhere else, so I think consistency wins out here. There might be an
argument that maps should be different (because they're conceptually a
read/write data structure not a syscall return value). But again, we
already have a map type that takes prog IDs, and that already does the
rewriting, so doing it different here would be even weirder...

-Toke

Re: [PATCH RFC bpf-next 0/4] bpf: Add support for XDP programs in DEVMAPs

[Daniel Borkmann]

On 5/25/20 2:56 PM, Toke Høiland-Jørgensen wrote:
> Jesper Dangaard Brouer <brouer@redhat.com> writes:
>> On Mon, 25 May 2020 14:15:32 +0200
>> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>> David Ahern <dsahern@gmail.com> writes:
>>>> On 5/22/20 9:59 AM, Toke Høiland-Jørgensen wrote:
>>>>> David Ahern <dsahern@kernel.org> writes:
>>>>>    
>>>>>> Implementation of Daniel's proposal for allowing DEVMAP entries to be
>>>>>> a device index, program id pair. Daniel suggested an fd to specify the
>>>>>> program, but that seems odd to me that you insert the value as an fd, but
>>>>>> read it back as an id since the fd can be closed.
>>>>>
>>>>> While I can be sympathetic to the argument that it seems odd, every
>>>>> other API uses FD for insert and returns ID, so why make it different
>>>>> here? Also, the choice has privilege implications, since the CAP_BPF
>>>>> series explicitly makes going from ID->FD a more privileged operation
>>>>> than just querying the ID.

[...]

>> I sympathize with Ahern on this.  It seems very weird to insert/write
>> one value-type, but read another value-type.
> 
> Yeah, I do kinda agree that it's a bit weird. But it's what we do
> everywhere else, so I think consistency wins out here. There might be an
> argument that maps should be different (because they're conceptually a
> read/write data structure not a syscall return value). But again, we
> already have a map type that takes prog IDs, and that already does the
> rewriting, so doing it different here would be even weirder...

Sorry for the late reply. Agree, it would at least be consistent to what is done
in tail call maps, and the XDP netlink API where you have the fd->id in both cases.
Either way, quick glance over the patches, the direction of this RFC looks good to
me, better fit than the prior XDP egress approaches.

Thanks,
Daniel