Re: [PATCH bpf-next v5 1/4] bpf: add function for XDP meta data length check

[Yonghong Song <yhs@fb.com>]

On 6/16/21 3:47 PM, Zvi Effron wrote:
> This commit prepares to use the XDP meta data length check in multiple
> places by making it into a defined macro instead of a literal.

defined macro => static inline function.

> 
> Co-developed-by: Cody Haas <chaas@riotgames.com>
> Signed-off-by: Cody Haas <chaas@riotgames.com>
> Co-developed-by: Lisa Watanabe <lwatanabe@riotgames.com>
> Signed-off-by: Lisa Watanabe <lwatanabe@riotgames.com>
> Signed-off-by: Zvi Effron <zeffron@riotgames.com>
> ---
>   include/net/xdp.h | 5 +++++
>   net/core/filter.c | 4 ++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 5533f0ab2afc..8bfd21bfeddc 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -276,6 +276,11 @@ xdp_data_meta_unsupported(const struct xdp_buff *xdp)
>   	return unlikely(xdp->data_meta > xdp->data);
>   }
>   
> +static __always_inline int
> +xdp_metalen_valid(unsigned long metalen) {
> +	return (metalen & (sizeof(__u32) - 1)) || (metalen > 32);
> +}

Maybe change the signature and function name to
static inline bool
xdp_metalen_invalid(...) { ...}

The function returns true if it is invalid.

Let us just use "static inline bool". Return type "int"
changed to "bool" as it is indeed return a boolean.

"__always_inline" gives stronger hint to do inlining.
Most kernel static inline functions use "inline" attribute to
indicate it is good to inline, but if for whatever reason
compiler didn't inline, it won't be a disaster. For a function
like below, I would be surprised if it is not inlined with
"inline" attribute.


> +
>   struct xdp_attachment_info {
>   	struct bpf_prog *prog;
>   	u32 flags;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 5b86e47ef079..b4a64a07de88 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -77,6 +77,7 @@
>   #include <net/transp_v6.h>
>   #include <linux/btf_ids.h>
>   #include <net/tls.h>
> +#include <net/xdp.h>
>   
>   static const struct bpf_func_proto *
>   bpf_sk_base_func_proto(enum bpf_func_id func_id);
> @@ -3905,8 +3906,7 @@ BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, xdp, int, offset)
>   	if (unlikely(meta < xdp_frame_end ||
>   		     meta > xdp->data))
>   		return -EINVAL;
> -	if (unlikely((metalen & (sizeof(__u32) - 1)) ||
> -		     (metalen > 32)))
> +	if (unlikely(xdp_metalen_valid(metalen)))
>   		return -EACCES;
>   
>   	xdp->data_meta = meta;
>