| Message ID | 20190218165528.15575-1-soltys@ziu.info |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2] bonding: fix PACKET_ORIGDEV regression | expand |
On 2/18/19 9:55 AM, Michal Soltys wrote: > This patch fixes a subtle PACKET_ORIGDEV regression which was a side > effect of fixes introduced by: > > 6a9e461f6fe4 bonding: pass link-local packets to bonding master also. > > ... to: > > b89f04c61efe bonding: deliver link-local packets with skb->dev set to link that packets arrived on > > While 6a9e461f6fe4 restored pre-b89f04c61efe presence of link-local > packets on bonding masters (which is required e.g. by linux bridges > participating in spanning tree or needed for lab-like setups created > with group_fwd_mask) it also caused the originating device > information to be lost due to cloning. > > Maciej Żenczykowski proposed another solution that doesn't require > packet cloning and retains original device information - instead of > returning RX_HANDLER_PASS for all link-local packets it's now limited > only to packets from inactive slaves. > > At the same time, packets passed to bonding masters retain correct > information about the originating device and PACKET_ORIGDEV can be used > to determine it. > > This elegantly solves all issues so far: > > - link-local packets that were removed from bonding masters > - LLDP daemons being forced to explicitly bind to slave interfaces > - PACKET_ORIGDEV having no effect on bond interfaces > > Fixes: 6a9e461f6fe4 (bonding: pass link-local packets to bonding master also.) > Reported-by: Vincent Bernat <vincent@bernat.ch> > Signed-off-by: Michal Soltys <soltys@ziu.info> > --- > drivers/net/bonding/bond_main.c | 35 +++++++++++++-------------------- > 1 file changed, 14 insertions(+), 21 deletions(-) > Hi Michal: Can you add test cases that shows the expectations of this API? Given the back and forth on the last set of patches -- and the impacts to lldpd users for example -- we really need test cases added to selftests.
Signed-off-by: Maciej Żenczykowski <maze@google.com>
It certainly seems like this should do the trick, but I do agree some
sort of tests would be nice.
From: Michal Soltys <soltys@ziu.info> Date: Mon, 18 Feb 2019 17:55:28 +0100 > This patch fixes a subtle PACKET_ORIGDEV regression which was a side > effect of fixes introduced by: > > 6a9e461f6fe4 bonding: pass link-local packets to bonding master also. > > ... to: > > b89f04c61efe bonding: deliver link-local packets with skb->dev set to link that packets arrived on ... > Fixes: 6a9e461f6fe4 (bonding: pass link-local packets to bonding master also.) > Reported-by: Vincent Bernat <vincent@bernat.ch> > Signed-off-by: Michal Soltys <soltys@ziu.info> Applied and queued up for -stable, thanks.
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 485462d3087f..537c90c8eb0a 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1183,29 +1183,22 @@ static rx_handler_result_t bond_handle_frame(struct sk_buff **pskb) } } - /* 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) they also must be available on master. Some of - * the use cases include (but are not limited to): LLDP agents - * that must be able to operate both on enslaved interfaces as - * well as on bonds themselves; linux bridges that must be able - * to process/pass BPDUs from attached bonds when any kind of - * STP version is enabled on the network. + /* + * For packets determined by bond_should_deliver_exact_match() call to + * be suppressed we want to make an exception for link-local packets. + * This is necessary for e.g. LLDP daemons to be able to monitor + * inactive slave links without being forced to bind to them + * explicitly. + * + * At the same time, packets that are passed to the bonding master + * (including link-local ones) can have their originating interface + * determined via PACKET_ORIGDEV socket option. */ - 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; - nskb->queue_mapping = 0; - netif_rx(nskb); - } - return RX_HANDLER_PASS; - } - if (bond_should_deliver_exact_match(skb, slave, bond)) + if (bond_should_deliver_exact_match(skb, slave, bond)) { + if (is_link_local_ether_addr(eth_hdr(skb)->h_dest)) + return RX_HANDLER_PASS; return RX_HANDLER_EXACT; + } skb->dev = bond->dev;
This patch fixes a subtle PACKET_ORIGDEV regression which was a side effect of fixes introduced by: 6a9e461f6fe4 bonding: pass link-local packets to bonding master also. ... to: b89f04c61efe bonding: deliver link-local packets with skb->dev set to link that packets arrived on While 6a9e461f6fe4 restored pre-b89f04c61efe presence of link-local packets on bonding masters (which is required e.g. by linux bridges participating in spanning tree or needed for lab-like setups created with group_fwd_mask) it also caused the originating device information to be lost due to cloning. Maciej Żenczykowski proposed another solution that doesn't require packet cloning and retains original device information - instead of returning RX_HANDLER_PASS for all link-local packets it's now limited only to packets from inactive slaves. At the same time, packets passed to bonding masters retain correct information about the originating device and PACKET_ORIGDEV can be used to determine it. This elegantly solves all issues so far: - link-local packets that were removed from bonding masters - LLDP daemons being forced to explicitly bind to slave interfaces - PACKET_ORIGDEV having no effect on bond interfaces Fixes: 6a9e461f6fe4 (bonding: pass link-local packets to bonding master also.) Reported-by: Vincent Bernat <vincent@bernat.ch> Signed-off-by: Michal Soltys <soltys@ziu.info> --- drivers/net/bonding/bond_main.c | 35 +++++++++++++-------------------- 1 file changed, 14 insertions(+), 21 deletions(-)