[PATCH net-next] net: make for_each_netdev_dump() a little more bug-proof

[PATCH net-next] net: make for_each_netdev_dump() a little more bug-proof

[Jakub Kicinski]

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@kernel.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)
 {
-- 
2.45.2

Re: [PATCH net-next] net: make for_each_netdev_dump() a little more bug-proof

[Przemek Kitszel]

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@kernel.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)
>   {

Re: [PATCH net-next] net: make for_each_netdev_dump() a little more bug-proof

[Jakub Kicinski]

On Fri, 14 Jun 2024 03:45:47 +0200 Przemek Kitszel wrote:
> 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.

I think we're already in C11 era for that exact reason 🤐️
But please don't take this as an invitation to do crazy things!

In netlink dumps, tho, we keep the index in persistent socket context,
because the iteration is split into multiple recvmsg() calls,
each one continuing where the previous one left off.

Re: [PATCH net-next] net: make for_each_netdev_dump() a little more bug-proof

[Przemek Kitszel]

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@kernel.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@intel.com>

Re: [PATCH net-next] net: make for_each_netdev_dump() a little more bug-proof

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 13 Jun 2024 14:33:16 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [net-next] net: make for_each_netdev_dump() a little more bug-proof
    https://git.kernel.org/netdev/net-next/c/f22b4b55edb5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html