Re: [PATCH RFC v2 29/33] xdp: allow bpf_xdp_adjust_tail() to grow packet size

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Wed, 08 Apr 2020 13:53:01 +0200 Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 7628b947dbc3..4d58a147eed0 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3422,12 +3422,26 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
>  
>  BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
>  {
> +	void *data_hard_end = xdp_data_hard_end(xdp);
>  	void *data_end = xdp->data_end + offset;
>  
[...]
> +	/* DANGER: ALL drivers MUST be converted to init xdp->frame_sz
> +	 * - Adding some chicken checks below
> +	 * - Will (likely) not be for upstream
> +	 */
> +	if (unlikely(xdp->frame_sz < (xdp->data_end - xdp->data_hard_start))) {
> +		WARN(1, "Too small xdp->frame_sz = %d\n", xdp->frame_sz);
> +		return -EINVAL;
> +	}
> +	if (unlikely(xdp->frame_sz > PAGE_SIZE)) {
> +		WARN(1, "Too BIG xdp->frame_sz = %d\n", xdp->frame_sz);
> +		return -EINVAL;
> +	}

Any opinions on above checks?
Should they be removed or kept?

The idea is to catch drivers that forgot to update xdp_buff->frame_sz,
by doing some sanity checks on this uninit value.  If I correctly
updated all XDP drivers in this patchset, then these checks should be
unnecessary, but will this be valuable for driver developers converting
new drivers to XDP to have these WARN checks?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Help for reviewers:

+/* Reserve memory area at end-of data area.
+ *
+ * This macro reserves tailroom in the XDP buffer by limiting the
+ * XDP/BPF data access to data_hard_end.  Notice same area (and size)
+ * is used for XDP_PASS, when constructing the SKB via build_skb().
+ */
+#define xdp_data_hard_end(xdp)				\
+	((xdp)->data_hard_start + (xdp)->frame_sz -	\
+	 SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))