netdev - Re: [patch net-next v8 2/3] net: core: Add offload stats to if_stats

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Sep 2016 16:36:19 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>, nogahf@...lanox.com,
        idosch@...lanox.com, eladr@...lanox.com, yotamg@...lanox.com,
        ogerlitz@...lanox.com, roopa@...ulusnetworks.com,
        linville@...driver.com, tgraf@...g.ch, gospo@...ulusnetworks.com,
        sfeldma@...il.com, sd@...asysnail.net, eranbe@...lanox.com,
        ast@...mgrid.com, edumazet@...gle.com, hannes@...essinduktion.org,
        f.fainelli@...il.com, dsa@...ulusnetworks.com
Subject: Re: [patch net-next v8 2/3] net: core: Add offload stats to if_stats_msg


> On Sep 8, 2016, at 9:19 AM, Jiri Pirko <jiri@...nulli.us> wrote:
> 
> From: Nogah Frankel <nogahf@...lanox.com>
> 
> Add a nested attribute of offload stats to if_stats_msg
> named IFLA_STATS_LINK_OFFLOAD_XSTATS.
> Under it, add SW stats, meaning stats only per packets that went via
> slowpath to the cpu, named IFLA_OFFLOAD_XSTATS_CPU_HIT.
> 
> Signed-off-by: Nogah Frankel <nogahf@...lanox.com>
> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
> ---
> include/uapi/linux/if_link.h |  9 ++++
> net/core/rtnetlink.c         | 97 ++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 102 insertions(+), 4 deletions(-)
> 
[snip]
> @@ -3655,6 +3725,22 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
> 		}
> 	}
> 
> +	if (stats_attr_valid(filter_mask, IFLA_STATS_LINK_OFFLOAD_XSTATS,
> +			     *idxattr)) {
> +		attr = nla_nest_start(skb, IFLA_STATS_LINK_OFFLOAD_XSTATS);
> +		if (!attr)
> +			goto nla_put_failure;
> +
> +		err = rtnl_get_offload_stats(skb, dev);
> +		if (err == -ENODATA)
> +			nla_nest_cancel(skb, attr);
> +		else
> +			nla_nest_end(skb, attr);
> +
> +		if ((err) && (err != -ENODATA))
> +			goto nla_put_failure;
> +	}
> +

Hmm, actually on a second read I think there’s a potential problem here. Since you don’t set *idxattr and if the space
isn’t enough the dump will get restarted and this will lead to an infinite loop.
I.e. if the previous attributes filled the skb and there’s not enough room for this one, it will return an error
but *idxattr will be == 0 from the previous attribute thus the whole dump will be restarted (this is in case someone
requests this attribute with some of the others of course).

Cheers,
 Nik


> 	nlmsg_end(skb, nlh);
> 
> 	return 0;
> @@ -3712,6 +3798,9 @@ 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_get_offload_stats_size(dev);
> +
> 	return size;
> }
> 
> -- 
> 2.5.5
>