netdev - Re: [RFC PATCH bpf-next 2/4] libbpf: check for channels.max_{t,r}x in xsk_get_max

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: <20190604170545.00003ee7@gmail.com>
Date:   Tue, 4 Jun 2019 17:05:45 +0200
From:   Maciej Fijalkowski <maciejromanfijalkowski@...il.com>
To:     Björn Töpel <bjorn.topel@...el.com>
Cc:     magnus.karlsson@...el.com, netdev@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, jakub.kicinski@...ronome.com,
        jonathan.lemon@...il.com, songliubraving@...com,
        bpf <bpf@...r.kernel.org>
Subject: Re: [RFC PATCH bpf-next 2/4] libbpf: check for channels.max_{t,r}x
 in xsk_get_max_queues

On Tue, 4 Jun 2019 10:06:57 +0200
Björn Töpel <bjorn.topel@...el.com> wrote:

> On 2019-06-03 15:19, Maciej Fijalkowski wrote:
> > When it comes down to ethtool's get channels API, various drivers are
> > reporting the queue count in two ways - they are setting max_combined or
> > max_tx/max_rx fields. When creating the eBPF maps for xsk socket, this
> > API is used so that we have an entries in maps per each queue.
> > In case where driver (mlx4, ice) reports queues in max_tx/max_rx, we end
> > up with eBPF maps with single entries, so it's not possible to attach an
> > AF_XDP socket onto queue other than 0 - xsk_set_bpf_maps() would try to
> > call bpf_map_update_elem() with key set to xsk->queue_id.
> > 
> > To fix this, let's look for channels.max_{t,r}x as well in
> > xsk_get_max_queues.
> > 
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> > ---
> >   tools/lib/bpf/xsk.c | 18 ++++++++++--------
> >   1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index 57dda1389870..514ab3fb06f4 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -339,21 +339,23 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
> >   	ifr.ifr_data = (void *)&channels;
> >   	strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ);
> >   	err = ioctl(fd, SIOCETHTOOL, &ifr);
> > -	if (err && errno != EOPNOTSUPP) {
> > -		ret = -errno;
> > -		goto out;
> > -	}
> > +	close(fd);
> > +
> > +	if (err && errno != EOPNOTSUPP)
> > +		return -errno;
> >   
> > -	if (channels.max_combined == 0 || errno == EOPNOTSUPP)
> > +	if (channels.max_combined)
> > +		ret = channels.max_combined;
> > +	else if (channels.max_rx && channels.max_tx)
> > +		ret = min(channels.max_rx, channels.max_tx);
> 
> Hmm, do we really need to look at max_tx? For each Rx, there's (usually)
> an XDP ring.

Probably we would be good to go with only max_rx, but in drivers during the
umem setup we also are comparing the queue id provided by user against the num
tx queues...so in theory, we could allocate the max_rx entries, but if the
current txq count is lower than reported max_rx, a bunch of map entries would
never be used, no?

>
> OTOH, when AF_XDP ZC is not implemented, it uses the skb path...
> 
> > +	else if (channels.max_combined == 0 || errno == EOPNOTSUPP)
> >   		/* If the device says it has no channels, then all traffic
> >   		 * is sent to a single stream, so max queues = 1.
> >   		 */
> >   		ret = 1;
> >   	else
> > -		ret = channels.max_combined;
> > +		ret = -1;
> >   
> > -out:
> > -	close(fd);
> >   	return ret;
> >   }
> >   
> >