netdev - Re: [PATCH net-next] net: make for_each_netdev

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]

Message-ID: <5324a503-3a9f-4bb0-ae73-df23dac5e258@intel.com>
Date: Fri, 14 Jun 2024 12:36:53 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>
Subject: Re: [PATCH net-next] net: make for_each_netdev_dump() a little more
 bug-proof

On 6/13/24 23:33, Jakub Kicinski wrote:
> I find the behavior of xa_for_each_start() slightly counter-intuitive.
> It doesn't end the iteration by making the index point after the last
> element. IOW calling xa_for_each_start() again after it "finished"
> will run the body of the loop for the last valid element, instead
> of doing nothing.
> 
> This works fine for netlink dumps if they terminate correctly
> (i.e. coalesce or carefully handle NLM_DONE), but as we keep getting
> reminded legacy dumps are unlikely to go away.
> 
> Fixing this generically at the xa_for_each_start() level seems hard -
> there is no index reserved for "end of iteration".
> ifindexes are 31b wide, tho, and iterator is ulong so for
> for_each_netdev_dump() it's safe to go to the next element.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>   include/linux/netdevice.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index f148a01dd1d1..85111502cf8f 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3021,7 +3021,8 @@ int call_netdevice_notifiers_info(unsigned long val,
>   #define net_device_entry(lh)	list_entry(lh, struct net_device, dev_list)
>   
>   #define for_each_netdev_dump(net, d, ifindex)				\
> -	xa_for_each_start(&(net)->dev_by_index, (ifindex), (d), (ifindex))
> +	for (; (d = xa_find(&(net)->dev_by_index, &ifindex,		\
> +			    ULONG_MAX, XA_PRESENT)); ifindex++)
>   
>   static inline struct net_device *next_net_device(struct net_device *dev)
>   {

Makes sense,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>