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

[Petr Machata <petrm@nvidia.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 24 Feb 2022 15:33:24 +0200 Ido Schimmel wrote:
>
>> @@ -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?

You are right:

        if (strict_start_type && type >= strict_start_type)
                validate |= NL_VALIDATE_STRICT;

This flag is there to begin with in non-_depreceted calls.

I'll drop the strict_start_type.

>> +	[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.

Nice, I'll add it.

>> +};
>> +
>> +#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?

The bitfield validates the mask as well, thereby making sure that
userspace and the kernel are in sync WRT which bits are meaningful.

Specifically in case of filtering, all meaningful bits are always going
to be the set ones. So it should be OK to just handroll the check that
value doesn't include any bits that we don't know about, and we don't
really need the mask.

So I can redo this as u32 if you prefer.