netdev - Re: [PATCH v5 bpf-next 10/24] xsk: add new netlink attribute dedicated for ZC max frags

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] [day] [month] [year] [list]

Message-ID: <ZLESlub35LTIJgzh@boxer>
Date: Fri, 14 Jul 2023 11:17:10 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
CC: bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>, "Daniel
 Borkmann" <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
	Network Development <netdev@...r.kernel.org>, "Karlsson, Magnus"
	<magnus.karlsson@...el.com>, Björn Töpel
	<bjorn@...nel.org>, "Sarkar, Tirthendu" <tirthendu.sarkar@...el.com>, "Simon
 Horman" <simon.horman@...igine.com>, Toke
 Høiland-Jørgensen <toke@...nel.org>
Subject: Re: [PATCH v5 bpf-next 10/24] xsk: add new netlink attribute
 dedicated for ZC max frags

On Thu, Jul 13, 2023 at 04:02:42PM -0700, Alexei Starovoitov wrote:
> On Thu, Jul 13, 2023 at 11:59 AM Maciej Fijalkowski
> <maciej.fijalkowski@...el.com> wrote:
> >
> > On Mon, Jul 10, 2023 at 06:09:28PM -0700, Alexei Starovoitov wrote:
> > > On Thu, Jul 6, 2023 at 1:47 PM Maciej Fijalkowski
> > > <maciej.fijalkowski@...el.com> wrote:
> > > >
> > > > Introduce new netlink attribute NETDEV_A_DEV_XDP_ZC_MAX_SEGS that will
> > > > carry maximum fragments that underlying ZC driver is able to handle on
> > > > TX side. It is going to be included in netlink response only when driver
> > > > supports ZC. Any value higher than 1 implies multi-buffer ZC support on
> > > > underlying device.
> > > >
> > > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> > >
> > > I suspect something in this patch makes XDP bonding test fail.
> > > See BPF CI.
> > >
> > > I can reproduce the failure locally as well.
> > > test_progs -t bond
> > > works without the series and fails with them.
> >
> > Hi Alexei,
> >
> > this fails on second bpf_xdp_query() call due to non-zero (?) contents at the
> > end of bpf_xdp_query_opts struct - currently it looks as following:
> >
> > $ pahole -C bpf_xdp_query_opts libbpf.so
> > struct bpf_xdp_query_opts {
> >         size_t                     sz;                   /*     0     8 */
> >         __u32                      prog_id;              /*     8     4 */
> >         __u32                      drv_prog_id;          /*    12     4 */
> >         __u32                      hw_prog_id;           /*    16     4 */
> >         __u32                      skb_prog_id;          /*    20     4 */
> >         __u8                       attach_mode;          /*    24     1 */
> >
> >         /* XXX 7 bytes hole, try to pack */
> >
> >         __u64                      feature_flags;        /*    32     8 */
> >         __u32                      xdp_zc_max_segs;      /*    40     4 */
> >
> >         /* size: 48, cachelines: 1, members: 8 */
> >         /* sum members: 37, holes: 1, sum holes: 7 */
> >         /* padding: 4 */
> >         /* last cacheline: 48 bytes */
> > };
> >
> > Fix is either to move xdp_zc_max_segs up to existing hole or to zero out
> > struct before bpf_xdp_query() calls, like:
> >
> >         memset(&query_opts, 0, sizeof(struct bpf_xdp_query_opts));
> >         query_opts.sz = sizeof(struct bpf_xdp_query_opts);
> 
> Right. That would be good to have to clear the hole,
> but probably unrelated.
> 
> > I am kinda confused as this is happening due to two things. First off
> > bonding driver sets its xdp_features to NETDEV_XDP_ACT_MASK and in turn
> > this implies ZC feature enabled which makes xdp_zc_max_segs being included
> > in the response (it's value is 1 as it's the default).
> >
> > Then, offsetofend(struct type, type##__last_field) that is used as one of
> > libbpf_validate_opts() args gives me 40 but bpf_xdp_query_opts::sz has
> > stored 48, so in the end we go through the last 8 bytes in
> > libbpf_is_mem_zeroed() and we hit the '1' from xdp_zc_max_segs.
> 
> Because this patch didn't update
> bpf_xdp_query_opts__last_field

Heh, bummer, this was right in front of me whole time. I think I need a
break:) but before that let me send v6. Thanks Alexei.

> 
> It added a new field, but didn't update the macro.
> 
> > So, (silly) questions:
> > - why bonding driver defaults to all features enabled?
> 
> doesn't really matter in this context.
> 
> > - why __last_field does not recognize xdp_zc_max_segs at the end?
> 
> because the patch didn't update it :)
> 
> > Besides, I think i'll move xdp_zc_max_segs above to the hole. This fixes
> > the bonding test for me.
> 
> No. Keep it at the end.