* [PATCH] loopback: set pkt_type to PACKET_HOST explicitly
@ 2013-06-26 7:34 Isaku Yamahata
2013-06-26 8:40 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Isaku Yamahata @ 2013-06-26 7:34 UTC (permalink / raw)
To: netdev; +Cc: yamahata, murphy.mccauley, Pravin B Shelar, Jesse Gross, dev
Reset pkt_type to PACKET_HOST when loopback device receives packet
before calling eth_type_trans()
ip-encapsulated packets can be handled by localhost. But skb->pkt_type
can be PACKET_OTHERHOST when packet comes into ip tunnel device. In that case,
the packet is dropped by ip_rcv() because loopback_xmit() doesn't set
skb->pkt_type to PACKET_HOST.
netns A | root netns | netns B
veth<->veth=bridge=gretap <-loop back-> gretap=bridge=veth<->veth
arp packet ->
pkt_type
BROADCAST----loopback_xmit()-->ip_rcv()------------------->
<- arp reply
pkt_type
ip_rcv()<---loopback_xmit()-----OTHERHOST
drop
sample operations
ip link add tapa type gretap remote 172.17.107.4 local 172.17.107.3
ip link add tapb type gretap remote 172.17.107.3 local 172.17.107.4
ip link set tapa up
ip link set tapb up
ip address add 172.17.107.3 dev tapa
ip address add 172.17.107.4 dev tapb
ip route get 172.17.107.3
> local 172.17.107.3 dev lo src 172.17.107.3
> cache <local>
ip route get 172.17.107.4
> local 172.17.107.4 dev lo src 172.17.107.4
> cache <local>
ip link add vetha type veth peer name vetha-peer
ip link add vethb type veth peer name vethb-peer
brctl addbr bra
brctl addbr brb
brctl addif bra tapa
brctl addif bra vetha-peer
brctl addif brb tapb
brctl addif brb vethb-peer
brctl show
> bridge name bridge id STP enabled interfaces
> bra 8000.6ea21e758ff1 no tapa
> vetha-peer
> brb 8000.420020eb92d5 no tapb
> vethb-peer
ip link set vetha-peer up
ip link set vethb-peer up
ip link set bra up
ip link set brb up
ip netns add a
ip netns add b
ip link set vetha netns a
ip link set vethb netns b
ip netns exec a ip address add 10.0.0.3/24 dev vetha
ip netns exec b ip address add 10.0.0.4/24 dev vethb
ip netns exec a ip link set vetha up
ip netns exec b ip link set vethb up
ip netns exec a arping -I vetha 10.0.0.4
ARPING 10.0.0.4 from 10.0.0.3 vetha
^CSent 2 probes (2 broadcast(s))
Received 0 response(s)
Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: Jesse Gross <jesse@nicira.com>
Cc: dev@openvswitch.org
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
drivers/net/loopback.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index fcbf680..2694638 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -82,6 +82,12 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
*/
skb_dst_force(skb);
+ /* pkt_type is not always PACKET_HOST because
+ * this skb comes from other components.
+ * Since eth_type_trans() sets pkt_type _except_ PACKET_HOST case,
+ * set it explicitly.
+ */
+ skb->pkt_type = PACKET_HOST;
skb->protocol = eth_type_trans(skb, dev);
/* it's OK to use per_cpu_ptr() because BHs are off */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] loopback: set pkt_type to PACKET_HOST explicitly
2013-06-26 7:34 [PATCH] loopback: set pkt_type to PACKET_HOST explicitly Isaku Yamahata
@ 2013-06-26 8:40 ` Eric Dumazet
2013-06-26 9:37 ` Isaku Yamahata
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-06-26 8:40 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: netdev, murphy.mccauley, Pravin B Shelar, Jesse Gross, dev
On Wed, 2013-06-26 at 16:34 +0900, Isaku Yamahata wrote:
> Reset pkt_type to PACKET_HOST when loopback device receives packet
> before calling eth_type_trans()
>
> ip-encapsulated packets can be handled by localhost. But skb->pkt_type
> can be PACKET_OTHERHOST when packet comes into ip tunnel device. In that case,
> the packet is dropped by ip_rcv() because loopback_xmit() doesn't set
> skb->pkt_type to PACKET_HOST.
>
> netns A | root netns | netns B
> veth<->veth=bridge=gretap <-loop back-> gretap=bridge=veth<->veth
>
> arp packet ->
> pkt_type
> BROADCAST----loopback_xmit()-->ip_rcv()------------------->
>
> <- arp reply
> pkt_type
> ip_rcv()<---loopback_xmit()-----OTHERHOST
> drop
>
> sample operations
> ip link add tapa type gretap remote 172.17.107.4 local 172.17.107.3
> ip link add tapb type gretap remote 172.17.107.3 local 172.17.107.4
> ip link set tapa up
> ip link set tapb up
> ip address add 172.17.107.3 dev tapa
> ip address add 172.17.107.4 dev tapb
> ip route get 172.17.107.3
> > local 172.17.107.3 dev lo src 172.17.107.3
> > cache <local>
> ip route get 172.17.107.4
> > local 172.17.107.4 dev lo src 172.17.107.4
> > cache <local>
> ip link add vetha type veth peer name vetha-peer
> ip link add vethb type veth peer name vethb-peer
> brctl addbr bra
> brctl addbr brb
> brctl addif bra tapa
> brctl addif bra vetha-peer
> brctl addif brb tapb
> brctl addif brb vethb-peer
> brctl show
> > bridge name bridge id STP enabled interfaces
> > bra 8000.6ea21e758ff1 no tapa
> > vetha-peer
> > brb 8000.420020eb92d5 no tapb
> > vethb-peer
> ip link set vetha-peer up
> ip link set vethb-peer up
> ip link set bra up
> ip link set brb up
> ip netns add a
> ip netns add b
> ip link set vetha netns a
> ip link set vethb netns b
> ip netns exec a ip address add 10.0.0.3/24 dev vetha
> ip netns exec b ip address add 10.0.0.4/24 dev vethb
> ip netns exec a ip link set vetha up
> ip netns exec b ip link set vethb up
> ip netns exec a arping -I vetha 10.0.0.4
> ARPING 10.0.0.4 from 10.0.0.3 vetha
> ^CSent 2 probes (2 broadcast(s))
> Received 0 response(s)
>
> Cc: Pravin B Shelar <pshelar@nicira.com>
> Cc: Jesse Gross <jesse@nicira.com>
> Cc: dev@openvswitch.org
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
> drivers/net/loopback.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index fcbf680..2694638 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -82,6 +82,12 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> */
> skb_dst_force(skb);
>
> + /* pkt_type is not always PACKET_HOST because
> + * this skb comes from other components.
> + * Since eth_type_trans() sets pkt_type _except_ PACKET_HOST case,
> + * set it explicitly.
> + */
> + skb->pkt_type = PACKET_HOST;
> skb->protocol = eth_type_trans(skb, dev);
>
> /* it's OK to use per_cpu_ptr() because BHs are off */
It sounds really strange to me.
This is not loopback duty to change pkt_type.
Where (and why) exactly pkt_type is set to PACKET_OTHERHOST ?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] loopback: set pkt_type to PACKET_HOST explicitly
2013-06-26 8:40 ` Eric Dumazet
@ 2013-06-26 9:37 ` Isaku Yamahata
2013-06-26 22:29 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Isaku Yamahata @ 2013-06-26 9:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, murphy.mccauley, Pravin B Shelar, Jesse Gross, dev
On Wed, Jun 26, 2013 at 01:40:22AM -0700, Eric Dumazet wrote:
> On Wed, 2013-06-26 at 16:34 +0900, Isaku Yamahata wrote:
> > Reset pkt_type to PACKET_HOST when loopback device receives packet
> > before calling eth_type_trans()
> >
> > ip-encapsulated packets can be handled by localhost. But skb->pkt_type
> > can be PACKET_OTHERHOST when packet comes into ip tunnel device. In that case,
> > the packet is dropped by ip_rcv() because loopback_xmit() doesn't set
> > skb->pkt_type to PACKET_HOST.
> >
> > netns A | root netns | netns B
> > veth<->veth=bridge=gretap <-loop back-> gretap=bridge=veth<->veth
> >
> > arp packet ->
> > pkt_type
> > BROADCAST----loopback_xmit()-->ip_rcv()------------------->
> >
> > <- arp reply
> > pkt_type
> > ip_rcv()<---loopback_xmit()-----OTHERHOST
> > drop
> >
> > sample operations
> > ip link add tapa type gretap remote 172.17.107.4 local 172.17.107.3
> > ip link add tapb type gretap remote 172.17.107.3 local 172.17.107.4
> > ip link set tapa up
> > ip link set tapb up
> > ip address add 172.17.107.3 dev tapa
> > ip address add 172.17.107.4 dev tapb
> > ip route get 172.17.107.3
> > > local 172.17.107.3 dev lo src 172.17.107.3
> > > cache <local>
> > ip route get 172.17.107.4
> > > local 172.17.107.4 dev lo src 172.17.107.4
> > > cache <local>
> > ip link add vetha type veth peer name vetha-peer
> > ip link add vethb type veth peer name vethb-peer
> > brctl addbr bra
> > brctl addbr brb
> > brctl addif bra tapa
> > brctl addif bra vetha-peer
> > brctl addif brb tapb
> > brctl addif brb vethb-peer
> > brctl show
> > > bridge name bridge id STP enabled interfaces
> > > bra 8000.6ea21e758ff1 no tapa
> > > vetha-peer
> > > brb 8000.420020eb92d5 no tapb
> > > vethb-peer
> > ip link set vetha-peer up
> > ip link set vethb-peer up
> > ip link set bra up
> > ip link set brb up
> > ip netns add a
> > ip netns add b
> > ip link set vetha netns a
> > ip link set vethb netns b
> > ip netns exec a ip address add 10.0.0.3/24 dev vetha
> > ip netns exec b ip address add 10.0.0.4/24 dev vethb
> > ip netns exec a ip link set vetha up
> > ip netns exec b ip link set vethb up
> > ip netns exec a arping -I vetha 10.0.0.4
> > ARPING 10.0.0.4 from 10.0.0.3 vetha
> > ^CSent 2 probes (2 broadcast(s))
> > Received 0 response(s)
> >
> > Cc: Pravin B Shelar <pshelar@nicira.com>
> > Cc: Jesse Gross <jesse@nicira.com>
> > Cc: dev@openvswitch.org
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > ---
> > drivers/net/loopback.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> > index fcbf680..2694638 100644
> > --- a/drivers/net/loopback.c
> > +++ b/drivers/net/loopback.c
> > @@ -82,6 +82,12 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
> > */
> > skb_dst_force(skb);
> >
> > + /* pkt_type is not always PACKET_HOST because
> > + * this skb comes from other components.
> > + * Since eth_type_trans() sets pkt_type _except_ PACKET_HOST case,
> > + * set it explicitly.
> > + */
> > + skb->pkt_type = PACKET_HOST;
> > skb->protocol = eth_type_trans(skb, dev);
> >
> > /* it's OK to use per_cpu_ptr() because BHs are off */
>
>
> It sounds really strange to me.
>
> This is not loopback duty to change pkt_type.
>
> Where (and why) exactly pkt_type is set to PACKET_OTHERHOST ?
veth does. vethb-peer in the above example.
(veth_xmit() -> dev_forward_skb() -> eth_type_trans())
The destination mac address of arp reply is set to the one of
vetha (!= vethb-peer). So vethb-peer sets pkt_type to OTHERHOST.
bridge and gretap doesn't touch skb->pkt_type.
--
yamahata
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] loopback: set pkt_type to PACKET_HOST explicitly
2013-06-26 9:37 ` Isaku Yamahata
@ 2013-06-26 22:29 ` David Miller
2013-06-27 5:29 ` Isaku Yamahata
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2013-06-26 22:29 UTC (permalink / raw)
To: yamahata; +Cc: eric.dumazet, netdev, murphy.mccauley, pshelar, jesse, dev
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Wed, 26 Jun 2013 18:37:51 +0900
> veth does. vethb-peer in the above example.
> (veth_xmit() -> dev_forward_skb() -> eth_type_trans())
> The destination mac address of arp reply is set to the one of
> vetha (!= vethb-peer). So vethb-peer sets pkt_type to OTHERHOST.
> bridge and gretap doesn't touch skb->pkt_type.
I think the dev_forward_skb() assignment of pkt_type should be done
after the call to eth_type_trans().
That's the whole point, we know we're looping the packet back to a
local device on this host.
I'm not applying this loopback patch, sorry.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] loopback: set pkt_type to PACKET_HOST explicitly
2013-06-26 22:29 ` David Miller
@ 2013-06-27 5:29 ` Isaku Yamahata
2013-06-27 8:50 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Isaku Yamahata @ 2013-06-27 5:29 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev, murphy.mccauley, pshelar, jesse, dev
On Wed, Jun 26, 2013 at 03:29:30PM -0700, David Miller wrote:
> From: Isaku Yamahata <yamahata@valinux.co.jp>
> Date: Wed, 26 Jun 2013 18:37:51 +0900
>
> > veth does. vethb-peer in the above example.
> > (veth_xmit() -> dev_forward_skb() -> eth_type_trans())
> > The destination mac address of arp reply is set to the one of
> > vetha (!= vethb-peer). So vethb-peer sets pkt_type to OTHERHOST.
> > bridge and gretap doesn't touch skb->pkt_type.
>
> I think the dev_forward_skb() assignment of pkt_type should be done
> after the call to eth_type_trans().
>
> That's the whole point, we know we're looping the packet back to a
> local device on this host.
>
> I'm not applying this loopback patch, sorry.
So you mean like this patch?
The callers of dev_forward_skb() are only veth, macvlan and l2tp, which
seem fine with this change.
>From 99281ba2b4f383623fe8da238bc6616952334bfe Mon Sep 17 00:00:00 2001
Message-Id: <99281ba2b4f383623fe8da238bc6616952334bfe.1372310725.git.yamahata@valinux.co.jp>
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Thu, 27 Jun 2013 12:15:11 +0900
Subject: [PATCH] core/dev: set pkt_type after eth_type_trans()
The dev_forward_skb() assignment of pkt_type should be done
after the call to eth_type_trans().
ip-encapsulated packets can be handled by localhost. But skb->pkt_type
can be PACKET_OTHERHOST when packet comes via veth into ip tunnel device.
In that case, the packet is dropped by ip_rcv().
netns A | root netns | netns B
veth<->veth=bridge=gretap <-loop back-> gretap=bridge=veth<->veth
arp packet ->
pkt_type
BROADCAST------------>ip_rcv()------------------------>
<- arp reply
pkt_type
ip_rcv()<-----------------OTHERHOST
drop
sample operations
ip link add tapa type gretap remote 172.17.107.4 local 172.17.107.3
ip link add tapb type gretap remote 172.17.107.3 local 172.17.107.4
ip link set tapa up
ip link set tapb up
ip address add 172.17.107.3 dev tapa
ip address add 172.17.107.4 dev tapb
ip route get 172.17.107.3
> local 172.17.107.3 dev lo src 172.17.107.3
> cache <local>
ip route get 172.17.107.4
> local 172.17.107.4 dev lo src 172.17.107.4
> cache <local>
ip link add vetha type veth peer name vetha-peer
ip link add vethb type veth peer name vethb-peer
brctl addbr bra
brctl addbr brb
brctl addif bra tapa
brctl addif bra vetha-peer
brctl addif brb tapb
brctl addif brb vethb-peer
brctl show
> bridge name bridge id STP enabled interfaces
> bra 8000.6ea21e758ff1 no tapa
> vetha-peer
> brb 8000.420020eb92d5 no tapb
> vethb-peer
ip link set vetha-peer up
ip link set vethb-peer up
ip link set bra up
ip link set brb up
ip netns add a
ip netns add b
ip link set vetha netns a
ip link set vethb netns b
ip netns exec a ip address add 10.0.0.3/24 dev vetha
ip netns exec b ip address add 10.0.0.4/24 dev vethb
ip netns exec a ip link set vetha up
ip netns exec b ip link set vethb up
ip netns exec a arping -I vetha 10.0.0.4
ARPING 10.0.0.4 from 10.0.0.3 vetha
^CSent 2 probes (2 broadcast(s))
Received 0 response(s)
Cc: eric.dumazet@gmail.com
Cc: Pravin B Shelar <pshelar@nicira.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
net/core/dev.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 722f633..b179b8a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1662,8 +1662,12 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
skb->skb_iif = 0;
skb_dst_drop(skb);
skb->tstamp.tv64 = 0;
- skb->pkt_type = PACKET_HOST;
skb->protocol = eth_type_trans(skb, dev);
+ /*
+ * eth_type_trans() can set pkt_type.
+ * clear pkt_type _after_ calling eth_type_trans()
+ */
+ skb->pkt_type = PACKET_HOST;
skb->mark = 0;
secpath_reset(skb);
nf_reset(skb);
--
1.7.10.4
--
yamahata
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] loopback: set pkt_type to PACKET_HOST explicitly
2013-06-27 5:29 ` Isaku Yamahata
@ 2013-06-27 8:50 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2013-06-27 8:50 UTC (permalink / raw)
To: yamahata; +Cc: eric.dumazet, netdev, murphy.mccauley, pshelar, jesse, dev
From: Isaku Yamahata <yamahata@valinux.co.jp>
Date: Thu, 27 Jun 2013 14:29:38 +0900
> So you mean like this patch? The callers of dev_forward_skb() are
> only veth, macvlan and l2tp, which seem fine with this change.
Yes, it is what I was suggesting.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-27 8:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-26 7:34 [PATCH] loopback: set pkt_type to PACKET_HOST explicitly Isaku Yamahata
2013-06-26 8:40 ` Eric Dumazet
2013-06-26 9:37 ` Isaku Yamahata
2013-06-26 22:29 ` David Miller
2013-06-27 5:29 ` Isaku Yamahata
2013-06-27 8:50 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox