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