netdev - Re: [PATCH v19 bpf-next 12/23] bpf: add multi-buff support to the bpf_xdp_adjust

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: <61ae76d882a48_106e020844@john.notmuch>
Date:   Mon, 06 Dec 2021 12:47:20 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        John Fastabend <john.fastabend@...il.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        ast@...nel.org, daniel@...earbox.net, shayagr@...zon.com,
        dsahern@...nel.org, brouer@...hat.com, echaudro@...hat.com,
        jasowang@...hat.com, alexander.duyck@...il.com, saeed@...nel.org,
        maciej.fijalkowski@...el.com, magnus.karlsson@...el.com,
        tirthendu.sarkar@...el.com, toke@...hat.com
Subject: Re: [PATCH v19 bpf-next 12/23] bpf: add multi-buff support to the
 bpf_xdp_adjust_tail() API

Lorenzo Bianconi wrote:
> > Lorenzo Bianconi wrote:
> > > > Lorenzo Bianconi wrote:
> > > > > From: Eelco Chaudron <echaudro@...hat.com>
> > > > > 
> > > > > This change adds support for tail growing and shrinking for XDP multi-buff.
> > > > > 
> > > > > When called on a multi-buffer packet with a grow request, it will work
> > > > > on the last fragment of the packet. So the maximum grow size is the
> > > > > last fragments tailroom, i.e. no new buffer will be allocated.
> > > > > A XDP mb capable driver is expected to set frag_size in xdp_rxq_info data
> > > > > structure to notify the XDP core the fragment size. frag_size set to 0 is
> > > > > interpreted by the XDP core as tail growing is not allowed.
> > > > > Introduce __xdp_rxq_info_reg utility routine to initialize frag_size field.
> > > > > 
> > > > > When shrinking, it will work from the last fragment, all the way down to
> > > > > the base buffer depending on the shrinking size. It's important to mention
> > > > > that once you shrink down the fragment(s) are freed, so you can not grow
> > > > > again to the original size.
> > > > > 
> > > > > Acked-by: Jakub Kicinski <kuba@...nel.org>
> > > > > Co-developed-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > > > > Signed-off-by: Eelco Chaudron <echaudro@...hat.com>

[...]

> > Then later there is the check 'if (unlikely(offset > 0) { ...}', but that
> > wont hit this case and we shrunk it back to a single frag. Did we want
> > to clear the mb in this case? I'm not seeing how it harms things to have
> > the mb bit set just trying to follow code here.
> 
> If I followed correctly your example, we will have sinfo->nr_frags = 1 at the
> end of the processing (since the first fragment has 2k size), right?
> If so mb bit must be set to 1. Am I missing something?
> Re-looking at the code I guess we should clear mb bit using sinfo->nr_frags
> instead:
> 
> 	if (!sinfo->nr_frags)
> 		xdp_buff_clear_mb(xdp);
> 
> Agree?

Agree that is more correct. I maybe messed up my example a bit, but I think
checking nr_frags clears it up.

Thanks,
John

> 
> Regards,
> Lorenzo