| Message ID | Y4SWJlh3ohJ6EPTL@kili (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e5214f363dabca240446272dac54d404501ad5e5 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] bonding: uninitialized variable in bond_miimon_inspect() | expand |
On Mon, Nov 28, 2022 at 4:36 PM Dan Carpenter <error27@gmail.com> wrote: > > The "ignore_updelay" variable needs to be initialized to false. > > Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > v2: Re-order so the declarations are in reverse Christmas tree order > Thanks, Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > Don't forget about: > drivers/net/bonding/bond_main.c:5071 bond_update_slave_arr() warn: missing error code here? 'bond_3ad_get_active_agg_info()' failed. 'ret' = '0' > I think that warning can be ignored, as bond_update_slave_arr() does consider the return value of bond_3ad_get_active_agg_info() but chooses to not bubble it up. Though the author of the function is the best person to answer it, at this point, it looks OK to me. Maybe a separate patch to address it would help to get the attention of the author. > drivers/net/bonding/bond_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c > index c87481033995..e01bb0412f1c 100644 > --- a/drivers/net/bonding/bond_main.c > +++ b/drivers/net/bonding/bond_main.c > @@ -2524,10 +2524,10 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in > /* called with rcu_read_lock() */ > static int bond_miimon_inspect(struct bonding *bond) > { > + bool ignore_updelay = false; > int link_state, commit = 0; > struct list_head *iter; > struct slave *slave; > - bool ignore_updelay; > > if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { > ignore_updelay = !rcu_dereference(bond->curr_active_slave); > -- > 2.35.1 >
On Mon, Nov 28, 2022 at 07:15:39PM +0530, Pavan Chebbi wrote: > On Mon, Nov 28, 2022 at 4:36 PM Dan Carpenter <error27@gmail.com> wrote: > > > > The "ignore_updelay" variable needs to be initialized to false. > > > > Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") > > Signed-off-by: Dan Carpenter <error27@gmail.com> > > --- > > v2: Re-order so the declarations are in reverse Christmas tree order > > > Thanks, > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > > > Don't forget about: > > drivers/net/bonding/bond_main.c:5071 bond_update_slave_arr() warn: missing error code here? 'bond_3ad_get_active_agg_info()' failed. 'ret' = '0' > > > > I think that warning can be ignored, as bond_update_slave_arr() does > consider the return value of bond_3ad_get_active_agg_info() but > chooses to not bubble it up. Though the author of the function is the > best person to answer it, at this point, it looks OK to me. Maybe a > separate patch to address it would help to get the attention of the > author. Heh... That's slightly vague. You're wrong to say that none of the callers care about the error code. It is checked in bond_slave_arr_handler(). regards, dan carpenter
On Mon, Nov 28, 2022 at 05:36:41PM +0300, Dan Carpenter wrote: > On Mon, Nov 28, 2022 at 07:15:39PM +0530, Pavan Chebbi wrote: > > On Mon, Nov 28, 2022 at 4:36 PM Dan Carpenter <error27@gmail.com> wrote: > > > > > > The "ignore_updelay" variable needs to be initialized to false. > > > > > > Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") > > > Signed-off-by: Dan Carpenter <error27@gmail.com> > > > --- > > > v2: Re-order so the declarations are in reverse Christmas tree order > > > > > Thanks, > > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > > > > > Don't forget about: > > > drivers/net/bonding/bond_main.c:5071 bond_update_slave_arr() warn: missing error code here? 'bond_3ad_get_active_agg_info()' failed. 'ret' = '0' > > > > > > > I think that warning can be ignored, as bond_update_slave_arr() does > > consider the return value of bond_3ad_get_active_agg_info() but > > chooses to not bubble it up. Though the author of the function is the > > best person to answer it, at this point, it looks OK to me. Maybe a > > separate patch to address it would help to get the attention of the > > author. > > Heh... That's slightly vague. > > You're wrong to say that none of the callers care about the error code. > It is checked in bond_slave_arr_handler(). If you don't know that's fine also... All the maintainers are CC'd. If they really care they can take a look otherwise there are so many other obvious bugs to care about and this is very minor. regards, dan carpenter
On Mon, Nov 28, 2022 at 8:10 PM Dan Carpenter <error27@gmail.com> wrote: > > On Mon, Nov 28, 2022 at 05:36:41PM +0300, Dan Carpenter wrote: > > On Mon, Nov 28, 2022 at 07:15:39PM +0530, Pavan Chebbi wrote: > > > On Mon, Nov 28, 2022 at 4:36 PM Dan Carpenter <error27@gmail.com> wrote: > > > > > > > > The "ignore_updelay" variable needs to be initialized to false. > > > > > > > > Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") > > > > Signed-off-by: Dan Carpenter <error27@gmail.com> > > > > --- > > > > v2: Re-order so the declarations are in reverse Christmas tree order > > > > > > > Thanks, > > > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > > > > > > > Don't forget about: > > > > drivers/net/bonding/bond_main.c:5071 bond_update_slave_arr() warn: missing error code here? 'bond_3ad_get_active_agg_info()' failed. 'ret' = '0' > > > > > > > > > > I think that warning can be ignored, as bond_update_slave_arr() does > > > consider the return value of bond_3ad_get_active_agg_info() but > > > chooses to not bubble it up. Though the author of the function is the > > > best person to answer it, at this point, it looks OK to me. Maybe a > > > separate patch to address it would help to get the attention of the > > > author. > > > > Heh... That's slightly vague. > > > > You're wrong to say that none of the callers care about the error code. > > It is checked in bond_slave_arr_handler(). > > If you don't know that's fine also... All the maintainers are CC'd. If > they really care they can take a look otherwise there are so many other > obvious bugs to care about and this is very minor. > No, I did not say nobody cares about the error code. I just said that bond_update_slave_arr() does care about bond_3ad_get_active_agg_info()'s return value, takes appropriate action and returns success to its caller. I think this is the scope and context of the warning message. To me this looks OK, but again, the maintainer/author is the best judge. > regards, > dan carpenter >
Dan Carpenter <error27@gmail.com> wrote: >The "ignore_updelay" variable needs to be initialized to false. > >Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") >Signed-off-by: Dan Carpenter <error27@gmail.com> Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com> >--- >v2: Re-order so the declarations are in reverse Christmas tree order > >Don't forget about: >drivers/net/bonding/bond_main.c:5071 bond_update_slave_arr() warn: missing error code here? 'bond_3ad_get_active_agg_info()' failed. 'ret' = '0' The code around the cited line is correct. A -1 return from bond_3ad_get_active_agg_info is not indicative of an error in the sense that something has failed, but indicates that there is no active aggregator. The code correctly returns 0 from bond_update_slave_arr, as returning non-zero would cause bond_slave_arr_handler to loop, retrying the call to bond_update_slave_arr (via workqueue). -J > drivers/net/bonding/bond_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index c87481033995..e01bb0412f1c 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -2524,10 +2524,10 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in > /* called with rcu_read_lock() */ > static int bond_miimon_inspect(struct bonding *bond) > { >+ bool ignore_updelay = false; > int link_state, commit = 0; > struct list_head *iter; > struct slave *slave; >- bool ignore_updelay; > > if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { > ignore_updelay = !rcu_dereference(bond->curr_active_slave); >-- >2.35.1 > --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Mon, Nov 28, 2022 at 10:30:15AM -0800, Jay Vosburgh wrote: > Dan Carpenter <error27@gmail.com> wrote: > > >The "ignore_updelay" variable needs to be initialized to false. > > > >Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") > >Signed-off-by: Dan Carpenter <error27@gmail.com> > > Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com> > > >--- > >v2: Re-order so the declarations are in reverse Christmas tree order > > > >Don't forget about: > >drivers/net/bonding/bond_main.c:5071 bond_update_slave_arr() warn: missing error code here? 'bond_3ad_get_active_agg_info()' failed. 'ret' = '0' > > The code around the cited line is correct. A -1 return from > bond_3ad_get_active_agg_info is not indicative of an error in the sense > that something has failed, but indicates that there is no active > aggregator. The code correctly returns 0 from bond_update_slave_arr, as > returning non-zero would cause bond_slave_arr_handler to loop, retrying > the call to bond_update_slave_arr (via workqueue). > Awesome, thanks for taking a look at this. regards, dan carpenter
Hello: This patch was applied to netdev/net-next.git (master) by Paolo Abeni <pabeni@redhat.com>: On Mon, 28 Nov 2022 14:06:14 +0300 you wrote: > The "ignore_updelay" variable needs to be initialized to false. > > Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > v2: Re-order so the declarations are in reverse Christmas tree order > > [...] Here is the summary with links: - [net-next,v2] bonding: uninitialized variable in bond_miimon_inspect() https://git.kernel.org/netdev/net-next/c/e5214f363dab You are awesome, thank you!
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index c87481033995..e01bb0412f1c 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2524,10 +2524,10 @@ static int bond_slave_info_query(struct net_device *bond_dev, struct ifslave *in /* called with rcu_read_lock() */ static int bond_miimon_inspect(struct bonding *bond) { + bool ignore_updelay = false; int link_state, commit = 0; struct list_head *iter; struct slave *slave; - bool ignore_updelay; if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { ignore_updelay = !rcu_dereference(bond->curr_active_slave);
The "ignore_updelay" variable needs to be initialized to false. Fixes: f8a65ab2f3ff ("bonding: fix link recovery in mode 2 when updelay is nonzero") Signed-off-by: Dan Carpenter <error27@gmail.com> --- v2: Re-order so the declarations are in reverse Christmas tree order Don't forget about: drivers/net/bonding/bond_main.c:5071 bond_update_slave_arr() warn: missing error code here? 'bond_3ad_get_active_agg_info()' failed. 'ret' = '0' drivers/net/bonding/bond_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)