[PATCH net-next 0/4] netdev: support dumping a single netdev in qstats

[PATCH net-next 0/4] netdev: support dumping a single netdev in qstats

[Jakub Kicinski]

I was writing a test for page pool which depended on qstats,
and got tired of having to filter dumps in user space.
Add support for dumping stats for a single netdev.

To get there we first need to add full support for extack
in dumps (and fix a dump error handling bug in YNL, sent
separately to the net tree).

Jakub Kicinski (4):
  netdev: support dumping a single netdev in qstats
  netlink: move extack writing helpers
  netlink: support all extack types in dumps
  selftests: drv-net: test dumping qstats per device

 Documentation/netlink/specs/netdev.yaml      |   1 +
 net/core/netdev-genl-gen.c                   |   1 +
 net/core/netdev-genl.c                       |  52 +++++--
 net/netlink/af_netlink.c                     | 135 ++++++++++---------
 tools/testing/selftests/drivers/net/stats.py |  62 ++++++++-
 tools/testing/selftests/net/lib/py/ksft.py   |  18 +++
 6 files changed, 188 insertions(+), 81 deletions(-)

-- 
2.44.0

[PATCH net-next 1/4] netdev: support dumping a single netdev in qstats

[Jakub Kicinski]

Having to filter the right ifindex in the tests is a bit tedious.
Add support for dumping qstats for a single ifindex.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/netlink/specs/netdev.yaml |  1 +
 net/core/netdev-genl-gen.c              |  1 +
 net/core/netdev-genl.c                  | 52 ++++++++++++++++++-------
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
index 76352dbd2be4..679c4130707c 100644
--- a/Documentation/netlink/specs/netdev.yaml
+++ b/Documentation/netlink/specs/netdev.yaml
@@ -486,6 +486,7 @@ name: netdev
       dump:
         request:
           attributes:
+            - ifindex
             - scope
         reply:
           attributes:
diff --git a/net/core/netdev-genl-gen.c b/net/core/netdev-genl-gen.c
index 8d8ace9ef87f..8350a0afa9ec 100644
--- a/net/core/netdev-genl-gen.c
+++ b/net/core/netdev-genl-gen.c
@@ -70,6 +70,7 @@ static const struct nla_policy netdev_napi_get_dump_nl_policy[NETDEV_A_NAPI_IFIN
 
 /* NETDEV_CMD_QSTATS_GET - dump */
 static const struct nla_policy netdev_qstats_get_nl_policy[NETDEV_A_QSTATS_SCOPE + 1] = {
+	[NETDEV_A_QSTATS_IFINDEX] = NLA_POLICY_MIN(NLA_U32, 1),
 	[NETDEV_A_QSTATS_SCOPE] = NLA_POLICY_MASK(NLA_UINT, 0x1),
 };
 
diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 7004b3399c2b..dd6510f2c652 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -639,6 +639,24 @@ netdev_nl_stats_by_netdev(struct net_device *netdev, struct sk_buff *rsp,
 	return -EMSGSIZE;
 }
 
+static int
+netdev_nl_qstats_get_dump_one(struct net_device *netdev, unsigned int scope,
+			      struct sk_buff *skb, const struct genl_info *info,
+			      struct netdev_nl_dump_ctx *ctx)
+{
+	if (!netdev->stat_ops)
+		return 0;
+
+	switch (scope) {
+	case 0:
+		return netdev_nl_stats_by_netdev(netdev, skb, info);
+	case NETDEV_QSTATS_SCOPE_QUEUE:
+		return netdev_nl_stats_by_queue(netdev, skb, info, ctx);
+	}
+
+	return -EINVAL;	/* Should not happen, per netlink policy */
+}
+
 int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
 				struct netlink_callback *cb)
 {
@@ -646,6 +664,7 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
 	const struct genl_info *info = genl_info_dump(cb);
 	struct net *net = sock_net(skb->sk);
 	struct net_device *netdev;
+	unsigned int ifindex;
 	unsigned int scope;
 	int err = 0;
 
@@ -653,21 +672,28 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
 	if (info->attrs[NETDEV_A_QSTATS_SCOPE])
 		scope = nla_get_uint(info->attrs[NETDEV_A_QSTATS_SCOPE]);
 
-	rtnl_lock();
-	for_each_netdev_dump(net, netdev, ctx->ifindex) {
-		if (!netdev->stat_ops)
-			continue;
+	ifindex = 0;
+	if (info->attrs[NETDEV_A_QSTATS_IFINDEX])
+		ifindex = nla_get_u32(info->attrs[NETDEV_A_QSTATS_IFINDEX]);
 
-		switch (scope) {
-		case 0:
-			err = netdev_nl_stats_by_netdev(netdev, skb, info);
-			break;
-		case NETDEV_QSTATS_SCOPE_QUEUE:
-			err = netdev_nl_stats_by_queue(netdev, skb, info, ctx);
-			break;
+	rtnl_lock();
+	if (ifindex) {
+		netdev = __dev_get_by_index(net, ifindex);
+		if (netdev && netdev->stat_ops) {
+			err = netdev_nl_qstats_get_dump_one(netdev, scope, skb,
+							    info, ctx);
+		} else {
+			NL_SET_BAD_ATTR(info->extack,
+					info->attrs[NETDEV_A_QSTATS_IFINDEX]);
+			err = netdev ? -EOPNOTSUPP : -ENODEV;
+		}
+	} else {
+		for_each_netdev_dump(net, netdev, ctx->ifindex) {
+			err = netdev_nl_qstats_get_dump_one(netdev, scope, skb,
+							    info, ctx);
+			if (err < 0)
+				break;
 		}
-		if (err < 0)
-			break;
 	}
 	rtnl_unlock();
 
-- 
2.44.0

Re: [PATCH net-next 1/4] netdev: support dumping a single netdev in qstats

[David Ahern]

On 4/19/24 8:35 PM, Jakub Kicinski wrote:
> Having to filter the right ifindex in the tests is a bit tedious.
> Add support for dumping qstats for a single ifindex.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  Documentation/netlink/specs/netdev.yaml |  1 +
>  net/core/netdev-genl-gen.c              |  1 +
>  net/core/netdev-genl.c                  | 52 ++++++++++++++++++-------
>  3 files changed, 41 insertions(+), 13 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 1/4] netdev: support dumping a single netdev in qstats

[Eric Dumazet]

On Sat, Apr 20, 2024 at 4:35 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Having to filter the right ifindex in the tests is a bit tedious.
> Add support for dumping qstats for a single ifindex.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  Documentation/netlink/specs/netdev.yaml |  1 +
>  net/core/netdev-genl-gen.c              |  1 +
>  net/core/netdev-genl.c                  | 52 ++++++++++++++++++-------
>  3 files changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> index 76352dbd2be4..679c4130707c 100644
> --- a/Documentation/netlink/specs/netdev.yaml
> +++ b/Documentation/netlink/specs/netdev.yaml
> @@ -486,6 +486,7 @@ name: netdev
>        dump:
>          request:
>            attributes:
> +            - ifindex
>              - scope
>          reply:
>            attributes:
> diff --git a/net/core/netdev-genl-gen.c b/net/core/netdev-genl-gen.c
> index 8d8ace9ef87f..8350a0afa9ec 100644
> --- a/net/core/netdev-genl-gen.c
> +++ b/net/core/netdev-genl-gen.c
> @@ -70,6 +70,7 @@ static const struct nla_policy netdev_napi_get_dump_nl_policy[NETDEV_A_NAPI_IFIN
>
>  /* NETDEV_CMD_QSTATS_GET - dump */
>  static const struct nla_policy netdev_qstats_get_nl_policy[NETDEV_A_QSTATS_SCOPE + 1] = {
> +       [NETDEV_A_QSTATS_IFINDEX] = NLA_POLICY_MIN(NLA_U32, 1),
>         [NETDEV_A_QSTATS_SCOPE] = NLA_POLICY_MASK(NLA_UINT, 0x1),
>  };
>
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 7004b3399c2b..dd6510f2c652 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -639,6 +639,24 @@ netdev_nl_stats_by_netdev(struct net_device *netdev, struct sk_buff *rsp,
>         return -EMSGSIZE;
>  }
>
> +static int
> +netdev_nl_qstats_get_dump_one(struct net_device *netdev, unsigned int scope,
> +                             struct sk_buff *skb, const struct genl_info *info,
> +                             struct netdev_nl_dump_ctx *ctx)
> +{
> +       if (!netdev->stat_ops)
> +               return 0;
> +
> +       switch (scope) {
> +       case 0:
> +               return netdev_nl_stats_by_netdev(netdev, skb, info);
> +       case NETDEV_QSTATS_SCOPE_QUEUE:
> +               return netdev_nl_stats_by_queue(netdev, skb, info, ctx);
> +       }
> +
> +       return -EINVAL; /* Should not happen, per netlink policy */
> +}
> +
>  int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
>                                 struct netlink_callback *cb)
>  {
> @@ -646,6 +664,7 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
>         const struct genl_info *info = genl_info_dump(cb);
>         struct net *net = sock_net(skb->sk);
>         struct net_device *netdev;
> +       unsigned int ifindex;
>         unsigned int scope;
>         int err = 0;
>
> @@ -653,21 +672,28 @@ int netdev_nl_qstats_get_dumpit(struct sk_buff *skb,
>         if (info->attrs[NETDEV_A_QSTATS_SCOPE])
>                 scope = nla_get_uint(info->attrs[NETDEV_A_QSTATS_SCOPE]);
>
> -       rtnl_lock();
> -       for_each_netdev_dump(net, netdev, ctx->ifindex) {
> -               if (!netdev->stat_ops)
> -                       continue;
> +       ifindex = 0;
> +       if (info->attrs[NETDEV_A_QSTATS_IFINDEX])
> +               ifindex = nla_get_u32(info->attrs[NETDEV_A_QSTATS_IFINDEX]);
>
> -               switch (scope) {
> -               case 0:
> -                       err = netdev_nl_stats_by_netdev(netdev, skb, info);
> -                       break;
> -               case NETDEV_QSTATS_SCOPE_QUEUE:
> -                       err = netdev_nl_stats_by_queue(netdev, skb, info, ctx);
> -                       break;
> +       rtnl_lock();
> +       if (ifindex) {
> +               netdev = __dev_get_by_index(net, ifindex);

I wonder if NLM_F_DUMP_FILTERED should not be reported to user space ?

Re: [PATCH net-next 1/4] netdev: support dumping a single netdev in qstats

[David Ahern]

On 4/21/24 1:17 PM, Eric Dumazet wrote:
> I wonder if NLM_F_DUMP_FILTERED should not be reported to user space ?

good point. We do set that flag for other dumps when a filter has been
used to limit data returned.

Re: [PATCH net-next 1/4] netdev: support dumping a single netdev in qstats

[Jakub Kicinski]

On Sun, 21 Apr 2024 13:32:24 -0600 David Ahern wrote:
> On 4/21/24 1:17 PM, Eric Dumazet wrote:
> > I wonder if NLM_F_DUMP_FILTERED should not be reported to user space ?  
> 
> good point. We do set that flag for other dumps when a filter has been
> used to limit data returned.

That flag appears to be a, hm, historic workaround?
If I was to guess what the motivation was I'd say that it's because
"old school netlink" didn't reject unknown attributes. And you wanted
to know whether the kernel did the filtering or you have to filter
again in user space? Am I close? :)

The flag is mostly used in the IP stack, I'd rather try to deprecate 
it than propagate it to new genetlink families which do full input
validation, rendering the flag 100% unnecessary.

Re: [PATCH net-next 1/4] netdev: support dumping a single netdev in qstats

[Eric Dumazet]

On Mon, Apr 22, 2024 at 3:48 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 21 Apr 2024 13:32:24 -0600 David Ahern wrote:
> > On 4/21/24 1:17 PM, Eric Dumazet wrote:
> > > I wonder if NLM_F_DUMP_FILTERED should not be reported to user space ?
> >
> > good point. We do set that flag for other dumps when a filter has been
> > used to limit data returned.
>
> That flag appears to be a, hm, historic workaround?
> If I was to guess what the motivation was I'd say that it's because
> "old school netlink" didn't reject unknown attributes. And you wanted
> to know whether the kernel did the filtering or you have to filter
> again in user space? Am I close? :)
>
> The flag is mostly used in the IP stack, I'd rather try to deprecate
> it than propagate it to new genetlink families which do full input
> validation, rendering the flag 100% unnecessary.

SGTM

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next 1/4] netdev: support dumping a single netdev in qstats

[David Ahern]

On 4/22/24 7:48 AM, Jakub Kicinski wrote:
> On Sun, 21 Apr 2024 13:32:24 -0600 David Ahern wrote:
>> On 4/21/24 1:17 PM, Eric Dumazet wrote:
>>> I wonder if NLM_F_DUMP_FILTERED should not be reported to user space ?  
>>
>> good point. We do set that flag for other dumps when a filter has been
>> used to limit data returned.
> 
> That flag appears to be a, hm, historic workaround?
> If I was to guess what the motivation was I'd say that it's because
> "old school netlink" didn't reject unknown attributes. And you wanted
> to know whether the kernel did the filtering or you have to filter
> again in user space? Am I close? :)

close enough based on what I can recall.

> 
> The flag is mostly used in the IP stack, I'd rather try to deprecate 
> it than propagate it to new genetlink families which do full input
> validation, rendering the flag 100% unnecessary.

[PATCH net-next 2/4] netlink: move extack writing helpers

[Jakub Kicinski]

Next change will need them in netlink_dump_done(), pure move.

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

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index dc8c3c01d51b..c5bb09597831 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2165,6 +2165,69 @@ __nlmsg_put(struct sk_buff *skb, u32 portid, u32 seq, int type, int len, int fla
 }
 EXPORT_SYMBOL(__nlmsg_put);
 
+static size_t
+netlink_ack_tlv_len(struct netlink_sock *nlk, int err,
+		    const struct netlink_ext_ack *extack)
+{
+	size_t tlvlen;
+
+	if (!extack || !test_bit(NETLINK_F_EXT_ACK, &nlk->flags))
+		return 0;
+
+	tlvlen = 0;
+	if (extack->_msg)
+		tlvlen += nla_total_size(strlen(extack->_msg) + 1);
+	if (extack->cookie_len)
+		tlvlen += nla_total_size(extack->cookie_len);
+
+	/* Following attributes are only reported as error (not warning) */
+	if (!err)
+		return tlvlen;
+
+	if (extack->bad_attr)
+		tlvlen += nla_total_size(sizeof(u32));
+	if (extack->policy)
+		tlvlen += netlink_policy_dump_attr_size_estimate(extack->policy);
+	if (extack->miss_type)
+		tlvlen += nla_total_size(sizeof(u32));
+	if (extack->miss_nest)
+		tlvlen += nla_total_size(sizeof(u32));
+
+	return tlvlen;
+}
+
+static void
+netlink_ack_tlv_fill(struct sk_buff *in_skb, struct sk_buff *skb,
+		     struct nlmsghdr *nlh, int err,
+		     const struct netlink_ext_ack *extack)
+{
+	if (extack->_msg)
+		WARN_ON(nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg));
+	if (extack->cookie_len)
+		WARN_ON(nla_put(skb, NLMSGERR_ATTR_COOKIE,
+				extack->cookie_len, extack->cookie));
+
+	if (!err)
+		return;
+
+	if (extack->bad_attr &&
+	    !WARN_ON((u8 *)extack->bad_attr < in_skb->data ||
+		     (u8 *)extack->bad_attr >= in_skb->data + in_skb->len))
+		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_OFFS,
+				    (u8 *)extack->bad_attr - (u8 *)nlh));
+	if (extack->policy)
+		netlink_policy_dump_write_attr(skb, extack->policy,
+					       NLMSGERR_ATTR_POLICY);
+	if (extack->miss_type)
+		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_MISS_TYPE,
+				    extack->miss_type));
+	if (extack->miss_nest &&
+	    !WARN_ON((u8 *)extack->miss_nest < in_skb->data ||
+		     (u8 *)extack->miss_nest > in_skb->data + in_skb->len))
+		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_MISS_NEST,
+				    (u8 *)extack->miss_nest - (u8 *)nlh));
+}
+
 /*
  * It looks a bit ugly.
  * It would be better to create kernel thread.
@@ -2406,69 +2469,6 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(__netlink_dump_start);
 
-static size_t
-netlink_ack_tlv_len(struct netlink_sock *nlk, int err,
-		    const struct netlink_ext_ack *extack)
-{
-	size_t tlvlen;
-
-	if (!extack || !test_bit(NETLINK_F_EXT_ACK, &nlk->flags))
-		return 0;
-
-	tlvlen = 0;
-	if (extack->_msg)
-		tlvlen += nla_total_size(strlen(extack->_msg) + 1);
-	if (extack->cookie_len)
-		tlvlen += nla_total_size(extack->cookie_len);
-
-	/* Following attributes are only reported as error (not warning) */
-	if (!err)
-		return tlvlen;
-
-	if (extack->bad_attr)
-		tlvlen += nla_total_size(sizeof(u32));
-	if (extack->policy)
-		tlvlen += netlink_policy_dump_attr_size_estimate(extack->policy);
-	if (extack->miss_type)
-		tlvlen += nla_total_size(sizeof(u32));
-	if (extack->miss_nest)
-		tlvlen += nla_total_size(sizeof(u32));
-
-	return tlvlen;
-}
-
-static void
-netlink_ack_tlv_fill(struct sk_buff *in_skb, struct sk_buff *skb,
-		     struct nlmsghdr *nlh, int err,
-		     const struct netlink_ext_ack *extack)
-{
-	if (extack->_msg)
-		WARN_ON(nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg));
-	if (extack->cookie_len)
-		WARN_ON(nla_put(skb, NLMSGERR_ATTR_COOKIE,
-				extack->cookie_len, extack->cookie));
-
-	if (!err)
-		return;
-
-	if (extack->bad_attr &&
-	    !WARN_ON((u8 *)extack->bad_attr < in_skb->data ||
-		     (u8 *)extack->bad_attr >= in_skb->data + in_skb->len))
-		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_OFFS,
-				    (u8 *)extack->bad_attr - (u8 *)nlh));
-	if (extack->policy)
-		netlink_policy_dump_write_attr(skb, extack->policy,
-					       NLMSGERR_ATTR_POLICY);
-	if (extack->miss_type)
-		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_MISS_TYPE,
-				    extack->miss_type));
-	if (extack->miss_nest &&
-	    !WARN_ON((u8 *)extack->miss_nest < in_skb->data ||
-		     (u8 *)extack->miss_nest > in_skb->data + in_skb->len))
-		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_MISS_NEST,
-				    (u8 *)extack->miss_nest - (u8 *)nlh));
-}
-
 void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 		 const struct netlink_ext_ack *extack)
 {
-- 
2.44.0

Re: [PATCH net-next 2/4] netlink: move extack writing helpers

[David Ahern]

On 4/19/24 8:35 PM, Jakub Kicinski wrote:
> Next change will need them in netlink_dump_done(), pure move.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/netlink/af_netlink.c | 126 +++++++++++++++++++--------------------
>  1 file changed, 63 insertions(+), 63 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

[PATCH net-next 3/4] netlink: support all extack types in dumps

[Jakub Kicinski]

Note that when this commit message refers to netlink dump
it only means the actual dumping part, the parsing / dump
start is handled by the same code as "doit".

Commit 4a19edb60d02 ("netlink: Pass extack to dump handlers")
added support for returning extack messages from dump handlers,
but left out other extack info, e.g. bad attribute.

This used to be fine because until YNL we had little practical
use for the machine readable attributes, and only messages were
used in practice.

YNL flips the preference 180 degrees, it's now much more useful
to point to a bad attr with NL_SET_BAD_ATTR() than type
an English message saying "attribute XYZ is $reason-why-bad".

Support all of extack. The fact that extack only gets added if
it fits remains unaddressed.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/af_netlink.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c5bb09597831..fa9c090cf629 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2198,7 +2198,7 @@ netlink_ack_tlv_len(struct netlink_sock *nlk, int err,
 
 static void
 netlink_ack_tlv_fill(struct sk_buff *in_skb, struct sk_buff *skb,
-		     struct nlmsghdr *nlh, int err,
+		     const struct nlmsghdr *nlh, int err,
 		     const struct netlink_ext_ack *extack)
 {
 	if (extack->_msg)
@@ -2214,7 +2214,7 @@ netlink_ack_tlv_fill(struct sk_buff *in_skb, struct sk_buff *skb,
 	    !WARN_ON((u8 *)extack->bad_attr < in_skb->data ||
 		     (u8 *)extack->bad_attr >= in_skb->data + in_skb->len))
 		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_OFFS,
-				    (u8 *)extack->bad_attr - (u8 *)nlh));
+				    (u8 *)extack->bad_attr - (const u8 *)nlh));
 	if (extack->policy)
 		netlink_policy_dump_write_attr(skb, extack->policy,
 					       NLMSGERR_ATTR_POLICY);
@@ -2225,7 +2225,7 @@ netlink_ack_tlv_fill(struct sk_buff *in_skb, struct sk_buff *skb,
 	    !WARN_ON((u8 *)extack->miss_nest < in_skb->data ||
 		     (u8 *)extack->miss_nest > in_skb->data + in_skb->len))
 		WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_MISS_NEST,
-				    (u8 *)extack->miss_nest - (u8 *)nlh));
+				    (u8 *)extack->miss_nest - (const u8 *)nlh));
 }
 
 /*
@@ -2238,6 +2238,7 @@ static int netlink_dump_done(struct netlink_sock *nlk, struct sk_buff *skb,
 			     struct netlink_ext_ack *extack)
 {
 	struct nlmsghdr *nlh;
+	size_t extack_len;
 
 	nlh = nlmsg_put_answer(skb, cb, NLMSG_DONE, sizeof(nlk->dump_done_errno),
 			       NLM_F_MULTI | cb->answer_flags);
@@ -2247,10 +2248,14 @@ static int netlink_dump_done(struct netlink_sock *nlk, struct sk_buff *skb,
 	nl_dump_check_consistent(cb, nlh);
 	memcpy(nlmsg_data(nlh), &nlk->dump_done_errno, sizeof(nlk->dump_done_errno));
 
-	if (extack->_msg && test_bit(NETLINK_F_EXT_ACK, &nlk->flags)) {
+	extack_len = netlink_ack_tlv_len(nlk, nlk->dump_done_errno, extack);
+	if (extack_len) {
 		nlh->nlmsg_flags |= NLM_F_ACK_TLVS;
-		if (!nla_put_string(skb, NLMSGERR_ATTR_MSG, extack->_msg))
+		if (skb_tailroom(skb) >= extack_len) {
+			netlink_ack_tlv_fill(cb->skb, skb, cb->nlh,
+					     nlk->dump_done_errno, extack);
 			nlmsg_end(skb, nlh);
+		}
 	}
 
 	return 0;
-- 
2.44.0

Re: [PATCH net-next 3/4] netlink: support all extack types in dumps

[David Ahern]

On 4/19/24 8:35 PM, Jakub Kicinski wrote:
> Note that when this commit message refers to netlink dump
> it only means the actual dumping part, the parsing / dump
> start is handled by the same code as "doit".
> 
> Commit 4a19edb60d02 ("netlink: Pass extack to dump handlers")
> added support for returning extack messages from dump handlers,
> but left out other extack info, e.g. bad attribute.
> 
> This used to be fine because until YNL we had little practical
> use for the machine readable attributes, and only messages were
> used in practice.
> 
> YNL flips the preference 180 degrees, it's now much more useful
> to point to a bad attr with NL_SET_BAD_ATTR() than type
> an English message saying "attribute XYZ is $reason-why-bad".
> 
> Support all of extack. The fact that extack only gets added if
> it fits remains unaddressed.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/netlink/af_netlink.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

[PATCH net-next 4/4] selftests: drv-net: test dumping qstats per device

[Jakub Kicinski]

Add a test for dumping qstats device by device.

ksft framework grows a ksft_raises() helper, to be used
under with, which should be familiar to unittest users.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/stats.py | 62 +++++++++++++++++++-
 tools/testing/selftests/net/lib/py/ksft.py   | 18 ++++++
 2 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/stats.py b/tools/testing/selftests/drivers/net/stats.py
index 947df3eb681f..7a7b16b180e2 100755
--- a/tools/testing/selftests/drivers/net/stats.py
+++ b/tools/testing/selftests/drivers/net/stats.py
@@ -1,8 +1,8 @@
 #!/usr/bin/env python3
 # SPDX-License-Identifier: GPL-2.0
 
-from lib.py import ksft_run, ksft_exit
-from lib.py import ksft_in, ksft_true, KsftSkipEx, KsftXfailEx
+from lib.py import ksft_run, ksft_exit, ksft_pr
+from lib.py import ksft_ge, ksft_eq, ksft_in, ksft_true, ksft_raises, KsftSkipEx, KsftXfailEx
 from lib.py import EthtoolFamily, NetdevFamily, RtnlFamily, NlError
 from lib.py import NetDrvEnv
 
@@ -77,9 +77,65 @@ rtnl = RtnlFamily()
             raise Exception("Qstats are lower, fetched later")
 
 
+def qstat_by_ifindex(cfg) -> None:
+    global netfam
+    global rtnl
+
+    # Construct a map ifindex -> [dump, by-index, dump]
+    ifindexes = {}
+    stats = netfam.qstats_get({}, dump=True)
+    for entry in stats:
+        ifindexes[entry['ifindex']] = [entry, None, None]
+
+    for ifindex in ifindexes.keys():
+        entry = netfam.qstats_get({"ifindex": ifindex}, dump=True)
+        ksft_eq(len(entry), 1)
+        ifindexes[entry[0]['ifindex']][1] = entry[0]
+
+    stats = netfam.qstats_get({}, dump=True)
+    for entry in stats:
+        ifindexes[entry['ifindex']][2] = entry
+
+    if len(ifindexes) == 0:
+        raise KsftSkipEx("No ifindex supports qstats")
+
+    # Now make sure the stats match/make sense
+    for ifindex, triple in ifindexes.items():
+        all_keys = triple[0].keys() | triple[1].keys() | triple[2].keys()
+
+        for key in all_keys:
+            ksft_ge(triple[1][key], triple[0][key], comment="bad key: " + key)
+            ksft_ge(triple[2][key], triple[1][key], comment="bad key: " + key)
+
+    # Test invalid dumps
+    # 0 is invalid
+    with ksft_raises(NlError) as cm:
+        netfam.qstats_get({"ifindex": 0}, dump=True)
+    ksft_eq(cm.exception.nl_msg.error, -34)
+    ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.ifindex')
+
+    # loopback has no stats
+    with ksft_raises(NlError) as cm:
+        netfam.qstats_get({"ifindex": 1}, dump=True)
+    ksft_eq(cm.exception.nl_msg.error, -95)
+    ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.ifindex')
+
+    # Try to get stats for lowest unused ifindex but not 0
+    devs = rtnl.getlink({}, dump=True)
+    all_ifindexes = set([dev["ifi-index"] for dev in devs])
+    lowest = 2
+    while lowest in all_ifindexes:
+        lowest += 1
+
+    with ksft_raises(NlError) as cm:
+        netfam.qstats_get({"ifindex": lowest}, dump=True)
+    ksft_eq(cm.exception.nl_msg.error, -19)
+    ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.ifindex')
+
+
 def main() -> None:
     with NetDrvEnv(__file__) as cfg:
-        ksft_run([check_pause, check_fec, pkt_byte_sum],
+        ksft_run([check_pause, check_fec, pkt_byte_sum, qstat_by_ifindex],
                  args=(cfg, ))
     ksft_exit()
 
diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index 25f2572fa540..e7f79f6185b0 100644
--- a/tools/testing/selftests/net/lib/py/ksft.py
+++ b/tools/testing/selftests/net/lib/py/ksft.py
@@ -53,6 +53,24 @@ KSFT_RESULT_ALL = True
         _fail("Check failed", a, "<", b, comment)
 
 
+class ksft_raises:
+    def __init__(self, expected_type):
+        self.exception = None
+        self.expected_type = expected_type
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        if exc_type is None:
+            _fail(f"Expected exception {str(self.expected_type.__name__)}, none raised")
+        elif self.expected_type != exc_type:
+            _fail(f"Expected exception {str(self.expected_type.__name__)}, raised {str(exc_type.__name__)}")
+        self.exception = exc_val
+        # Suppress the exception if its the expected one
+        return self.expected_type == exc_type
+
+
 def ksft_busy_wait(cond, sleep=0.005, deadline=1, comment=""):
     end = time.monotonic() + deadline
     while True:
-- 
2.44.0

Re: [PATCH net-next 0/4] netdev: support dumping a single netdev in qstats

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 19 Apr 2024 19:35:38 -0700 you wrote:
> I was writing a test for page pool which depended on qstats,
> and got tired of having to filter dumps in user space.
> Add support for dumping stats for a single netdev.
> 
> To get there we first need to add full support for extack
> in dumps (and fix a dump error handling bug in YNL, sent
> separately to the net tree).
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] netdev: support dumping a single netdev in qstats
    https://git.kernel.org/netdev/net-next/c/ce05d0f20368
  - [net-next,2/4] netlink: move extack writing helpers
    https://git.kernel.org/netdev/net-next/c/652332e3f1d6
  - [net-next,3/4] netlink: support all extack types in dumps
    https://git.kernel.org/netdev/net-next/c/8af4f60472fc
  - [net-next,4/4] selftests: drv-net: test dumping qstats per device
    https://git.kernel.org/netdev/net-next/c/237109259283

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