Re: [PATCH net] net: fix stack overflow when LRO is disabled for virtual interfaces

[Taehee Yoo <ap420073@gmail.com>]

On 5/15/23 22:11, Simon Horman wrote:
Hi Simon,
Thank you so much for the review!

 > On Mon, May 15, 2023 at 05:37:40AM +0000, Taehee Yoo wrote:
 >> When the virtual interface's feature is updated, it synchronizes the
 >> updated feature for its own lower interface.
 >> This propagation logic should be worked as the iteration, not 
recursively.
 >> But it works recursively due to the netdev notification unexpectedly.
 >> This problem occurs when it disables LRO only for the team and bonding
 >> interface type.
 >>
 >>         team0
 >>           |
 >>    +------+------+-----+-----+
 >>    |      |      |     |     |
 >> team1  team2  team3  ...  team200
 >>
 >> If team0's LRO feature is updated, it generates the NETDEV_FEAT_CHANGE
 >> event to its own lower interfaces(team1 ~ team200).
 >> It is worked by netdev_sync_lower_features().
 >> So, the NETDEV_FEAT_CHANGE notification logic of each lower interface
 >> work iteratively.
 >> But generated NETDEV_FEAT_CHANGE event is also sent to the upper
 >> interface too.
 >> upper interface(team0) generates the NETDEV_FEAT_CHANGE event for 
its own
 >> lower interfaces again.
 >> lower and upper interfaces receive this event and generate this
 >> event again and again.
 >> So, the stack overflow occurs.
 >>
 >> But it is not the infinite loop issue.
 >> Because the netdev_sync_lower_features() updates features before
 >> generating the NETDEV_FEAT_CHANGE event.
 >> Already synchronized lower interfaces skip notification logic.
 >> So, it is just the problem that iteration logic is changed to the
 >> recursive unexpectedly due to the notification mechanism.
 >>
 >> Reproducer:
 >>
 >> ip link add team0 type team
 >> ethtool -K team0 lro on
 >> for i in {1..200}
 >> do
 >>          ip link add team$i master team0 type team
 >>          ethtool -K team$i lro on
 >> done
 >>
 >> ethtool -K team0 lro off
 >>
 >> In order to fix it, the priv_notifier_ctx net_device member is 
introduced.
 >> This variable can be used by each interface in its own way in the
 >> notification context. The bonding and team interface is going to use it
 >> to avoid duplicated NETDEV_FEAT_CHANGE event handling.
 >>
 >> Reported-by: syzbot+60748c96cf5c6df8e581@syzkaller.appspotmail.com
 >> Fixes: fd867d51f889 ("net/core: generic support for disabling netdev 
features down stack")
 >> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
 >
 > ...
 >
 >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
 >> index 08fbd4622ccf..ebd49a54f0d5 100644
 >> --- a/include/linux/netdevice.h
 >> +++ b/include/linux/netdevice.h
 >> @@ -2393,6 +2393,7 @@ struct net_device {
 >>   	unsigned		threaded:1;
 >>
 >>   	struct list_head	net_notifier_list;
 >> +	u32			priv_notifier_ctx;
 >
 > Hi Taehee,
 >
 > Please add this new field to the kdoc for struct net_device.
 >

Thanks! I will check this before submitting the v2 patch.

Thank you so much,
Taehee Yoo


 >>
 >>   #if IS_ENABLED(CONFIG_MACSEC)
 >>   	/* MACsec management functions */
 >
 > ...
 >
 > ---
 > pw-bot: cr