Re: [PATCH net-next] bonding: do not pass link-local packets to master-interface
Mahesh Bandewar (महेश बंडेवार) wrote: >On Fri, Apr 14, 2017 at 12:30 PM, Jay Vosburgh > wrote: >> >> >> Chonggang Li wrote: >> >> >Previously link-local packets excluding LACP (which are handled by >> >the recv_probe) received on bond slave interfaces are delivered to >> >stack with bond-master device with RX_HANDLER_ANOTHER, however all >> >link-local packets are link specific and should be delivered with >> >exact same link/dev. >> >> In what case is the current behavior a problem (my guess would >> be something related to LLDP)? What if, e.g., the bond is a bridge >> port, will STP frames no longer propagate to the bridge? >> >When a link-local frame made appear to arrive on bond-master, it >looses value since 'the info' about link on which it arrived is lost. This information should really be in the commit message, along with something about the LLDP issue being solved. >So idea behind this is to pass the frame to the stack with the link on >which it arrived without involving the bonding-master. The same will >happen for the STP frames too. Do you see problem with this approach? My look through the STP code suggests not, but I'm far from an expert there. I'm just concerned that this change will cause some obscure regression in something that depends on the current behavior. >> Also, I think the description would be better if it mentioned >> specifically that the patch is changing how skb->dev is set for link >> local frames (bond->dev vs receiving interface), e.g., >> >> "[...] however all link-local packets are link specific and >> should be delivered with skb->dev set to the original device." >> >yes, makes sense. > >> >Signed-off-by: Chonggang Li >> >Signed-off-by: Mahesh Bandewar >> >Signed-off-by: Maciej Żenczykowski >> >--- >> > drivers/net/bonding/bond_main.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> >diff --git a/drivers/net/bonding/bond_main.c >> >b/drivers/net/bonding/bond_main.c >> >index 01e4a69af421..aeca3d8541b9 100644 >> >--- a/drivers/net/bonding/bond_main.c >> >+++ b/drivers/net/bonding/bond_main.c >> >@@ -1176,6 +1176,9 @@ static rx_handler_result_t bond_handle_frame(struct >> >sk_buff **pskb) >> > } >> > } >> > >> >+ /* link-local packets should not be passed to master interface */ >> >+ if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) >> >+ return RX_HANDLER_PASS; >> >> Since this returns _PASS and not _EXACT, the packet will go >> through the ptype_base packet handlers, so is the comment strictly >> correct? >> >The stack does not have all link-local specific packet-type handlers >registered by default so returning _EXACT would find nothing and >packet will be dropped, right? So returning _PASS will deliver packet >to the stack with skb->dev as the link on which packet arrived and >stack can take whatever (default) action it has to take. Fair enough; I do think the comment would be better phrased as something like "don't change skb->dev of link local frames"; on first reading, I thought it meant the frames would be dropped. -J >> > if (bond_should_deliver_exact_match(skb, slave, bond)) >> > return RX_HANDLER_EXACT; >> > >> >-- >> >2.12.2.762.g0e3151a226-goog >> > >> --- -Jay Vosburgh, jay.vosbu...@canonical.com
Re: [PATCH net-next] bonding: do not pass link-local packets to master-interface
On Fri, Apr 14, 2017 at 12:30 PM, Jay Vosburgh wrote: > > > Chonggang Li wrote: > > >Previously link-local packets excluding LACP (which are handled by > >the recv_probe) received on bond slave interfaces are delivered to > >stack with bond-master device with RX_HANDLER_ANOTHER, however all > >link-local packets are link specific and should be delivered with > >exact same link/dev. > > In what case is the current behavior a problem (my guess would > be something related to LLDP)? What if, e.g., the bond is a bridge > port, will STP frames no longer propagate to the bridge? > When a link-local frame made appear to arrive on bond-master, it looses value since 'the info' about link on which it arrived is lost. So idea behind this is to pass the frame to the stack with the link on which it arrived without involving the bonding-master. The same will happen for the STP frames too. Do you see problem with this approach? > Also, I think the description would be better if it mentioned > specifically that the patch is changing how skb->dev is set for link > local frames (bond->dev vs receiving interface), e.g., > > "[...] however all link-local packets are link specific and > should be delivered with skb->dev set to the original device." > yes, makes sense. > >Signed-off-by: Chonggang Li > >Signed-off-by: Mahesh Bandewar > >Signed-off-by: Maciej Żenczykowski > >--- > > drivers/net/bonding/bond_main.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > >diff --git a/drivers/net/bonding/bond_main.c > >b/drivers/net/bonding/bond_main.c > >index 01e4a69af421..aeca3d8541b9 100644 > >--- a/drivers/net/bonding/bond_main.c > >+++ b/drivers/net/bonding/bond_main.c > >@@ -1176,6 +1176,9 @@ static rx_handler_result_t bond_handle_frame(struct > >sk_buff **pskb) > > } > > } > > > >+ /* link-local packets should not be passed to master interface */ > >+ if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) > >+ return RX_HANDLER_PASS; > > Since this returns _PASS and not _EXACT, the packet will go > through the ptype_base packet handlers, so is the comment strictly > correct? > The stack does not have all link-local specific packet-type handlers registered by default so returning _EXACT would find nothing and packet will be dropped, right? So returning _PASS will deliver packet to the stack with skb->dev as the link on which packet arrived and stack can take whatever (default) action it has to take. > -J > > > if (bond_should_deliver_exact_match(skb, slave, bond)) > > return RX_HANDLER_EXACT; > > > >-- > >2.12.2.762.g0e3151a226-goog > > > > --- > -Jay Vosburgh, jay.vosbu...@canonical.com
Re: [PATCH net-next] bonding: do not pass link-local packets to master-interface
> In what case is the current behavior a problem (my guess would > be something related to LLDP)? LLDP is indeed the case we're trying to solve here. Listen on one socket and get LLDP for all interfaces in the system. > What if, e.g., the bond is a bridge > port, will STP frames no longer propagate to the bridge? That's an interesting question. I don't actually know how this should work. Should this change perhaps only apply to packets we would otherwise RX_HANDLER_EXACT? ie. only affect link local packets on inactive slaves? but continue reparenting link local packets on active slaves? That would seem a little inconsistent... but less of a change.
Re: [PATCH net-next] bonding: do not pass link-local packets to master-interface
Chonggang Li wrote: >Previously link-local packets excluding LACP (which are handled by >the recv_probe) received on bond slave interfaces are delivered to >stack with bond-master device with RX_HANDLER_ANOTHER, however all >link-local packets are link specific and should be delivered with >exact same link/dev. In what case is the current behavior a problem (my guess would be something related to LLDP)? What if, e.g., the bond is a bridge port, will STP frames no longer propagate to the bridge? Also, I think the description would be better if it mentioned specifically that the patch is changing how skb->dev is set for link local frames (bond->dev vs receiving interface), e.g., "[...] however all link-local packets are link specific and should be delivered with skb->dev set to the original device." >Signed-off-by: Chonggang Li >Signed-off-by: Mahesh Bandewar >Signed-off-by: Maciej Żenczykowski >--- > drivers/net/bonding/bond_main.c | 3 +++ > 1 file changed, 3 insertions(+) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 01e4a69af421..aeca3d8541b9 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1176,6 +1176,9 @@ static rx_handler_result_t bond_handle_frame(struct >sk_buff **pskb) > } > } > >+ /* link-local packets should not be passed to master interface */ >+ if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) >+ return RX_HANDLER_PASS; Since this returns _PASS and not _EXACT, the packet will go through the ptype_base packet handlers, so is the comment strictly correct? -J > if (bond_should_deliver_exact_match(skb, slave, bond)) > return RX_HANDLER_EXACT; > >-- >2.12.2.762.g0e3151a226-goog > --- -Jay Vosburgh, jay.vosbu...@canonical.com
[PATCH net-next] bonding: do not pass link-local packets to master-interface
Previously link-local packets excluding LACP (which are handled by the recv_probe) received on bond slave interfaces are delivered to stack with bond-master device with RX_HANDLER_ANOTHER, however all link-local packets are link specific and should be delivered with exact same link/dev. Signed-off-by: Chonggang Li Signed-off-by: Mahesh Bandewar Signed-off-by: Maciej Żenczykowski --- drivers/net/bonding/bond_main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 01e4a69af421..aeca3d8541b9 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1176,6 +1176,9 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb) } } + /* link-local packets should not be passed to master interface */ + if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) + return RX_HANDLER_PASS; if (bond_should_deliver_exact_match(skb, slave, bond)) return RX_HANDLER_EXACT; -- 2.12.2.762.g0e3151a226-goog
