| Message ID | 20231118081653.1481260-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v4] bonding: return -ENOMEM 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. >So return -ENOMEM in alb_upper_dev_walk to stop walking, print some warn >information when failed to allocate memory for vlan tags in >bond_verify_device_path. > >I also think that the following function calls >netdev_walk_all_upper_dev_rcu >---->>>alb_upper_dev_walk >---------->>>bond_verify_device_path >From this way, "end device" can eventually be obtained from "start device" >in bond_verify_device_path, IS_ERR(tags) could be instead of >IS_ERR_OR_NULL(tags) in alb_upper_dev_walk. > >Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com> >--- >v4: print information instead of warn >v3: return -ENOMEM instead of zero to stop walk >v2: use WARN_ON_ONCE instead of WARN_ON >--- > drivers/net/bonding/bond_alb.c | 3 ++- > drivers/net/bonding/bond_main.c | 5 ++++- > 2 files changed, 6 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c >index dc2c7b979656..7edf0fd58c34 100644 >--- a/drivers/net/bonding/bond_alb.c >+++ b/drivers/net/bonding/bond_alb.c >@@ -985,7 +985,8 @@ 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(); >+ return -ENOMEM; >+ > alb_send_lp_vid(slave, upper->dev_addr, > tags[0].vlan_proto, tags[0].vlan_id); > kfree(tags); >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 51d47eda1c87..1a40bd08f984 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -2967,8 +2967,11 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev, > > if (start_dev == end_dev) { > tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC); >- if (!tags) >+ if (!tags) { >+ net_err_ratelimited("%s: %s: Failed to allocate tags\n", >+ __func__, start_dev->name); > return ERR_PTR(-ENOMEM); >+ } > tags[level].vlan_proto = BOND_VLAN_PROTO_NONE; > return tags; > } >-- >2.34.1 > >
On Sat, 2023-11-18 at 16:16 +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. > So return -ENOMEM in alb_upper_dev_walk to stop walking, print some warn > information when failed to allocate memory for vlan tags in > bond_verify_device_path. > > I also think that the following function calls > netdev_walk_all_upper_dev_rcu > ---->>>alb_upper_dev_walk > ---------->>>bond_verify_device_path > > From this way, "end device" can eventually be obtained from "start device" > in bond_verify_device_path, IS_ERR(tags) could be instead of > IS_ERR_OR_NULL(tags) in alb_upper_dev_walk. > > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > v4: print information instead of warn > v3: return -ENOMEM instead of zero to stop walk > v2: use WARN_ON_ONCE instead of WARN_ON > --- > drivers/net/bonding/bond_alb.c | 3 ++- > drivers/net/bonding/bond_main.c | 5 ++++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c > index dc2c7b979656..7edf0fd58c34 100644 > --- a/drivers/net/bonding/bond_alb.c > +++ b/drivers/net/bonding/bond_alb.c > @@ -985,7 +985,8 @@ 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(); > + return -ENOMEM; > + > alb_send_lp_vid(slave, upper->dev_addr, > tags[0].vlan_proto, tags[0].vlan_id); > kfree(tags); > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index 51d47eda1c87..1a40bd08f984 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -2967,8 +2967,11 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev, > > if (start_dev == end_dev) { > tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC); > - if (!tags) > + if (!tags) { > + net_err_ratelimited("%s: %s: Failed to allocate tags\n", > + __func__, start_dev->name); I'm sorry for the repeated back and forth, but checkpatch points out that the above warning adds little value: if the memory allocation fails, the mm layer will emit a lot warning comprising the backtrace. I suggest to drop it, thanks! Paolo
On 2023/11/21 19:41, Paolo Abeni wrote: > On Sat, 2023-11-18 at 16:16 +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. >> So return -ENOMEM in alb_upper_dev_walk to stop walking, print some warn >> information when failed to allocate memory for vlan tags in >> bond_verify_device_path. >> >> I also think that the following function calls >> netdev_walk_all_upper_dev_rcu >> ---->>>alb_upper_dev_walk >> ---------->>>bond_verify_device_path >>> From this way, "end device" can eventually be obtained from "start device" >> in bond_verify_device_path, IS_ERR(tags) could be instead of >> IS_ERR_OR_NULL(tags) in alb_upper_dev_walk. >> >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> v4: print information instead of warn >> v3: return -ENOMEM instead of zero to stop walk >> v2: use WARN_ON_ONCE instead of WARN_ON >> --- >> drivers/net/bonding/bond_alb.c | 3 ++- >> drivers/net/bonding/bond_main.c | 5 ++++- >> 2 files changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c >> index dc2c7b979656..7edf0fd58c34 100644 >> --- a/drivers/net/bonding/bond_alb.c >> +++ b/drivers/net/bonding/bond_alb.c >> @@ -985,7 +985,8 @@ 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(); >> + return -ENOMEM; >> + >> alb_send_lp_vid(slave, upper->dev_addr, >> tags[0].vlan_proto, tags[0].vlan_id); >> kfree(tags); >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >> index 51d47eda1c87..1a40bd08f984 100644 >> --- a/drivers/net/bonding/bond_main.c >> +++ b/drivers/net/bonding/bond_main.c >> @@ -2967,8 +2967,11 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev, >> >> if (start_dev == end_dev) { >> tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC); >> - if (!tags) >> + if (!tags) { >> + net_err_ratelimited("%s: %s: Failed to allocate tags\n", >> + __func__, start_dev->name); > > I'm sorry for the repeated back and forth, but checkpatch points out > that the above warning adds little value: if the memory allocation > fails, the mm layer will emit a lot warning comprising the backtrace. > > I suggest to drop it, thanks! Thank you for your advice. And I will send v5 later. Zhengchao Shao > > Paolo >
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c index dc2c7b979656..7edf0fd58c34 100644 --- a/drivers/net/bonding/bond_alb.c +++ b/drivers/net/bonding/bond_alb.c @@ -985,7 +985,8 @@ 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(); + return -ENOMEM; + alb_send_lp_vid(slave, upper->dev_addr, tags[0].vlan_proto, tags[0].vlan_id); kfree(tags); diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 51d47eda1c87..1a40bd08f984 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2967,8 +2967,11 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev, if (start_dev == end_dev) { tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC); - if (!tags) + if (!tags) { + net_err_ratelimited("%s: %s: Failed to allocate tags\n", + __func__, start_dev->name); return ERR_PTR(-ENOMEM); + } tags[level].vlan_proto = BOND_VLAN_PROTO_NONE; return 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. So return -ENOMEM in alb_upper_dev_walk to stop walking, print some warn information when failed to allocate memory for vlan tags in bond_verify_device_path. I also think that the following function calls netdev_walk_all_upper_dev_rcu ---->>>alb_upper_dev_walk ---------->>>bond_verify_device_path From this way, "end device" can eventually be obtained from "start device" in bond_verify_device_path, IS_ERR(tags) could be instead of IS_ERR_OR_NULL(tags) in alb_upper_dev_walk. Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- v4: print information instead of warn v3: return -ENOMEM instead of zero to stop walk v2: use WARN_ON_ONCE instead of WARN_ON --- drivers/net/bonding/bond_alb.c | 3 ++- drivers/net/bonding/bond_main.c | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-)