| Message ID | 20231113092754.3285306-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] bonding: use WARN_ON instead of BUG in alb_upper_dev_walk | expand |
Zhengchao Shao <shaozhengchao@huawei.com> wrote: >If failed to allocate "tags" or could not find the final upper device from >start_dev's upper list in bond_verify_device_path(), only the loopback >detection of the current upper device should be affected, and the system is >no need to be panic. > >Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >--- > drivers/net/bonding/bond_alb.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c >index dc2c7b979656..5519cc95b966 100644 >--- a/drivers/net/bonding/bond_alb.c >+++ b/drivers/net/bonding/bond_alb.c >@@ -984,8 +984,10 @@ static int alb_upper_dev_walk(struct net_device *upper, > */ > if (netif_is_macvlan(upper) && !strict_match) { > tags = bond_verify_device_path(bond->dev, upper, 0); >- if (IS_ERR_OR_NULL(tags)) >- BUG(); >+ if (IS_ERR_OR_NULL(tags)) { >+ WARN_ON(1); >+ return 0; This seems reasonable enough, although I'd suggest the using WARN_ON_ONCE instead of WARN_ON. Alternatively, this could stay as WARN_ON if the above also returns non-zero in order to terminate the netdev_walk_all_upper_dev_rcu walk. The intent here is to avoid spamming the log if there's a lot of macvlans above the bond. If the allocation in bond_verify_device_path failed, trying again immediately seems likely to fail as well. We could also arrange for whatever called alb_upper_dev_walk to reschedule at a slightly later time, but I don't think that's worth the trouble. The bond will by default resend learning packets once per second, so issues related to a lost learning packet should resolve relatively quickly. -J >+ } > alb_send_lp_vid(slave, upper->dev_addr, > tags[0].vlan_proto, tags[0].vlan_id); > kfree(tags); >-- >2.34.1 --- -Jay Vosburgh, jay.vosburgh@canonical.com
On 2023/11/14 8:31, Jay Vosburgh wrote: > Zhengchao Shao <shaozhengchao@huawei.com> wrote: > >> If failed to allocate "tags" or could not find the final upper device from >> start_dev's upper list in bond_verify_device_path(), only the loopback >> detection of the current upper device should be affected, and the system is >> no need to be panic. >> >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> drivers/net/bonding/bond_alb.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c >> index dc2c7b979656..5519cc95b966 100644 >> --- a/drivers/net/bonding/bond_alb.c >> +++ b/drivers/net/bonding/bond_alb.c >> @@ -984,8 +984,10 @@ static int alb_upper_dev_walk(struct net_device *upper, >> */ >> if (netif_is_macvlan(upper) && !strict_match) { >> tags = bond_verify_device_path(bond->dev, upper, 0); >> - if (IS_ERR_OR_NULL(tags)) >> - BUG(); >> + if (IS_ERR_OR_NULL(tags)) { >> + WARN_ON(1); >> + return 0; > > This seems reasonable enough, although I'd suggest the using > WARN_ON_ONCE instead of WARN_ON. Alternatively, this could stay as > WARN_ON if the above also returns non-zero in order to terminate the > netdev_walk_all_upper_dev_rcu walk. The intent here is to avoid > spamming the log if there's a lot of macvlans above the bond. If the > allocation in bond_verify_device_path failed, trying again immediately > seems likely to fail as well. Hi Jay: Thank you for your reply. I do agree with you. I will send v2. Zhengchao Shao > > We could also arrange for whatever called alb_upper_dev_walk to > reschedule at a slightly later time, but I don't think that's worth the > trouble. The bond will by default resend learning packets once per > second, so issues related to a lost learning packet should resolve > relatively quickly. > > -J > >> + } >> alb_send_lp_vid(slave, upper->dev_addr, >> tags[0].vlan_proto, tags[0].vlan_id); >> kfree(tags); >> -- >> 2.34.1 > > --- > -Jay Vosburgh, jay.vosburgh@canonical.com
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index dc2c7b979656..5519cc95b966 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -984,8 +984,10 @@ static int alb_upper_dev_walk(struct net_device *upper, */ if (netif_is_macvlan(upper) && !strict_match) { tags = bond_verify_device_path(bond->dev, upper, 0); - if (IS_ERR_OR_NULL(tags)) - BUG(); + if (IS_ERR_OR_NULL(tags)) { + WARN_ON(1); + return 0; + } alb_send_lp_vid(slave, upper->dev_addr, tags[0].vlan_proto, tags[0].vlan_id); kfree(tags);
If failed to allocate "tags" or could not find the final upper device from start_dev's upper list in bond_verify_device_path(), only the loopback detection of the current upper device should be affected, and the system is no need to be panic. Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- drivers/net/bonding/bond_alb.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)