| Message ID | 20231114091829.2509952-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] bonding: use WARN_ON_ONCE instead of BUG in alb_upper_dev_walk | expand |
On Tue, Nov 14, 2023 at 05:18:29PM +0800, Zhengchao Shao 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. > Using WARN_ON_ONCE here is to avoid spamming the log if there's a lot of > macvlans above the bond. > > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > v2: use WARN_ON_ONCE instead of WARN_ON > --- > 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..a7bad0fff8cb 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_ONCE(1); > + return 0; Return 0 for an error looks weird. Should we keep walking the list if allocate "tags" failed? Thanks Hangbin
On 2023/11/14 18:16, Hangbin Liu wrote: > On Tue, Nov 14, 2023 at 05:18:29PM +0800, Zhengchao Shao 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. >> Using WARN_ON_ONCE here is to avoid spamming the log if there's a lot of >> macvlans above the bond. >> >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> v2: use WARN_ON_ONCE instead of WARN_ON >> --- >> 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..a7bad0fff8cb 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_ONCE(1); >> + return 0; > > Return 0 for an error looks weird. Should we keep walking the list if > allocate "tags" failed? > > Thanks > Hangbin > Hi Hangbin: I think minimizing the impact of a single allocation failure is OK. Zhengchao Shao
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. >Using WARN_ON_ONCE here is to avoid spamming the log if there's a lot of >macvlans above the bond. > >Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >--- >v2: use WARN_ON_ONCE instead of WARN_ON >--- > 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..a7bad0fff8cb 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_ONCE(1); >+ return 0; Ok, I know this is what I said, but on reflection, I think this should really return non-zero to terminate the device walk. -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/15 3:40, 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. >> Using WARN_ON_ONCE here is to avoid spamming the log if there's a lot of >> macvlans above the bond. >> >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> v2: use WARN_ON_ONCE instead of WARN_ON >> --- >> 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..a7bad0fff8cb 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_ONCE(1); >> + return 0; > > Ok, I know this is what I said, but on reflection, I think this > should really return non-zero to terminate the device walk. > > -J > After this failure, there is a high probability that walk process will still fail. Therefore, it is OK to exit directly. Thanks. I will send V3. Zhengchao Shao > >> + } >> 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..a7bad0fff8cb 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_ONCE(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. Using WARN_ON_ONCE here is to avoid spamming the log if there's a lot of macvlans above the bond. Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- v2: use WARN_ON_ONCE instead of WARN_ON --- drivers/net/bonding/bond_alb.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)