netdev - Re: [PATCH bpf v2 2/8] net: Move {l,t,d}stats allocation to core and convert veth & vrf

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: Mon, 13 Nov 2023 10:03:05 +0000
From: Simon Horman <horms@...nel.org>
To: Daniel Borkmann <daniel@...earbox.net>
Cc: martin.lau@...nel.org, kuba@...nel.org, razor@...ckwall.org,
	sdf@...gle.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
	David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH bpf v2 2/8] net: Move {l,t,d}stats allocation to core and
 convert veth & vrf

On Sun, Nov 12, 2023 at 09:30:03PM +0100, Daniel Borkmann wrote:
> Move {l,t,d}stats allocation to the core and let netdevs pick the stats
> type they need. That way the driver doesn't have to bother with error
> handling (allocation failure checking, making sure free happens in the
> right spot, etc) - all happening in the core.
> 
> Co-developed-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
> Cc: David Ahern <dsahern@...nel.org>

Hi Daniel,

sorry I was a bit to hasty in hitting the send button for my previous
message. I have a some more minor feedback on this series.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0d548431f3fa..75db81496db5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10049,6 +10049,44 @@ void netif_tx_stop_all_queues(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(netif_tx_stop_all_queues);
>  
> +static int netdev_do_alloc_pcpu_stats(struct net_device *dev)
> +{
> +	void __percpu *v;
> +
> +	switch (dev->pcpu_stat_type) {
> +	case NETDEV_PCPU_STAT_NONE:
> +		return 0;
> +	case NETDEV_PCPU_STAT_LSTATS:
> +		v = dev->lstats = netdev_alloc_pcpu_stats(struct pcpu_lstats);
> +		break;
> +	case NETDEV_PCPU_STAT_TSTATS:
> +		v = dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
> +		break;
> +	case NETDEV_PCPU_STAT_DSTATS:
> +		v = dev->dstats = netdev_alloc_pcpu_stats(struct pcpu_dstats);
> +		break;
> +	}
> +
> +	return v ? 0 : -ENOMEM;

Perhaps this cannot happen, but if none of the cases in the switch
statement are met, then v will be uninitialised here.

As flagged by Smatch.

> +}
> +

...

> @@ -10469,6 +10513,7 @@ void netdev_run_todo(void)
>  		WARN_ON(rcu_access_pointer(dev->ip_ptr));
>  		WARN_ON(rcu_access_pointer(dev->ip6_ptr));
>  
> +		netdev_do_free_pcpu_stats(dev);
>  		if (dev->priv_destructor)
>  			dev->priv_destructor(dev);
>  		if (dev->needs_free_netdev)

nit: the hunk above seems unnecessary; one blank line is enough.