Re: [PATCH RFC bpf-next 02/20] trait: XDP support

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5250 bytes --]

On Mar 05, arthur@arthurfabre.com wrote:
> From: Arthur Fabre <afabre@cloudflare.com>
> 

[...]

> +static __always_inline void *xdp_buff_traits(const struct xdp_buff *xdp)
> +{
> +	return xdp->data_hard_start + _XDP_FRAME_SIZE;
> +}
> +
>  static __always_inline void
>  xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
>  {
> @@ -133,6 +139,13 @@ xdp_prepare_buff(struct xdp_buff *xdp, unsigned char *hard_start,
>  	xdp->data = data;
>  	xdp->data_end = data + data_len;
>  	xdp->data_meta = meta_valid ? data : data + 1;
> +
> +	if (meta_valid) {

can we relax this constraint and use xdp->data as end boundary here?

> +		/* We assume drivers reserve enough headroom to store xdp_frame
> +		 * and the traits header.
> +		 */
> +		traits_init(xdp_buff_traits(xdp), xdp->data_meta);
> +	}
>  }
>  
>  /* Reserve memory area at end-of data area.
> @@ -267,6 +280,8 @@ struct xdp_frame {
>  	u32 flags; /* supported values defined in xdp_buff_flags */
>  };
>  
> +static_assert(sizeof(struct xdp_frame) == _XDP_FRAME_SIZE);
> +
>  static __always_inline bool xdp_frame_has_frags(const struct xdp_frame *frame)
>  {
>  	return !!(frame->flags & XDP_FLAGS_HAS_FRAGS);
> @@ -517,6 +532,11 @@ static inline bool xdp_metalen_invalid(unsigned long metalen)
>  	return !IS_ALIGNED(metalen, sizeof(u32)) || metalen > meta_max;
>  }
>  
> +static __always_inline void *xdp_meta_hard_start(const struct xdp_buff *xdp)
> +{
> +	return xdp_buff_traits(xdp) + traits_size(xdp_buff_traits(xdp));

here we are always consuming sizeof(struct __trait_hdr)), right? We can do
somehing smarter and check if traits are really used? (e.g. adding in the flags
in xdp_buff)?

> +}
> +
>  struct xdp_attachment_info {
>  	struct bpf_prog *prog;
>  	u32 flags;
> diff --git a/net/core/filter.c b/net/core/filter.c
> index dcc53ac5c5458f67a422453134665d43d466a02e..79b78e7cd57fd78c6cc8443da54ae96408c496b0 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -85,6 +85,7 @@
>  #include <linux/un.h>
>  #include <net/xdp_sock_drv.h>
>  #include <net/inet_dscp.h>
> +#include <net/trait.h>
>  
>  #include "dev.h"
>  
> @@ -3935,9 +3936,8 @@ static unsigned long xdp_get_metalen(const struct xdp_buff *xdp)
>  
>  BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
>  {
> -	void *xdp_frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
>  	unsigned long metalen = xdp_get_metalen(xdp);
> -	void *data_start = xdp_frame_end + metalen;
> +	void *data_start = xdp_meta_hard_start(xdp) + metalen;

We could waste 16byte here, right?

Regards,
Lorenzo

>  	void *data = xdp->data + offset;
>  
>  	if (unlikely(data < data_start ||
> @@ -4228,13 +4228,12 @@ static const struct bpf_func_proto bpf_xdp_adjust_tail_proto = {
>  
>  BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, xdp, int, offset)
>  {
> -	void *xdp_frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
>  	void *meta = xdp->data_meta + offset;
>  	unsigned long metalen = xdp->data - meta;
>  
>  	if (xdp_data_meta_unsupported(xdp))
>  		return -ENOTSUPP;
> -	if (unlikely(meta < xdp_frame_end ||
> +	if (unlikely(meta < xdp_meta_hard_start(xdp) ||
>  		     meta > xdp->data))
>  		return -EINVAL;
>  	if (unlikely(xdp_metalen_invalid(metalen)))
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 2c6ab6fb452f7b90d85125ae17fef96cfc9a8576..2e87f82aa5f835f60295d859a524e40bd47c42ee 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -1032,3 +1032,53 @@ void xdp_features_clear_redirect_target(struct net_device *dev)
>  	xdp_set_features_flag(dev, val);
>  }
>  EXPORT_SYMBOL_GPL(xdp_features_clear_redirect_target);
> +
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc int bpf_xdp_trait_set(const struct xdp_buff *xdp, u64 key,
> +				  const void *val, u64 val__sz, u64 flags)
> +{
> +	if (xdp_data_meta_unsupported(xdp))
> +		return -EOPNOTSUPP;
> +
> +	return trait_set(xdp_buff_traits(xdp), xdp->data_meta, key,
> +			 val, val__sz, flags);
> +}
> +
> +__bpf_kfunc int bpf_xdp_trait_get(const struct xdp_buff *xdp, u64 key,
> +				  void *val, u64 val__sz)
> +{
> +	if (xdp_data_meta_unsupported(xdp))
> +		return -EOPNOTSUPP;
> +
> +	return trait_get(xdp_buff_traits(xdp), key, val, val__sz);
> +}
> +
> +__bpf_kfunc int bpf_xdp_trait_del(const struct xdp_buff *xdp, u64 key)
> +{
> +	if (xdp_data_meta_unsupported(xdp))
> +		return -EOPNOTSUPP;
> +
> +	return trait_del(xdp_buff_traits(xdp), key);
> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +BTF_KFUNCS_START(xdp_trait)
> +// TODO - should we use KF_TRUSTED_ARGS? https://www.kernel.org/doc/html/next/bpf/kfuncs.html#kf-trusted-args-flag
> +BTF_ID_FLAGS(func, bpf_xdp_trait_set)
> +BTF_ID_FLAGS(func, bpf_xdp_trait_get)
> +BTF_ID_FLAGS(func, bpf_xdp_trait_del)
> +BTF_KFUNCS_END(xdp_trait)
> +
> +static const struct btf_kfunc_id_set xdp_trait_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set = &xdp_trait,
> +};
> +
> +static int xdp_trait_init(void)
> +{
> +	return register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP,
> +					 &xdp_trait_kfunc_set);
> +}
> +late_initcall(xdp_trait_init);
> 
> -- 
> 2.43.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]