| Message ID | 20240420023543.3300306-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ce05d0f20368b583b43c99a7c8673e8a7187b76b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdev: support dumping a single netdev in qstats | expand |
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>
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 ?
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.
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.
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>
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.
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();
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(-)