On 5/28/20 1:01 AM, Andrii Nakryiko wrote: > On Wed, May 27, 2020 at 5:15 PM David Ahern <dsah...@kernel.org> wrote: >> >> From: David Ahern <dsah...@gmail.com> >> >> 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 <dsah...@gmail.com> >> --- >> 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. > >> 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. > >> + attr->map_flags & ~DEV_CREATE_FLAG_MASK) >> return -EINVAL; >> > > [...] >