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: <87505132-2f1b-dc4d-5c1f-d52fc8dca647@intel.com>
Date:   Tue, 4 Jun 2019 10:06:57 +0200
From:   Björn Töpel <bjorn.topel@...el.com>
To:     Maciej Fijalkowski <maciejromanfijalkowski@...il.com>,
        magnus.karlsson@...el.com, netdev@...r.kernel.org
Cc:     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 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.

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;
>   }
>   
>