On 10/04/2017 08:29 PM, Chenbo Feng wrote:
From: Chenbo Feng <fengc@xxxxxxxxxx> Introduce the map read/write flags to the eBPF syscalls that returns the map fd. The flags is used to set up the file mode when construct a new file descriptor for bpf maps. To not break the backward capability, the f_flags is set to O_RDWR if the flag passed by syscall is 0. Otherwise it should be O_RDONLY or O_WRONLY. When the userspace want to modify or read the map content, it will check the file mode to see if it is allowed to make the change.
[...]
+int bpf_get_file_flag(int flags) +{ + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY)) + return -EINVAL; + if (flags & BPF_F_RDONLY) + return O_RDONLY; + if (flags & BPF_F_WRONLY) + return O_WRONLY; + return O_RDWR; } /* helper macro to check that unused fields 'union bpf_attr' are zero */ @@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr) { int numa_node = bpf_map_attr_numa_node(attr); struct bpf_map *map; + int f_flags; int err; err = CHECK_ATTR(BPF_MAP_CREATE); if (err) return -EINVAL; + f_flags = bpf_get_file_flag(attr->map_flags); + if (f_flags < 0) + return f_flags;
Wait, I just noticed, given you add BPF_F_RDONLY/BPF_F_WRONLY to attr->map_flags, and later go into find_and_alloc_map(), for map alloc, which is e.g. array_map_alloc(). There, we bail out with EINVAL on attr->map_flags & ~BPF_F_NUMA_NODE, which is the case for both BPF_F_RDONLY/BPF_F_WRONLY ... I would have expected that the entire code was tested properly; what was tested exactly in the set?
if (numa_node != NUMA_NO_NODE && ((unsigned int)numa_node >= nr_node_ids || !node_online(numa_node))) @@ -376,7 +412,7 @@ static int map_create(union bpf_attr *attr) if (err) goto free_map; - err = bpf_map_new_fd(map); + err = bpf_map_new_fd(map, f_flags); if (err < 0) { /* failed to allocate fd. * bpf_map_put() is needed because the above @@ -491,6 +527,11 @@ static int map_lookup_elem(union bpf_attr *attr)
[...]