netdev - RE: [PATCH v4 bpf-next 06/13] bpf: introduce bpf_xdp_get_frags_{count, total

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5f7748fc80bd9_38b02081@john-XPS-13-9370.notmuch>
Date:   Fri, 02 Oct 2020 08:36:28 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net, kuba@...nel.org, ast@...nel.org,
        daniel@...earbox.net, shayagr@...zon.com, sameehj@...zon.com,
        john.fastabend@...il.com, dsahern@...nel.org, brouer@...hat.com,
        lorenzo.bianconi@...hat.com, echaudro@...hat.com
Subject: RE: [PATCH v4 bpf-next 06/13] bpf: introduce
 bpf_xdp_get_frags_{count, total_size} helpers

Lorenzo Bianconi wrote:
> From: Sameeh Jubran <sameehj@...zon.com>
> 
> Introduce the two following bpf helpers in order to provide some
> metadata about a xdp multi-buff fame to bpf layer:
> 
> - bpf_xdp_get_frags_count()
>   get the number of fragments for a given xdp multi-buffer.

Same comment as in the cover letter can you provide a use case
for how/where I would use xdp_get_frags_count()? Is it just for
debug? If its just debug do we really want a uapi helper for it.

> 
> * bpf_xdp_get_frags_total_size()
>   get the total size of fragments for a given xdp multi-buffer.

This is awkward IMO. If total size is needed it should return total size
in all cases not just in the mb case otherwise programs will have two
paths the mb path and the non-mb path. And if you have mixed workload
the branch predictor will miss? Plus its extra instructions to load.

And if its useful for something beyond just debug and its going to be
read every packet or something I think we should put it in the metadata
so that its not hidden behind a helper which likely will show up as
overhead on a 40+gbps nic. The use case I have in mind is counting
bytes maybe sliced by IP or protocol. Here you will always read it
and I don't want code with a if/else stuck in the middle when if
we do it right we have a single read.

> 
> Signed-off-by: Sameeh Jubran <sameehj@...zon.com>
> Co-developed-by: Lorenzo Bianconi <lorenzo@...nel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
>  include/uapi/linux/bpf.h       | 14 ++++++++++++
>  net/core/filter.c              | 42 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 14 ++++++++++++
>  3 files changed, 70 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 4f556cfcbfbe..0715995eb18c 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3668,6 +3668,18 @@ union bpf_attr {
>   * 	Return
>   * 		The helper returns **TC_ACT_REDIRECT** on success or
>   * 		**TC_ACT_SHOT** on error.
> + *
> + * int bpf_xdp_get_frags_count(struct xdp_buff *xdp_md)
> + *	Description
> + *		Get the number of fragments for a given xdp multi-buffer.
> + *	Return
> + *		The number of fragments
> + *
> + * int bpf_xdp_get_frags_total_size(struct xdp_buff *xdp_md)
> + *	Description
> + *		Get the total size of fragments for a given xdp multi-buffer.

Why just fragments? Will I have to also add the initial frag0 to it
or not. I think the description is a bit ambiguous.

> + *	Return
> + *		The total size of fragments for a given xdp multi-buffer.
>   */

[...]

> +const struct bpf_func_proto bpf_xdp_get_frags_count_proto = {
> +	.func		= bpf_xdp_get_frags_count,
> +	.gpl_only	= false,
> +	.ret_type	= RET_INTEGER,
> +	.arg1_type	= ARG_PTR_TO_CTX,
> +};
> +
> +BPF_CALL_1(bpf_xdp_get_frags_total_size, struct  xdp_buff*, xdp)
> +{
> +	struct skb_shared_info *sinfo;
> +	int nfrags, i, size = 0;
> +
> +	if (likely(!xdp->mb))
> +		return 0;
> +
> +	sinfo = xdp_get_shared_info_from_buff(xdp);
> +	nfrags = min_t(u8, sinfo->nr_frags, MAX_SKB_FRAGS);
> +
> +	for (i = 0; i < nfrags; i++)
> +		size += skb_frag_size(&sinfo->frags[i]);

Wont the hardware just know this? I think walking the frag list
just to get the total seems wrong. The hardware should have a
total_len field somewhere we can just read no? If mvneta doesn't
know the total length that seems like a driver limitation and we
shouldn't encode it in the helper.

> +
> +	return size;
> +}