RE: [PATCH bpf-next 5/8] xdp: add xdp_do_redirect_frame() for pre-computed xdp_frames

[John Fastabend <john.fastabend@gmail.com>]

Toke Høiland-Jørgensen wrote:
> Add an xdp_do_redirect_frame() variant which supports pre-computed
> xdp_frame structures. This will be used in bpf_prog_run() to avoid having
> to write to the xdp_frame structure when the XDP program doesn't modify the
> frame boundaries.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  include/linux/filter.h |  4 ++++
>  net/core/filter.c      | 28 +++++++++++++++++++++-------
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index b6a216eb217a..845452c83e0f 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1022,6 +1022,10 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>  int xdp_do_redirect(struct net_device *dev,
>  		    struct xdp_buff *xdp,
>  		    struct bpf_prog *prog);
> +int xdp_do_redirect_frame(struct net_device *dev,
> +			  struct xdp_buff *xdp,
> +			  struct xdp_frame *xdpf,
> +			  struct bpf_prog *prog);

I don't really like that we are passing both the xdp_buff ptr and
xdp_frame *xdpf around when one is always null it looks like?

>  void xdp_do_flush(void);
>  
>  /* The xdp_do_flush_map() helper has been renamed to drop the _map suffix, as
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 1e86130a913a..d8fe74cc8b66 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3957,14 +3957,13 @@ u32 xdp_master_redirect(struct xdp_buff *xdp)
>  }
>  EXPORT_SYMBOL_GPL(xdp_master_redirect);
>  
> -int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> -		    struct bpf_prog *xdp_prog)
> +static int __xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> +			     struct xdp_frame *xdpf, struct bpf_prog *xdp_prog)
>  {
>  	struct bpf_redirect_info *ri = this_cpu_ptr(&bpf_redirect_info);
>  	enum bpf_map_type map_type = ri->map_type;
>  	void *fwd = ri->tgt_value;
>  	u32 map_id = ri->map_id;
> -	struct xdp_frame *xdpf;
>  	struct bpf_map *map;
>  	int err;
>  
> @@ -3976,10 +3975,12 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>  		goto out;
>  	}
>  
> -	xdpf = xdp_convert_buff_to_frame(xdp);
> -	if (unlikely(!xdpf)) {
> -		err = -EOVERFLOW;
> -		goto err;
> +	if (!xdpf) {
> +		xdpf = xdp_convert_buff_to_frame(xdp);
> +		if (unlikely(!xdpf)) {
> +			err = -EOVERFLOW;
> +			goto err;
> +		}

This is a bit ugly imo. Can we just decide what gets handed to the function
rather than having this mid function conversion?

If we can't get consistency at least a xdpf_do_redirect() and then make
a xdp_do_redirect( return xdpf_do_redirect(xdp_convert_buff_to_frame(xdp)))
that just does the conversion and passes it through.

Or did I miss something?

>  	}
>  
>  	switch (map_type) {
> @@ -4024,8 +4025,21 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>  	_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map_type, map_id, ri->tgt_index, err);
>  	return err;
>  }
> +
> +int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
> +		    struct bpf_prog *xdp_prog)
> +{
> +	return __xdp_do_redirect(dev, xdp, NULL, xdp_prog);

same here. Just do the conversion and call,

 __xdpf_do_redirect(dev, xdpf, xdp_prog)

skipping the null pointr?

> +}
>  EXPORT_SYMBOL_GPL(xdp_do_redirect);
>  
> +int xdp_do_redirect_frame(struct net_device *dev, struct xdp_buff *xdp,
> +			  struct xdp_frame *xdpf, struct bpf_prog *xdp_prog)
> +{
> +	return __xdp_do_redirect(dev, xdp, xdpf, xdp_prog);
> +}
> +EXPORT_SYMBOL_GPL(xdp_do_redirect_frame);
> +
>  static int xdp_do_generic_redirect_map(struct net_device *dev,
>  				       struct sk_buff *skb,
>  				       struct xdp_buff *xdp,
> -- 
> 2.34.0
> 

Thanks,
John