netdev - Re: [PATCH bpf-next 1/7] netdev-genl: create a simple family for netdev stuff

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: <20230120191126.06c9d514@kernel.org>
Date:   Fri, 20 Jan 2023 19:11:26 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org, davem@...emloft.net,
        hawk@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
        toke@...hat.com, memxor@...il.com, alardam@...il.com,
        saeedm@...dia.com, anthony.l.nguyen@...el.com, gospo@...adcom.com,
        vladimir.oltean@....com, nbd@....name, john@...ozen.org,
        leon@...nel.org, simon.horman@...igine.com, aelior@...vell.com,
        christophe.jaillet@...adoo.fr, ecree.xilinx@...il.com,
        mst@...hat.com, bjorn@...nel.org, magnus.karlsson@...el.com,
        maciej.fijalkowski@...el.com, intel-wired-lan@...ts.osuosl.org,
        lorenzo.bianconi@...hat.com, niklas.soderlund@...igine.com
Subject: Re: [PATCH bpf-next 1/7] netdev-genl: create a simple family for
 netdev stuff

On Fri, 20 Jan 2023 18:16:50 +0100 Lorenzo Bianconi wrote:
> From: Jakub Kicinski <kuba@...nel.org>
> 
> Add a Netlink spec-compatible family for netdevs.
> This is a very simple implementation without much
> thought going into it.
> 
> It allows us to reap all the benefits of Netlink specs,
> one can use the generic client to issue the commands:
> 
>   $ ./gen.py --spec netdev.yaml --do dev_get --json='{"ifindex": 2}'
>   {'ifindex': 2, 'xdp-features': 31}
> 
>   $ ./gen.py --spec netdev.yaml --dump dev_get
>   [{'ifindex': 1, 'xdp-features': 0}, {'ifindex': 2, 'xdp-features': 31}]

In the meantime I added support for rendering enums in Python.
So you can show names in the example. eg:

$ ./cli.py --spec netdev.yaml --dump dev_get 
[{'ifindex': 1, 'xdp-features': set()},
 {'ifindex': 2,
  'xdp-features': {'ndo-xmit', 'pass', 'redirect', 'aborted', 'drop'}},
 {'ifindex': 3, 'xdp-features': {'rx-sg'}}]

> the generic python library does not have flags-by-name
> support, yet, but we also don't have to carry strings
> in the messages, as user space can get the names from
> the spec.
> 
> Co-developed-by: Lorenzo Bianconi <lorenzo@...nel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> Co-developed-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@...il.com>
> Co-developed-by: Marek Majtyka <alardam@...il.com>
> Signed-off-by: Marek Majtyka <alardam@...il.com>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  Documentation/netlink/specs/netdev.yaml |  72 ++++++++++

FWIW I'm not 100% sure if we should scope the family to all of netdev
or just xdp. Same for the name of the op, should we call the op dev_get
or dev_xdp_get..

> diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> new file mode 100644
> index 000000000000..254fc336d469
> --- /dev/null
> +++ b/include/uapi/linux/netdev.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* Do not edit directly, auto-generated from: */

Like this line says, you can't hand edit this file.
Next time someone adds an attribute all your changes will be wiped.

> +/*	Documentation/netlink/specs/netdev.yaml */
> +/* YNL-GEN uapi header */
> +
> +#ifndef _UAPI_LINUX_NETDEV_H
> +#define _UAPI_LINUX_NETDEV_H
> +
> +#define NETDEV_FAMILY_NAME	"netdev"
> +#define NETDEV_FAMILY_VERSION	1
> +
> +enum netdev_xdp_act {
> +	NETDEV_XDP_ACT_ABORTED_BIT,
> +	NETDEV_XDP_ACT_DROP_BIT,
> +	NETDEV_XDP_ACT_PASS_BIT,
> +	NETDEV_XDP_ACT_TX_BIT,
> +	NETDEV_XDP_ACT_REDIRECT_BIT,
> +	NETDEV_XDP_ACT_NDO_XMIT_BIT,
> +	NETDEV_XDP_ACT_XSK_ZEROCOPY_BIT,
> +	NETDEV_XDP_ACT_HW_OFFLOAD_BIT,
> +	NETDEV_XDP_ACT_RX_SG_BIT,
> +	NETDEV_XDP_ACT_NDO_XMIT_SG_BIT

You need to add -bit to all the enum names in the yaml if you want 
to have _BIT in the name here.

> +};
> +
> +#define NETDEV_XDP_ACT_ABORTED		BIT(NETDEV_XDP_ACT_ABORTED_BIT)
> +#define NETDEV_XDP_ACT_DROP		BIT(NETDEV_XDP_ACT_DROP_BIT)
> +#define NETDEV_XDP_ACT_PASS		BIT(NETDEV_XDP_ACT_PASS_BIT)
> +#define NETDEV_XDP_ACT_TX		BIT(NETDEV_XDP_ACT_TX_BIT)
> +#define NETDEV_XDP_ACT_REDIRECT		BIT(NETDEV_XDP_ACT_REDIRECT_BIT)
> +#define NETDEV_XDP_ACT_NDO_XMIT		BIT(NETDEV_XDP_ACT_NDO_XMIT_BIT)
> +#define NETDEV_XDP_ACT_XSK_ZEROCOPY	BIT(NETDEV_XDP_ACT_XSK_ZEROCOPY_BIT)
> +#define NETDEV_XDP_ACT_HW_OFFLOAD	BIT(NETDEV_XDP_ACT_HW_OFFLOAD_BIT)
> +#define NETDEV_XDP_ACT_RX_SG		BIT(NETDEV_XDP_ACT_RX_SG_BIT)
> +#define NETDEV_XDP_ACT_NDO_XMIT_SG	BIT(NETDEV_XDP_ACT_NDO_XMIT_SG_BIT)
> +
> +#define NETDEV_XDP_ACT_BASIC		(NETDEV_XDP_ACT_DROP |	\
> +					 NETDEV_XDP_ACT_PASS |	\
> +					 NETDEV_XDP_ACT_TX |	\
> +					 NETDEV_XDP_ACT_ABORTED)
> +#define NETDEV_XDP_ACT_FULL		(NETDEV_XDP_ACT_BASIC |	\
> +					 NETDEV_XDP_ACT_REDIRECT)
> +#define NETDEV_XDP_ACT_ZC		(NETDEV_XDP_ACT_FULL |	\
> +					 NETDEV_XDP_ACT_XSK_ZEROCOPY)

These defines don't belong in uAPI. Especially the use of BIT().

> +			if (err < 0)
> +				break;
> +cont:
> +			idx++;
> +		}
> +	}
> +
> +	rtnl_unlock();
> +
> +	if (err != -EMSGSIZE)
> +		return err;
> +
> +	cb->args[1] = idx;
> +	cb->args[0] = h;
> +	cb->seq = net->dev_base_seq;
> +	nl_dump_check_consistent(cb, nlmsg_hdr(skb));

I think that this line can be dropped.

> +	return skb->len;
> +}