[PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[Dan Carpenter]

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(-)

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

Re: [PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[Dan Carpenter]

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

Re: [PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[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

Re: [PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[Jay Vosburgh]

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

Re: [PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[Dan Carpenter]

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

Re: [PATCH net-next v2] bonding: uninitialized variable in bond_miimon_inspect()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html