[RFC,6/9] bpf: split map modification structs

Commit Message

Lorenz Bauer Oct. 14, 2021, 2:34 p.m. UTC

---
 include/uapi/linux/bpf.h | 51 +++++++++++++++++++++++------
 kernel/bpf/syscall.c     | 70 ++++++++++++++++------------------------
 2 files changed, 70 insertions(+), 51 deletions(-)

Comments

Alexei Starovoitov Oct. 20, 2021, 5:13 p.m. UTC | #1

On Thu, Oct 14, 2021 at 03:34:31PM +0100, Lorenz Bauer wrote:
> ---
>  include/uapi/linux/bpf.h | 51 +++++++++++++++++++++++------
>  kernel/bpf/syscall.c     | 70 ++++++++++++++++------------------------
>  2 files changed, 70 insertions(+), 51 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f1c163778d7a..d3acd12d98c1 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1294,18 +1294,41 @@ struct bpf_map_create_attr {
>  						 */
>  };
>  
> +struct bpf_map_lookup_elem_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +	__bpf_md_ptr(void *, value);
> +	__u64 flags;
> +};
> +
> +struct bpf_map_update_elem_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +	__bpf_md_ptr(void *, value);
> +	__u64 flags;
> +};
> +
> +struct bpf_map_delete_elem_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +};
> +
> +struct bpf_map_get_next_key_attr {
> +	__u32 map_fd;
> +	__bpf_md_ptr(const void *, key);
> +	__bpf_md_ptr(void *, next_key);
> +};
> +
>  union bpf_attr {
>  	struct bpf_map_create_attr map_create;
>  
> -	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
> -		__u32		map_fd;
> -		__aligned_u64	key;
> -		union {
> -			__aligned_u64 value;
> -			__aligned_u64 next_key;
> -		};
> -		__u64		flags;
> -	};
> +	struct bpf_map_lookup_elem_attr map_lookup_elem;
> +
> +	struct bpf_map_update_elem_attr map_update_elem;
> +
> +	struct bpf_map_delete_elem_attr map_delete_elem;
> +
> +	struct bpf_map_get_next_key_attr map_get_next_key;

I think such additions to bpf_attr are good. They do make it
the interface more obvious.
But please don't move or delete the existing anon struct.
Adding 'deprecated' comment won't make it deprecated.
It will be used regardless.
If we ever extend 'map_lookup' command we might even add
fields to both. tbd.

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f1c163778d7a..d3acd12d98c1 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1294,18 +1294,41 @@  struct bpf_map_create_attr {
 						 */
 };
 
+struct bpf_map_lookup_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_update_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, value);
+	__u64 flags;
+};
+
+struct bpf_map_delete_elem_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+};
+
+struct bpf_map_get_next_key_attr {
+	__u32 map_fd;
+	__bpf_md_ptr(const void *, key);
+	__bpf_md_ptr(void *, next_key);
+};
+
 union bpf_attr {
 	struct bpf_map_create_attr map_create;
 
-	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
-		__u32		map_fd;
-		__aligned_u64	key;
-		union {
-			__aligned_u64 value;
-			__aligned_u64 next_key;
-		};
-		__u64		flags;
-	};
+	struct bpf_map_lookup_elem_attr map_lookup_elem;
+
+	struct bpf_map_update_elem_attr map_update_elem;
+
+	struct bpf_map_delete_elem_attr map_delete_elem;
+
+	struct bpf_map_get_next_key_attr map_get_next_key;
 
 	struct { /* struct used by BPF_MAP_*_BATCH commands */
 		__aligned_u64	in_batch;	/* start batch,
@@ -1524,6 +1547,16 @@  union bpf_attr {
 		__u32	btf_value_type_id;
 		__u32	btf_vmlinux_value_type_id;
 	};
+
+	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
+		__u32		map_fd;
+		__aligned_u64	key;
+		union {
+			__aligned_u64 value;
+			__aligned_u64 next_key;
+		};
+		__u64		flags;
+	};
 } __attribute__((aligned(8)));
 
 /* The description below is an attempt at providing documentation to eBPF
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index f7b57877acd2..c4aecdbb390e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -728,6 +728,11 @@  int bpf_get_file_flag(int flags)
 		__tmp; \
 	})
 
+#define CHECK_ATTR_TAIL(attr, field) \
+	(memchr_inv((void *)(attr) + offsetofend(typeof(*attr), field), 0, \
+		    sizeof(*(attr)) - offsetofend(typeof(*attr), field)) != NULL ? -EINVAL : 0)
+
+
 /* dst and src must have at least "size" number of bytes.
  * Return strlen on success and < 0 on error.
  */
@@ -1041,23 +1046,17 @@  static void *___bpf_copy_key(bpfptr_t ukey, u64 key_size)
 	return NULL;
 }
 
-/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_LOOKUP_ELEM_LAST_FIELD flags
-
-static int map_lookup_elem(union bpf_attr *attr)
+static int map_lookup_elem(struct bpf_map_lookup_elem_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	void __user *uvalue = u64_to_user_ptr(attr->value);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	void __user *uvalue = u64_to_user_ptr(attr->value_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *value;
 	u32 value_size;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_LOOKUP_ELEM))
-		return -EINVAL;
-
 	if (attr->flags & ~BPF_F_LOCK)
 		return -EINVAL;
 
@@ -1108,23 +1107,17 @@  static int map_lookup_elem(union bpf_attr *attr)
 	return err;
 }
 
-
-#define BPF_MAP_UPDATE_ELEM_LAST_FIELD flags
-
-static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
+static int map_update_elem(struct bpf_map_update_elem_attr *attr, bpfptr_t uattr)
 {
-	bpfptr_t ukey = make_bpfptr(attr->key, uattr.is_kernel);
-	bpfptr_t uvalue = make_bpfptr(attr->value, uattr.is_kernel);
-	int ufd = attr->map_fd;
+	bpfptr_t ukey = make_bpfptr(attr->key_u64, uattr.is_kernel);
+	bpfptr_t uvalue = make_bpfptr(attr->value_u64, uattr.is_kernel);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *value;
 	u32 value_size;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_UPDATE_ELEM))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -1168,20 +1161,15 @@  static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
 	return err;
 }
 
-#define BPF_MAP_DELETE_ELEM_LAST_FIELD key
-
-static int map_delete_elem(union bpf_attr *attr)
+static int map_delete_elem(struct bpf_map_delete_elem_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	struct fd f;
 	void *key;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_DELETE_ELEM))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -1220,22 +1208,16 @@  static int map_delete_elem(union bpf_attr *attr)
 	return err;
 }
 
-/* last field in 'union bpf_attr' used by this command */
-#define BPF_MAP_GET_NEXT_KEY_LAST_FIELD next_key
-
-static int map_get_next_key(union bpf_attr *attr)
+static int map_get_next_key(struct bpf_map_get_next_key_attr *attr)
 {
-	void __user *ukey = u64_to_user_ptr(attr->key);
-	void __user *unext_key = u64_to_user_ptr(attr->next_key);
-	int ufd = attr->map_fd;
+	void __user *ukey = u64_to_user_ptr(attr->key_u64);
+	void __user *unext_key = u64_to_user_ptr(attr->next_key_u64);
+	int ufd = attr->fd;
 	struct bpf_map *map;
 	void *key, *next_key;
 	struct fd f;
 	int err;
 
-	if (CHECK_ATTR(BPF_MAP_GET_NEXT_KEY))
-		return -EINVAL;
-
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
@@ -4578,16 +4560,20 @@  static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 		err = map_create(ATTR_FIELD(&attr, map_create));
 		break;
 	case BPF_MAP_LOOKUP_ELEM:
-		err = map_lookup_elem(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_lookup_elem);
+		err = err ?: map_lookup_elem(&attr.map_lookup_elem);
 		break;
 	case BPF_MAP_UPDATE_ELEM:
-		err = map_update_elem(&attr, uattr);
+		err = CHECK_ATTR_TAIL(&attr, map_update_elem);
+		err = err ?: map_update_elem(&attr.map_update_elem, uattr);
 		break;
 	case BPF_MAP_DELETE_ELEM:
-		err = map_delete_elem(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_delete_elem);
+		err = err ?: map_delete_elem(&attr.map_delete_elem);
 		break;
 	case BPF_MAP_GET_NEXT_KEY:
-		err = map_get_next_key(&attr);
+		err = CHECK_ATTR_TAIL(&attr, map_get_next_key);
+		err = err ?: map_get_next_key(&attr.map_get_next_key);
 		break;
 	case BPF_MAP_FREEZE:
 		err = map_freeze(&attr);