diff mbox series

[next] ipvlan: add L2 check for packets arriving via virtual devices

Message ID 20171207231543.7637-1-mahesh@bandewar.net
State Accepted, archived
Delegated to: David Miller
Headers show
Series [next] ipvlan: add L2 check for packets arriving via virtual devices | expand

Commit Message

Mahesh Bandewar Dec. 7, 2017, 11:15 p.m. UTC 
From: Mahesh Bandewar <maheshb@google.com>

Packets that don't have dest mac as the mac of the master device should
not be entertained by the IPvlan rx-handler. This is mostly true as the
packet path mostly takes care of that, except when the master device is
a virtual device. As demonstrated in the following case -

  ip netns add ns1
  ip link add ve1 type veth peer name ve2
  ip link add link ve2 name iv1 type ipvlan mode l2
  ip link set dev iv1 netns ns1
  ip link set ve1 up
  ip link set ve2 up
  ip -n ns1 link set iv1 up
  ip addr add 192.168.10.1/24 dev ve1
  ip -n ns1 addr 192.168.10.2/24 dev iv1
  ping -c2 192.168.10.2
  <Works!>
  ip neigh show dev ve1
  ip neigh show 192.168.10.2 lladdr <random> dev ve1
  ping -c2 192.168.10.2
  <Still works! Wrong!!>

This patch adds that missing check in the IPvlan rx-handler.

Reported-by: Amit Sikka <amit.sikka@ericsson.com>
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
 drivers/net/ipvlan/ipvlan_core.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

David Miller Dec. 11, 2017, 4:15 p.m. UTC | #1 
From: Mahesh Bandewar <mahesh@bandewar.net>
Date: Thu,  7 Dec 2017 15:15:43 -0800

> From: Mahesh Bandewar <maheshb@google.com>
> 
> Packets that don't have dest mac as the mac of the master device should
> not be entertained by the IPvlan rx-handler. This is mostly true as the
> packet path mostly takes care of that, except when the master device is
> a virtual device. As demonstrated in the following case -
 ...
> This patch adds that missing check in the IPvlan rx-handler.
> 
> Reported-by: Amit Sikka <amit.sikka@ericsson.com>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

Applied, but it's a shame that the data plane takes on this new MAC
compare operation.
Mahesh Bandewar (महेश बंडेवार) Dec. 11, 2017, 7:38 p.m. UTC | #2 
On Mon, Dec 11, 2017 at 8:15 AM, David Miller <davem@davemloft.net> wrote:
> From: Mahesh Bandewar <mahesh@bandewar.net>
> Date: Thu,  7 Dec 2017 15:15:43 -0800
>
>> From: Mahesh Bandewar <maheshb@google.com>
>>
>> Packets that don't have dest mac as the mac of the master device should
>> not be entertained by the IPvlan rx-handler. This is mostly true as the
>> packet path mostly takes care of that, except when the master device is
>> a virtual device. As demonstrated in the following case -
>  ...
>> This patch adds that missing check in the IPvlan rx-handler.
>>
>> Reported-by: Amit Sikka <amit.sikka@ericsson.com>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>
> Applied, but it's a shame that the data plane takes on this new MAC
> compare operation.
Your comment made me think little more about this and a discussion
with Eric kind of put things in perspective. eth_type_trans() does the
right thing and sets the packet_type correctly (when .ndo_xmit of veth
is called). However IPvlan is over-aggressive in packet scrubbing and
that scrub changes packet type. This causes the actual problem. It's
not clear to me why skb_scrub_packet() changes the packet type to
PACKET_HOST unconditionally? But that's another issue.

I'll send another patch to remove excessive scrubbing in IPvlan and
revert of this patch so that this additional comparison (though not
expensive!) can be avoided.

Thanks,
--mahesh..
David Miller Dec. 11, 2017, 8:16 p.m. UTC | #3 
From: Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com>

Date: Mon, 11 Dec 2017 11:38:04 -0800

> On Mon, Dec 11, 2017 at 8:15 AM, David Miller <davem@davemloft.net> wrote:

>> From: Mahesh Bandewar <mahesh@bandewar.net>

>> Date: Thu,  7 Dec 2017 15:15:43 -0800

>>

>>> From: Mahesh Bandewar <maheshb@google.com>

>>>

>>> Packets that don't have dest mac as the mac of the master device should

>>> not be entertained by the IPvlan rx-handler. This is mostly true as the

>>> packet path mostly takes care of that, except when the master device is

>>> a virtual device. As demonstrated in the following case -

>>  ...

>>> This patch adds that missing check in the IPvlan rx-handler.

>>>

>>> Reported-by: Amit Sikka <amit.sikka@ericsson.com>

>>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>

>>

>> Applied, but it's a shame that the data plane takes on this new MAC

>> compare operation.

> Your comment made me think little more about this and a discussion

> with Eric kind of put things in perspective. eth_type_trans() does the

> right thing and sets the packet_type correctly (when .ndo_xmit of veth

> is called). However IPvlan is over-aggressive in packet scrubbing and

> that scrub changes packet type. This causes the actual problem. It's

> not clear to me why skb_scrub_packet() changes the packet type to

> PACKET_HOST unconditionally? But that's another issue.

> 

> I'll send another patch to remove excessive scrubbing in IPvlan and

> revert of this patch so that this additional comparison (though not

> expensive!) can be avoided.


Thanks for looking more deeply into this.
diff mbox series

Patch

diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index 9774c96ac7bb..0bc7f721b717 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -322,6 +322,10 @@  static int ipvlan_rcv_frame(struct ipvl_addr *addr, struct sk_buff **pskb,
 		if (dev_forward_skb(ipvlan->dev, skb) == NET_RX_SUCCESS)
 			success = true;
 	} else {
+		if (!ether_addr_equal_64bits(eth_hdr(skb)->h_dest,
+					     ipvlan->phy_dev->dev_addr))
+			skb->pkt_type = PACKET_OTHERHOST;
+
 		ret = RX_HANDLER_ANOTHER;
 		success = true;
 	}