netdev - Re: [PATCH net-next 03/14] net: rtnetlink: RTM_GETSTATS: Allow filtering inside nests

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]

Date:   Thu, 24 Feb 2022 22:14:47 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Ido Schimmel <idosch@...dia.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, petrm@...dia.com,
        jiri@...dia.com, razor@...ckwall.org, roopa@...dia.com,
        dsahern@...il.com, andrew@...n.ch, mlxsw@...dia.com
Subject: Re: [PATCH net-next 03/14] net: rtnetlink: RTM_GETSTATS: Allow
 filtering inside nests

On Thu, 24 Feb 2022 15:33:24 +0200 Ido Schimmel wrote:
> From: Petr Machata <petrm@...dia.com>
> 
> The filter_mask field of RTM_GETSTATS header determines which top-level
> attributes should be included in the netlink response. This saves
> processing time by only including the bits that the user cares about
> instead of always dumping everything. This is doubly important for
> HW-backed statistics that would typically require a trip to the device to
> fetch the stats.
> 
> So far there was only one HW-backed stat suite per attribute. However,
> IFLA_STATS_LINK_OFFLOAD_XSTATS is a nest, and will gain a new stat suite in
> the following patches. It would therefore be advantageous to be able to
> filter within that nest, and select just one or the other HW-backed
> statistics suite.
> 
> Extend rtnetlink so that RTM_GETSTATS permits attributes in the payload.
> The scheme is as follows:
> 
>     - RTM_GETSTATS
> 	- struct if_stats_msg
> 	- attr nest IFLA_STATS_GET_FILTERS
> 	    - attr IFLA_STATS_LINK_OFFLOAD_XSTATS
> 		- struct nla_bitfield32 filter_mask
> 
> This scheme reuses the existing enumerators by nesting them in a dedicated
> context attribute. This is covered by policies as usual, therefore a
> gradual opt-in is possible. Currently only IFLA_STATS_LINK_OFFLOAD_XSTATS
> nest has filtering enabled, because for the SW counters the issue does not
> seem to be that important.
> 
> rtnl_offload_xstats_get_size() and _fill() are extended to observe the
> requested filters.
> 
> Signed-off-by: Petr Machata <petrm@...dia.com>
> Signed-off-by: Ido Schimmel <idosch@...dia.com>

> @@ -5319,8 +5339,12 @@ static size_t if_nlmsg_stats_size(const struct net_device *dev,
>  		}
>  	}
>  
> -	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS, 0))
> -		size += rtnl_offload_xstats_get_size(dev);
> +	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS, 0)) {
> +		u32 off_filter_mask;
> +
> +		off_filter_mask = filters->mask[IFLA_STATS_LINK_OFFLOAD_XSTATS];
> +		size += rtnl_offload_xstats_get_size(dev, off_filter_mask);
> +	}
>  
>  	if (stats_attr_valid(filter_mask, IFLA_STATS_AF_SPEC, 0)) {
>  		struct rtnl_af_ops *af_ops;
> @@ -5344,6 +5368,75 @@ static size_t if_nlmsg_stats_size(const struct net_device *dev,
>  	return size;
>  }
>  
> +static const struct nla_policy
> +rtnl_stats_get_policy[IFLA_STATS_GETSET_MAX + 1] = {
> +	[IFLA_STATS_GETSET_UNSPEC] = { .strict_start_type = 1 },

I don't think we need the .strict_start_type if the policy is not used
in parse calls with a _deprecated() suffix, no?

> +	[IFLA_STATS_GET_FILTERS] = { .type = NLA_NESTED },

NLA_POLICY_NESTED()? Maybe one day we'll have policy dumping 
for rtnetlink and it'll be useful to have policies linked up.

> +};
> +
> +#define RTNL_STATS_OFFLOAD_XSTATS_VALID ((1 << __IFLA_OFFLOAD_XSTATS_MAX) - 1)
> +
> +static const struct nla_policy
> +rtnl_stats_get_policy_filters[IFLA_STATS_MAX + 1] = {
> +	[IFLA_STATS_UNSPEC] = { .strict_start_type = 1 },
> +	[IFLA_STATS_LINK_OFFLOAD_XSTATS] =
> +			NLA_POLICY_BITFIELD32(RTNL_STATS_OFFLOAD_XSTATS_VALID),
> +};
> +
> +static int rtnl_stats_get_parse_filters(struct nlattr *ifla_filters,
> +					struct rtnl_stats_dump_filters *filters,
> +					struct netlink_ext_ack *extack)
> +{
> +	struct nlattr *tb[IFLA_STATS_MAX + 1];
> +	int err;
> +	int at;
> +
> +	err = nla_parse_nested(tb, IFLA_STATS_MAX, ifla_filters,
> +			       rtnl_stats_get_policy_filters, extack);
> +	if (err < 0)
> +		return err;
> +
> +	for (at = 1; at <= IFLA_STATS_MAX; at++) {
> +		if (tb[at]) {
> +			if (!(filters->mask[0] & IFLA_STATS_FILTER_BIT(at))) {
> +				NL_SET_ERR_MSG(extack, "Filtered attribute not enabled in filter_mask");
> +				return -EINVAL;
> +			}
> +			filters->mask[at] = nla_get_bitfield32(tb[at]).value;

Why use bitfield if we only use the .value, a u32 would do?

> +		}
> +	}