[PATCH net-next 0/2] net: support extack in dump and simplify ethtool uAPI

[PATCH net-next 0/2] net: support extack in dump and simplify ethtool uAPI

[Jakub Kicinski]

Ethtool currently requires header nest to be always present even if
it doesn't have to carry any attr for a given request. This inflicts
unnecessary pain on the users.

What makes it worse is that extack was not working in dump's ->start()
callback. Address both of those issues.

Jakub Kicinski (2):
  netlink: support extack in dump ->start()
  net: ethtool: don't require empty header nests

 include/linux/netlink.h  | 1 +
 net/ethtool/netlink.c    | 2 ++
 net/netlink/af_netlink.c | 2 ++
 net/netlink/genetlink.c  | 2 ++
 4 files changed, 7 insertions(+)

-- 
2.40.1

[PATCH net-next 1/2] netlink: support extack in dump ->start()

[Jakub Kicinski]

Commit 4a19edb60d02 ("netlink: Pass extack to dump handlers")
added extack support to netlink dumps. It was focused on rtnl
and since rtnl does not use ->start(), ->done() callbacks
it ignored those. Genetlink on the other hand uses ->start()
extensively, for parsing and input validation.

Pass the extact in via struct netlink_dump_control and link
it to cb for the time of ->start(). Both struct netlink_dump_control
and extack itself live on the stack so we can't keep the same
extack for the duration of the dump. This means that the extack
visible in ->start() and each ->dump() callbacks will be different.
Corner cases like reporting a warning message in DONE across dump
calls are still not supported.

We could put the extack (for dumps) in the socket struct,
but layering makes it slightly awkward (extack pointer is decided
before the DO / DUMP split).

The genetlink dump error extacks are now surfaced:

  $ cli.py --spec netlink/specs/ethtool.yaml --dump channels-get
  lib.ynl.NlError: Netlink error: Invalid argument
  nl_len = 64 (48) nl_flags = 0x300 nl_type = 2
	error: -22	extack: {'msg': 'request header missing'}

Previously extack was missing:

  $ cli.py --spec netlink/specs/ethtool.yaml --dump channels-get
  lib.ynl.NlError: Netlink error: Invalid argument
  nl_len = 36 (20) nl_flags = 0x100 nl_type = 2
	error: -22

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/netlink.h  | 1 +
 net/netlink/af_netlink.c | 2 ++
 net/netlink/genetlink.c  | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 19c0791ed9d5..9eec3f4f5351 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -311,6 +311,7 @@ struct netlink_dump_control {
 	int (*start)(struct netlink_callback *);
 	int (*dump)(struct sk_buff *skb, struct netlink_callback *);
 	int (*done)(struct netlink_callback *);
+	struct netlink_ext_ack *extack;
 	void *data;
 	struct module *module;
 	u32 min_dump_alloc;
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 3a1e0fd5bf14..cbd9aa7ee24a 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2360,7 +2360,9 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	cb->strict_check = !!(nlk2->flags & NETLINK_F_STRICT_CHK);
 
 	if (control->start) {
+		cb->extack = control->extack;
 		ret = control->start(cb);
+		cb->extack = NULL;
 		if (ret)
 			goto error_put;
 	}
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 04c4036bf406..a157247a1e45 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -912,6 +912,7 @@ static int genl_family_rcv_msg_dumpit(const struct genl_family *family,
 			.start = genl_start,
 			.dump = genl_lock_dumpit,
 			.done = genl_lock_done,
+			.extack = extack,
 		};
 
 		genl_unlock();
@@ -924,6 +925,7 @@ static int genl_family_rcv_msg_dumpit(const struct genl_family *family,
 			.start = genl_start,
 			.dump = ops->dumpit,
 			.done = genl_parallel_done,
+			.extack = extack,
 		};
 
 		err = __netlink_dump_start(net->genl_sock, skb, nlh, &c);
-- 
2.40.1

[PATCH net-next 2/2] net: ethtool: don't require empty header nests

[Jakub Kicinski]

Ethtool currently requires a header nest (which is used to carry
the common family options) in all requests including dumps.

  $ cli.py --spec netlink/specs/ethtool.yaml --dump channels-get
  lib.ynl.NlError: Netlink error: Invalid argument
  nl_len = 64 (48) nl_flags = 0x300 nl_type = 2
	error: -22      extack: {'msg': 'request header missing'}

  $ cli.py --spec netlink/specs/ethtool.yaml --dump channels-get \
           --json '{"header":{}}';  )
  [{'combined-count': 1,
    'combined-max': 1,
    'header': {'dev-index': 2, 'dev-name': 'enp1s0'}}]

Requiring the header nest to always be there may seem nice
from the consistency perspective, but it's not serving any
practical purpose. We shouldn't burden the user like this.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 08120095cc68..5dd5e8222c45 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -96,6 +96,8 @@ int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 	int ret;
 
 	if (!header) {
+		if (!require_dev)
+			return 0;
 		NL_SET_ERR_MSG(extack, "request header missing");
 		return -EINVAL;
 	}
-- 
2.40.1

Re: [PATCH net-next 0/2] net: support extack in dump and simplify ethtool uAPI

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri,  9 Jun 2023 14:53:29 -0700 you wrote:
> Ethtool currently requires header nest to be always present even if
> it doesn't have to carry any attr for a given request. This inflicts
> unnecessary pain on the users.
> 
> What makes it worse is that extack was not working in dump's ->start()
> callback. Address both of those issues.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] netlink: support extack in dump ->start()
    https://git.kernel.org/netdev/net-next/c/5ab8c41cef30
  - [net-next,2/2] net: ethtool: don't require empty header nests
    https://git.kernel.org/netdev/net-next/c/500e1340d1d2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html