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

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]

Date: Thu, 18 May 2023 02:28:29 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
 jiri@...nulli.us, j.vosburgh@...il.com, andy@...yhouse.net,
 netdev@...r.kernel.org, jarod@...hat.com, razor@...ckwall.org,
 simon.horman@...igine.com, wangyufen@...wei.com,
 syzbot+60748c96cf5c6df8e581@...kaller.appspotmail.com
Subject: Re: [PATCH net v2] net: fix stack overflow when LRO is disabled for
 virtual interfaces

On 5/18/23 01:15, Jakub Kicinski wrote:

Hi Jakub,
Thank you so much for the review!

 > On Wed, 17 May 2023 14:30:10 +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.
 >
 > Why doesn't the (already synchronized) upper not skip the update?

The skipping logic of this is existing in the netdev_sync_lower_features().
The purpose of this is to synchronize the lower interfaces, not the 
upper interface.
Actually, there is no upper-only synchronization logic.

Both bonding and team interfaces rely on notification mechanisms to work 
their own logic such as synchronization.
The notification is a broadcasting mechanism.
So, both lower and upper receive this event, and it works its own 
notification handling.
But the notification mechanism currently doesn't have options such as 
filtering and these interfaces receive this event with updated feature 
flags.
So, the upper interface can't distinguish whether the received event is 
the first event or duplicated event.

 >
 >> 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 notifier_ctx member of bonding/team is 
introduced.
 >

Thank you so much!
Taehee Yoo