Date:   Sat, 15 Dec 2018 16:43:50 +0000
From:   Martin Lau <kafai@...com>
To:     Yonghong Song <yhs@...com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 3/8] bpf: enable cgroup local storage map
 pretty print with kind_flag

On Fri, Dec 14, 2018 at 03:34:28PM -0800, Yonghong Song wrote:
> Commit 970289fc0a83 ("bpf: add bpffs pretty print for cgroup
> local storage maps") added bpffs pretty print for cgroup
> local storage maps. The commit worked for struct without kind_flag
> set.
> 
> This patch refactored and made pretty print also work
> with kind_flag set for the struct.
> 
> Signed-off-by: Yonghong Song <yhs@...com>
> ---
>  include/linux/btf.h        |  5 ++++-
>  kernel/bpf/btf.c           | 37 ++++++++++++++++++++++++++++---------
>  kernel/bpf/local_storage.c | 17 ++++-------------
>  3 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 58000d7e06e3..12502e25e767 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -7,6 +7,7 @@
>  #include <linux/types.h>
>  
>  struct btf;
> +struct btf_member;
>  struct btf_type;
>  union bpf_attr;
>  
> @@ -46,7 +47,9 @@ void btf_type_seq_show(const struct btf *btf, u32 type_id, void *obj,
>  		       struct seq_file *m);
>  int btf_get_fd_by_id(u32 id);
>  u32 btf_id(const struct btf *btf);
> -bool btf_type_is_reg_int(const struct btf_type *t, u32 expected_size);
> +bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
> +			   const struct btf_member *m,
> +			   u32 expected_offset, u32 expected_size);
>  
>  #ifdef CONFIG_BPF_SYSCALL
>  const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index ec3f80d3bef6..30028fa187df 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -546,22 +546,41 @@ static bool btf_type_int_is_regular(const struct btf_type *t)
>  }
>  
>  /*
> - * Check that given type is a regular int and has the expected size.
> + * Check that given struct member is a regular int with expected
> + * offset and size.
>   */
> -bool btf_type_is_reg_int(const struct btf_type *t, u32 expected_size)
> +bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
> +			   const struct btf_member *m,
> +			   u32 expected_offset, u32 expected_size)
>  {
> -	u8 nr_bits, nr_bytes;
> -	u32 int_data;
> +	const struct btf_type *t;
> +	u32 id, int_data;
> +	u8 nr_bits;
>  
> -	if (!btf_type_is_int(t))
> +	id = m->type;
> +	t = btf_type_id_size(btf, &id, NULL);
> +	if (!t || !btf_type_is_int(t))
>  		return false;
>  
>  	int_data = btf_type_int(t);
>  	nr_bits = BTF_INT_BITS(int_data);
> -	nr_bytes = BITS_ROUNDUP_BYTES(nr_bits);
> -	if (BITS_PER_BYTE_MASKED(nr_bits) ||
> -	    BTF_INT_OFFSET(int_data) ||
> -	    nr_bytes != expected_size)
> +	if (btf_type_kflag(s)) {
> +		u32 bitfield_size = BTF_MEMBER_BITFIELD_SIZE(m->offset);
> +		u32 bit_offset = BTF_MEMBER_BIT_OFFSET(m->offset);
> +
> +		/* if kflag set, int should be a regular int and
> +		 * bit offset should be at byte boundary.
> +		 */
> +		return !bitfield_size &&
Make sense.  When kflag == true, by design, the member is a reg int only if
bitfield_size == 0.  The BTF verifier has already ensured everything else.

Acked-by: Martin KaFai Lau <kafai@...com>

> +		       BITS_ROUNDUP_BYTES(bit_offset) == expected_offset &&
> +		       BITS_ROUNDUP_BYTES(nr_bits) == expected_size;
> +	}
> +
> +	if (BTF_INT_OFFSET(int_data) ||
> +	    BITS_PER_BYTE_MASKED(m->offset) ||
> +	    BITS_ROUNDUP_BYTES(m->offset) != expected_offset ||
> +	    BITS_PER_BYTE_MASKED(nr_bits) ||
> +	    BITS_ROUNDUP_BYTES(nr_bits) != expected_size)
>  		return false;
>  
>  	return true;
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 5eca03da0989..07a34ef562a0 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -315,9 +315,8 @@ static int cgroup_storage_check_btf(const struct bpf_map *map,
>  				    const struct btf_type *key_type,
>  				    const struct btf_type *value_type)
>  {
> -	const struct btf_type *t;
>  	struct btf_member *m;
> -	u32 id, size;
> +	u32 offset, size;
>  
>  	/* Key is expected to be of struct bpf_cgroup_storage_key type,
>  	 * which is:
> @@ -338,25 +337,17 @@ static int cgroup_storage_check_btf(const struct bpf_map *map,
>  	 * The first field must be a 64 bit integer at 0 offset.
>  	 */
>  	m = (struct btf_member *)(key_type + 1);
> -	if (m->offset)
> -		return -EINVAL;
> -	id = m->type;
> -	t = btf_type_id_size(btf, &id, NULL);
>  	size = FIELD_SIZEOF(struct bpf_cgroup_storage_key, cgroup_inode_id);
> -	if (!t || !btf_type_is_reg_int(t, size))
> +	if (!btf_member_is_reg_int(btf, key_type, m, 0, size))
>  		return -EINVAL;
>  
>  	/*
>  	 * The second field must be a 32 bit integer at 64 bit offset.
>  	 */
>  	m++;
> -	if (m->offset != offsetof(struct bpf_cgroup_storage_key, attach_type) *
> -	    BITS_PER_BYTE)
> -		return -EINVAL;
> -	id = m->type;
> -	t = btf_type_id_size(btf, &id, NULL);
> +	offset = offsetof(struct bpf_cgroup_storage_key, attach_type);
>  	size = FIELD_SIZEOF(struct bpf_cgroup_storage_key, attach_type);
> -	if (!t || !btf_type_is_reg_int(t, size))
> +	if (!btf_member_is_reg_int(btf, key_type, m, offset, size))
>  		return -EINVAL;
>  
>  	return 0;
> -- 
> 2.17.1
> 

Powered by blists - more mailing lists