diff mbox

[net] bonding: Avoid possible de-sync with arp_validate

Message ID 994d2dc994a801fc3aae7aa912266b09ea45d95c.1378489255.git.mleitner@redhat.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Marcelo Ricardo Leitner Sept. 6, 2013, 5:41 p.m. UTC
Hi Dave,

Please queue this one for -stable trees too.

Thanks.

---8<---

As bond_store_arp_validation and bond_store_arp_interval doesn't
deal with each other, we can't chain both at bond_open, otherwise
we are open to this de-sync state, steps in sequence:
- bond is down
- arp_interval = 0
- arp_validate = 1 or 2
- bond is up
- arp_interval = 1

This would lead to bond issuing ARP requests but never listening
to the replies, because the tap wasn't attached. After reaching
this state, while bond is up, setting arp_validate won't help
as it only acts if needed.
---
 drivers/net/bonding/bond_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Nikolay Aleksandrov Sept. 6, 2013, 5:49 p.m. UTC | #1
On 09/06/2013 07:41 PM, Marcelo Ricardo Leitner wrote:
> Hi Dave,
> 
> Please queue this one for -stable trees too.
> 
> Thanks.
> 
> ---8<---
> 
> As bond_store_arp_validation and bond_store_arp_interval doesn't
> deal with each other, we can't chain both at bond_open, otherwise
> we are open to this de-sync state, steps in sequence:
> - bond is down
> - arp_interval = 0
> - arp_validate = 1 or 2
> - bond is up
> - arp_interval = 1
> 
> This would lead to bond issuing ARP requests but never listening
> to the replies, because the tap wasn't attached. After reaching
> this state, while bond is up, setting arp_validate won't help
> as it only acts if needed.
> ---
>  drivers/net/bonding/bond_main.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 39e5b1c..805d098 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev)
>  	if (bond->params.miimon)  /* link check interval, in milliseconds. */
>  		queue_delayed_work(bond->wq, &bond->mii_work, 0);
>  
> -	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
> +	if (bond->params.arp_interval)    /* arp interval, in milliseconds. */
>  		queue_delayed_work(bond->wq, &bond->arp_work, 0);
> -		if (bond->params.arp_validate)
> -			bond->recv_probe = bond_arp_rcv;
> -	}
> +	if (bond->params.arp_validate)
> +		bond->recv_probe = bond_arp_rcv;
>  
>  	if (bond->params.mode == BOND_MODE_8023AD) {
>  		queue_delayed_work(bond->wq, &bond->ad_work, 0);
> 
Thanks for fixing this.

Acked-by: Nikolay Aleksandrov <nikolay@redhat.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jay Vosburgh Sept. 6, 2013, 5:56 p.m. UTC | #2
Marcelo Ricardo Leitner <mleitner@redhat.com> wrote:

>Hi Dave,
>
>Please queue this one for -stable trees too.
>
>Thanks.
>
>---8<---
>
>As bond_store_arp_validation and bond_store_arp_interval doesn't
>deal with each other, we can't chain both at bond_open, otherwise
>we are open to this de-sync state, steps in sequence:
>- bond is down
>- arp_interval = 0
>- arp_validate = 1 or 2
>- bond is up
>- arp_interval = 1
>
>This would lead to bond issuing ARP requests but never listening
>to the replies, because the tap wasn't attached. After reaching
>this state, while bond is up, setting arp_validate won't help
>as it only acts if needed.

	You need to sign off you patch.

> drivers/net/bonding/bond_main.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 39e5b1c..805d098 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev)
> 	if (bond->params.miimon)  /* link check interval, in milliseconds. */
> 		queue_delayed_work(bond->wq, &bond->mii_work, 0);
>
>-	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
>+	if (bond->params.arp_interval)    /* arp interval, in milliseconds. */
> 		queue_delayed_work(bond->wq, &bond->arp_work, 0);
>-		if (bond->params.arp_validate)
>-			bond->recv_probe = bond_arp_rcv;
>-	}
>+	if (bond->params.arp_validate)
>+		bond->recv_probe = bond_arp_rcv;

	Won't this set the recv_probe when arp_interval is 0 (disabled)?
That's going to make bonding look at a bunch of traffic it's not going
to do anything with.

	Why is this better than having bonding_store_arp_interval set
recv_probe if arp_validate is set when it queues the arp_work?  And,
presumably, clear the recv_probe if arp_interval is being cleared.

	-J

> 	if (bond->params.mode == BOND_MODE_8023AD) {
> 		queue_delayed_work(bond->wq, &bond->ad_work, 0);
>-- 
>1.8.3.1
>

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 6, 2013, 5:58 p.m. UTC | #3
From: Marcelo Ricardo Leitner <mleitner@redhat.com>
Date: Fri,  6 Sep 2013 14:41:53 -0300

> Please queue this one for -stable trees too.

This is very much not the correct way to submit a patch.

First of all, you haven't signed off on the patch.

Second of all, any supplemental information that doesn't belong in the
commit message needs to go after a "---" dileanator line.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcelo Ricardo Leitner Sept. 6, 2013, 6:02 p.m. UTC | #4
Em 06-09-2013 14:58, David Miller escreveu:
> From: Marcelo Ricardo Leitner <mleitner@redhat.com>
> Date: Fri,  6 Sep 2013 14:41:53 -0300
>
>> Please queue this one for -stable trees too.
>
> This is very much not the correct way to submit a patch.
>
> First of all, you haven't signed off on the patch.
>
> Second of all, any supplemental information that doesn't belong in the
> commit message needs to go after a "---" dileanator line.

Okay, sorry for that. I'll resubmit as soon as I work on Jay's comments.

Thanks
Marcelo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcelo Ricardo Leitner Sept. 6, 2013, 6:13 p.m. UTC | #5
Em 06-09-2013 14:56, Jay Vosburgh escreveu:
> Marcelo Ricardo Leitner <mleitner@redhat.com> wrote:
>
>> Hi Dave,
>>
>> Please queue this one for -stable trees too.
>>
>> Thanks.
>>
>> ---8<---
>>
>> As bond_store_arp_validation and bond_store_arp_interval doesn't
>> deal with each other, we can't chain both at bond_open, otherwise
>> we are open to this de-sync state, steps in sequence:
>> - bond is down
>> - arp_interval = 0
>> - arp_validate = 1 or 2
>> - bond is up
>> - arp_interval = 1
>>
>> This would lead to bond issuing ARP requests but never listening
>> to the replies, because the tap wasn't attached. After reaching
>> this state, while bond is up, setting arp_validate won't help
>> as it only acts if needed.
>
> 	You need to sign off you patch.

My bad, sorry

>> drivers/net/bonding/bond_main.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 39e5b1c..805d098 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev)
>> 	if (bond->params.miimon)  /* link check interval, in milliseconds. */
>> 		queue_delayed_work(bond->wq, &bond->mii_work, 0);
>>
>> -	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
>> +	if (bond->params.arp_interval)    /* arp interval, in milliseconds. */
>> 		queue_delayed_work(bond->wq, &bond->arp_work, 0);
>> -		if (bond->params.arp_validate)
>> -			bond->recv_probe = bond_arp_rcv;
>> -	}
>> +	if (bond->params.arp_validate)
>> +		bond->recv_probe = bond_arp_rcv;
>
> 	Won't this set the recv_probe when arp_interval is 0 (disabled)?
> That's going to make bonding look at a bunch of traffic it's not going
> to do anything with.

Yes.

> 	Why is this better than having bonding_store_arp_interval set
> recv_probe if arp_validate is set when it queues the arp_work?  And,
> presumably, clear the recv_probe if arp_interval is being cleared.
>
> 	-J

Yeah, no good. You're right.

Self-NACK here. Nikolay will submit a better patch later.

Thanks,
Marcelo

>> 	if (bond->params.mode == BOND_MODE_8023AD) {
>> 		queue_delayed_work(bond->wq, &bond->ad_work, 0);
>> --
>> 1.8.3.1
>>
>
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 39e5b1c..805d098 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3180,11 +3180,10 @@  static int bond_open(struct net_device *bond_dev)
 	if (bond->params.miimon)  /* link check interval, in milliseconds. */
 		queue_delayed_work(bond->wq, &bond->mii_work, 0);
 
-	if (bond->params.arp_interval) {  /* arp interval, in milliseconds. */
+	if (bond->params.arp_interval)    /* arp interval, in milliseconds. */
 		queue_delayed_work(bond->wq, &bond->arp_work, 0);
-		if (bond->params.arp_validate)
-			bond->recv_probe = bond_arp_rcv;
-	}
+	if (bond->params.arp_validate)
+		bond->recv_probe = bond_arp_rcv;
 
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		queue_delayed_work(bond->wq, &bond->ad_work, 0);