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: <d6e29acc-c759-48ce-bea2-3088b4d3ea86@intel.com>
Date: Fri, 14 Jun 2024 03:45:47 +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, 

you are right, it would be easier to go one step [of macros] up, we have
  453│ #define xa_for_each_range(xa, index, entry, start, last)         \
  454│         for (index = start,                                      \
  455│              entry = xa_find(xa, &index, last, XA_PRESENT);      \
  456│              entry;                                              \
  457│              entry = xa_find_after(xa, &index, last, XA_PRESENT))

You could simply change L456 to:
entry || (index = 0);
to achieve what you want; but that would slow down a little lot's of
places, only to change value of the index that should not be used :P

For me a proper solution would be to fast forward into C11 era, and move
@index allocation into the loop, so value could not be abused.

but that is a lot of usage code, so I'm not against your current patch

> 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)
>   {