Re: [PATCH next] bonding: pass link-local packets to bonding master also.
On 2018-07-17 11:32, Michal Soltys wrote:
> On 07/17/2018 01:53 AM, Mahesh Bandewar (महेश बंडेवार) wrote:
>> On Mon, Jul 16, 2018 at 2:24 PM, Jay Vosburgh
>> wrote:
>>> Mahesh Bandewar wrote:
>>>
From: Mahesh Bandewar
Commit b89f04c61efe ("bonding: deliver link-local packets with
skb->dev set to link that packets arrived on") changed the behavior
of how link-local-multicast packets are processed. The change in
the behavior broke some legacy use cases where these packets are
expected to arrive on bonding master device also.
This patch passes the packet to the stack with the link it arrived
on as well as passes to the bonding-master device to preserve the
legacy use case.
>>>
>>> Michal, can you test this? I'm travelling this week and won't
>>> be able to run the patch.
>
>
> Yes, will test today and report.
>
The patch looks to be working fine - tested both passive bridge
(stp_state == 0) and with in-kernel implementation active (stp_state ==
1). No loops, no issues so far.
Re: [PATCH next] bonding: pass link-local packets to bonding master also.
On 07/17/2018 01:57 AM, Mahesh Bandewar (महेश बंडेवार) wrote:
On Mon, Jul 16, 2018 at 4:33 PM, Stephen Hemminger
wrote:
On Sun, 15 Jul 2018 18:12:46 -0700
Mahesh Bandewar wrote:
From: Mahesh Bandewar
Commit b89f04c61efe ("bonding: deliver link-local packets with
skb->dev set to link that packets arrived on") changed the behavior
of how link-local-multicast packets are processed. The change in
the behavior broke some legacy use cases where these packets are
expected to arrive on bonding master device also.
This patch passes the packet to the stack with the link it arrived
on as well as passes to the bonding-master device to preserve the
legacy use case.
Reported-by: Michal Soltys
Signed-off-by: Mahesh Bandewar
Thanks for fixing this.
Why not add a Fixes: tag instead of just talking about the commit?
That helps the stable maintainers know which versions of the kernel
need the patch.
Well, I thought about it. It's definitely 'related' but not sure it
'fixes' in true sense. It definitely fixes the broken legacy case
though. Is that sufficient to add 'fixes' tag?
It's __not__ broken legacy case. It's normal behavior, starting with
specification covering LLDP itself (IEEE Std 802.1AB-2016, page 18, '6.8 LLDP
and Link Aggregation') and ending with a linux bridge actively doing stp via
in-kernel implementation or with userspace helper (or inactively passing) and
being blind to bpdus. Not mentioning a very recent kernel feature like
per-port group_fwd_mask rendered useless in this case.
Unless you also consider attaching a bond to a linux bridge as a broken legacy
use case. Among other things mentioned in the other thread.
In this context, the comment in code/log message IMHO (of the attached patch),
should be changed - as it will be just confusing for anyone reading it in the
future.
(and I'd very much like the fix to hit relevant stable kernels as well)
Re: [PATCH next] bonding: pass link-local packets to bonding master also.
On 07/17/2018 01:53 AM, Mahesh Bandewar (महेश बंडेवार) wrote:
On Mon, Jul 16, 2018 at 2:24 PM, Jay Vosburgh
wrote:
Mahesh Bandewar wrote:
From: Mahesh Bandewar
Commit b89f04c61efe ("bonding: deliver link-local packets with
skb->dev set to link that packets arrived on") changed the behavior
of how link-local-multicast packets are processed. The change in
the behavior broke some legacy use cases where these packets are
expected to arrive on bonding master device also.
This patch passes the packet to the stack with the link it arrived
on as well as passes to the bonding-master device to preserve the
legacy use case.
Michal, can you test this? I'm travelling this week and won't
be able to run the patch.
Yes, will test today and report.
Re: [PATCH next] bonding: pass link-local packets to bonding master also.
On Mon, 16 Jul 2018 16:57:22 -0700
Mahesh Bandewar (महेश बंडेवार) wrote:
> On Mon, Jul 16, 2018 at 4:33 PM, Stephen Hemminger
> wrote:
> > On Sun, 15 Jul 2018 18:12:46 -0700
> > Mahesh Bandewar wrote:
> >
> >> From: Mahesh Bandewar
> >>
> >> Commit b89f04c61efe ("bonding: deliver link-local packets with
> >> skb->dev set to link that packets arrived on") changed the behavior
> >> of how link-local-multicast packets are processed. The change in
> >> the behavior broke some legacy use cases where these packets are
> >> expected to arrive on bonding master device also.
> >>
> >> This patch passes the packet to the stack with the link it arrived
> >> on as well as passes to the bonding-master device to preserve the
> >> legacy use case.
> >>
> >> Reported-by: Michal Soltys
> >> Signed-off-by: Mahesh Bandewar
> >
> > Thanks for fixing this.
> >
> > Why not add a Fixes: tag instead of just talking about the commit?
> > That helps the stable maintainers know which versions of the kernel
> > need the patch.
> Well, I thought about it. It's definitely 'related' but not sure it
> 'fixes' in true sense. It definitely fixes the broken legacy case
> though. Is that sufficient to add 'fixes' tag?
The previous commit caused a regression. your change fixes the regression
Re: [PATCH next] bonding: pass link-local packets to bonding master also.
On Mon, Jul 16, 2018 at 4:33 PM, Stephen Hemminger
wrote:
> On Sun, 15 Jul 2018 18:12:46 -0700
> Mahesh Bandewar wrote:
>
>> From: Mahesh Bandewar
>>
>> Commit b89f04c61efe ("bonding: deliver link-local packets with
>> skb->dev set to link that packets arrived on") changed the behavior
>> of how link-local-multicast packets are processed. The change in
>> the behavior broke some legacy use cases where these packets are
>> expected to arrive on bonding master device also.
>>
>> This patch passes the packet to the stack with the link it arrived
>> on as well as passes to the bonding-master device to preserve the
>> legacy use case.
>>
>> Reported-by: Michal Soltys
>> Signed-off-by: Mahesh Bandewar
>
> Thanks for fixing this.
>
> Why not add a Fixes: tag instead of just talking about the commit?
> That helps the stable maintainers know which versions of the kernel
> need the patch.
Well, I thought about it. It's definitely 'related' but not sure it
'fixes' in true sense. It definitely fixes the broken legacy case
though. Is that sufficient to add 'fixes' tag?
Re: [PATCH next] bonding: pass link-local packets to bonding master also.
On Mon, Jul 16, 2018 at 2:24 PM, Jay Vosburgh
wrote:
> Mahesh Bandewar wrote:
>
>>From: Mahesh Bandewar
>>
>>Commit b89f04c61efe ("bonding: deliver link-local packets with
>>skb->dev set to link that packets arrived on") changed the behavior
>>of how link-local-multicast packets are processed. The change in
>>the behavior broke some legacy use cases where these packets are
>>expected to arrive on bonding master device also.
>>
>>This patch passes the packet to the stack with the link it arrived
>>on as well as passes to the bonding-master device to preserve the
>>legacy use case.
>
> Michal, can you test this? I'm travelling this week and won't
> be able to run the patch.
>
> Mahesh, will this confuse LLDP, et al, daemons that, e.g., bind
> to every possible interface and now see the same LLDP PDU (identical
> Chassis ID, Port ID, et al, TLVs) on multiple interfaces?
>
Well it's hard to say. In the previous world when these packets used
to appear only on bonding-master, that service had to go extra-lengths
to figure it out which link it actually came on in. With the earlier
change (SHA1: b89f04c61efe) it didn't have to but with this patch, the
best thing that they could do is just ignore those packets coming from
(any) virtual devices. The only reason why I'm OK with this change is
because L2 of a physical link is shared with a virtual link (bonding
master) and hence both links receiving the same link-local-multicast
seems acceptable. Making them appear only on bonding-master is just
wrong while correcting that behavior breaks the legacy use case and
here we are.
BTW when links are aggregated and using LACP, these packets don't
arrive the system-mac but the real mac of the sender with a dest
multicast-mac.
--mahesh..
> Thanks,
>
> -J
>
>>Reported-by: Michal Soltys
>>Signed-off-by: Mahesh Bandewar
>>---
>> drivers/net/bonding/bond_main.c | 17 +++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 9a2ea3c1f949..1d3b7d8448f2 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1177,9 +1177,22 @@ static rx_handler_result_t bond_handle_frame(struct
>>sk_buff **pskb)
>> }
>> }
>>
>>- /* don't change skb->dev for link-local packets */
>>- if (is_link_local_ether_addr(eth_hdr(skb)->h_dest))
>>+ /* Link-local multicast packets should be passed to the
>>+ * stack on the link they arrive as well as pass them to the
>>+ * bond-master device. These packets are mostly usable when
>>+ * stack receives it with the link on which they arrive
>>+ * (e.g. LLDP) but there may be some legacy behavior that
>>+ * expects these packets to appear on bonding master too.
>>+ */
>>+ if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) {
>>+ struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>>+
>>+ if (nskb) {
>>+ nskb->dev = bond->dev;
>>+ netif_rx(nskb);
>>+ }
>> return RX_HANDLER_PASS;
>>+ }
>> if (bond_should_deliver_exact_match(skb, slave, bond))
>> return RX_HANDLER_EXACT;
>>
>>--
>>2.18.0.203.gfac676dfb9-goog
>
> ---
> -Jay Vosburgh, jay.vosbu...@canonical.com
Re: [PATCH next] bonding: pass link-local packets to bonding master also.
On Sun, 15 Jul 2018 18:12:46 -0700
Mahesh Bandewar wrote:
> From: Mahesh Bandewar
>
> Commit b89f04c61efe ("bonding: deliver link-local packets with
> skb->dev set to link that packets arrived on") changed the behavior
> of how link-local-multicast packets are processed. The change in
> the behavior broke some legacy use cases where these packets are
> expected to arrive on bonding master device also.
>
> This patch passes the packet to the stack with the link it arrived
> on as well as passes to the bonding-master device to preserve the
> legacy use case.
>
> Reported-by: Michal Soltys
> Signed-off-by: Mahesh Bandewar
Thanks for fixing this.
Why not add a Fixes: tag instead of just talking about the commit?
That helps the stable maintainers know which versions of the kernel
need the patch.
Re: [PATCH next] bonding: pass link-local packets to bonding master also.
Mahesh Bandewar wrote:
>From: Mahesh Bandewar
>
>Commit b89f04c61efe ("bonding: deliver link-local packets with
>skb->dev set to link that packets arrived on") changed the behavior
>of how link-local-multicast packets are processed. The change in
>the behavior broke some legacy use cases where these packets are
>expected to arrive on bonding master device also.
>
>This patch passes the packet to the stack with the link it arrived
>on as well as passes to the bonding-master device to preserve the
>legacy use case.
Michal, can you test this? I'm travelling this week and won't
be able to run the patch.
Mahesh, will this confuse LLDP, et al, daemons that, e.g., bind
to every possible interface and now see the same LLDP PDU (identical
Chassis ID, Port ID, et al, TLVs) on multiple interfaces?
Thanks,
-J
>Reported-by: Michal Soltys
>Signed-off-by: Mahesh Bandewar
>---
> drivers/net/bonding/bond_main.c | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 9a2ea3c1f949..1d3b7d8448f2 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1177,9 +1177,22 @@ static rx_handler_result_t bond_handle_frame(struct
>sk_buff **pskb)
> }
> }
>
>- /* don't change skb->dev for link-local packets */
>- if (is_link_local_ether_addr(eth_hdr(skb)->h_dest))
>+ /* Link-local multicast packets should be passed to the
>+ * stack on the link they arrive as well as pass them to the
>+ * bond-master device. These packets are mostly usable when
>+ * stack receives it with the link on which they arrive
>+ * (e.g. LLDP) but there may be some legacy behavior that
>+ * expects these packets to appear on bonding master too.
>+ */
>+ if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) {
>+ struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>+
>+ if (nskb) {
>+ nskb->dev = bond->dev;
>+ netif_rx(nskb);
>+ }
> return RX_HANDLER_PASS;
>+ }
> if (bond_should_deliver_exact_match(skb, slave, bond))
> return RX_HANDLER_EXACT;
>
>--
>2.18.0.203.gfac676dfb9-goog
---
-Jay Vosburgh, jay.vosbu...@canonical.com
[PATCH next] bonding: pass link-local packets to bonding master also.
From: Mahesh Bandewar
Commit b89f04c61efe ("bonding: deliver link-local packets with
skb->dev set to link that packets arrived on") changed the behavior
of how link-local-multicast packets are processed. The change in
the behavior broke some legacy use cases where these packets are
expected to arrive on bonding master device also.
This patch passes the packet to the stack with the link it arrived
on as well as passes to the bonding-master device to preserve the
legacy use case.
Reported-by: Michal Soltys
Signed-off-by: Mahesh Bandewar
---
drivers/net/bonding/bond_main.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9a2ea3c1f949..1d3b7d8448f2 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1177,9 +1177,22 @@ static rx_handler_result_t bond_handle_frame(struct
sk_buff **pskb)
}
}
- /* don't change skb->dev for link-local packets */
- if (is_link_local_ether_addr(eth_hdr(skb)->h_dest))
+ /* Link-local multicast packets should be passed to the
+* stack on the link they arrive as well as pass them to the
+* bond-master device. These packets are mostly usable when
+* stack receives it with the link on which they arrive
+* (e.g. LLDP) but there may be some legacy behavior that
+* expects these packets to appear on bonding master too.
+*/
+ if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) {
+ struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+
+ if (nskb) {
+ nskb->dev = bond->dev;
+ netif_rx(nskb);
+ }
return RX_HANDLER_PASS;
+ }
if (bond_should_deliver_exact_match(skb, slave, bond))
return RX_HANDLER_EXACT;
--
2.18.0.203.gfac676dfb9-goog
