* [PATCH net] bonding: Avoid possible de-sync with arp_validate
@ 2013-09-06 17:41 Marcelo Ricardo Leitner
2013-09-06 17:49 ` Nikolay Aleksandrov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2013-09-06 17:41 UTC (permalink / raw)
To: netdev; +Cc: naleksan, fubar, andy
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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
2013-09-06 17:41 [PATCH net] bonding: Avoid possible de-sync with arp_validate Marcelo Ricardo Leitner
@ 2013-09-06 17:49 ` Nikolay Aleksandrov
2013-09-06 17:56 ` Jay Vosburgh
2013-09-06 17:58 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Nikolay Aleksandrov @ 2013-09-06 17:49 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: netdev, naleksan, fubar, andy
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
2013-09-06 17:41 [PATCH net] bonding: Avoid possible de-sync with arp_validate Marcelo Ricardo Leitner
2013-09-06 17:49 ` Nikolay Aleksandrov
@ 2013-09-06 17:56 ` Jay Vosburgh
2013-09-06 18:13 ` Marcelo Ricardo Leitner
2013-09-06 17:58 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Jay Vosburgh @ 2013-09-06 17:56 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: netdev, naleksan, andy
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
2013-09-06 17:56 ` Jay Vosburgh
@ 2013-09-06 18:13 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2013-09-06 18:13 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, naleksan, andy
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
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
2013-09-06 17:41 [PATCH net] bonding: Avoid possible de-sync with arp_validate Marcelo Ricardo Leitner
2013-09-06 17:49 ` Nikolay Aleksandrov
2013-09-06 17:56 ` Jay Vosburgh
@ 2013-09-06 17:58 ` David Miller
2013-09-06 18:02 ` Marcelo Ricardo Leitner
2 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2013-09-06 17:58 UTC (permalink / raw)
To: mleitner; +Cc: netdev, naleksan, fubar, andy
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
2013-09-06 17:58 ` David Miller
@ 2013-09-06 18:02 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2013-09-06 18:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, naleksan, fubar, andy
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-06 18:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-06 17:41 [PATCH net] bonding: Avoid possible de-sync with arp_validate Marcelo Ricardo Leitner
2013-09-06 17:49 ` Nikolay Aleksandrov
2013-09-06 17:56 ` Jay Vosburgh
2013-09-06 18:13 ` Marcelo Ricardo Leitner
2013-09-06 17:58 ` David Miller
2013-09-06 18:02 ` Marcelo Ricardo Leitner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).