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
| ||
|
Message-ID: <cc269865-d3c7-f8e6-9a61-25794f5ae220@iogearbox.net> Date: Mon, 13 Nov 2023 14:05:36 +0100 From: Daniel Borkmann <daniel@...earbox.net> To: Simon Horman <horms@...nel.org> 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 11/13/23 11:03 AM, Simon Horman wrote: > 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. Good point, I'll add a guard in case someone tries to set an invalid value outside of the enum. >> +} >> + > > ... > >> @@ -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. I'm not sure which one you mean? Thanks, Daniel
Powered by blists - more mailing lists