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

On 5/28/20 1:01 AM, Andrii Nakryiko wrote:

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

quoted

From: David Ahern <redacted>

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 <redacted>
---
 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(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 54b93f8b49b8..d27302ecaa9c 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -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?

sure, added 'bpf_' to the name.

quoted

 enum sk_action {
        SK_DROP = 0,
        SK_PASS,

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index a51d9fb7a359..069a50113e26 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c

@@ -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)

        /* 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

+           attr->map_flags & ~DEV_CREATE_FLAG_MASK)
                return -EINVAL;

[...]