[PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Frank Blaschka]

From: Frank Blaschka <frank.blaschka@de.ibm.com>

commit 8a83a00b0735190384a348156837918271034144 unconditionally
drops dst reference when skb->dev is set. This causes a regression
with VLAN and the qeth_l3 network driver. qeth_l3 can not get gw
information from the skb coming from the vlan driver. It is only
valid to drop the dst in case of different name spaces.

Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
---
 net/core/dev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1881,8 +1881,8 @@ EXPORT_SYMBOL(netif_device_attach);
 #ifdef CONFIG_NET_NS
 void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
 {
-	skb_dst_drop(skb);
 	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
+		skb_dst_drop(skb);
 		secpath_reset(skb);
 		nf_reset(skb);
 		skb_init_secmark(skb);

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Eric Dumazet]

On Mon, 2012-04-30 at 07:38 +0200, Frank Blaschka wrote:
> From: Frank Blaschka <frank.blaschka@de.ibm.com>
> 
> commit 8a83a00b0735190384a348156837918271034144 unconditionally
> drops dst reference when skb->dev is set. This causes a regression
> with VLAN and the qeth_l3 network driver. qeth_l3 can not get gw
> information from the skb coming from the vlan driver. It is only
> valid to drop the dst in case of different name spaces.
> 
> Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
> ---
>  net/core/dev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1881,8 +1881,8 @@ EXPORT_SYMBOL(netif_device_attach);
>  #ifdef CONFIG_NET_NS
>  void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
>  {
> -	skb_dst_drop(skb);
>  	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
> +		skb_dst_drop(skb);
>  		secpath_reset(skb);
>  		nf_reset(skb);
>  		skb_init_secmark(skb);
> 

You forgot CC Arnd Bergmann <arnd@arndb.de> ?

But we do want to do the skb_dst_drop() in dev_forward_skb()

Your patch breaks dev_forward_skb() then.

But apparently this forward path was alredy broken in Arnd patch...

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Frank Blaschka]

On Mon, Apr 30, 2012 at 07:51:33AM +0200, Eric Dumazet wrote:
> On Mon, 2012-04-30 at 07:38 +0200, Frank Blaschka wrote:
> > From: Frank Blaschka <frank.blaschka@de.ibm.com>
> > 
> > commit 8a83a00b0735190384a348156837918271034144 unconditionally
> > drops dst reference when skb->dev is set. This causes a regression
> > with VLAN and the qeth_l3 network driver. qeth_l3 can not get gw
> > information from the skb coming from the vlan driver. It is only
> > valid to drop the dst in case of different name spaces.
> > 
> > Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
> > ---
> >  net/core/dev.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1881,8 +1881,8 @@ EXPORT_SYMBOL(netif_device_attach);
> >  #ifdef CONFIG_NET_NS
> >  void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
> >  {
> > -	skb_dst_drop(skb);
> >  	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
> > +		skb_dst_drop(skb);
> >  		secpath_reset(skb);
> >  		nf_reset(skb);
> >  		skb_init_secmark(skb);
> > 
> 
> You forgot CC Arnd Bergmann <arnd@arndb.de> ?
> 
> But we do want to do the skb_dst_drop() in dev_forward_skb()
> 
> Your patch breaks dev_forward_skb() then.
> 
> But apparently this forward path was alredy broken in Arnd patch...
>
Hi Eric,

do you have and suggestions how to fix this regression?
The qeth_l3 driver needs the gw information to address the frame.
The way to look at dst works well for the non vlan case and was
also working for vlan prior Arnds changes. 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Frank Blaschka]

On Mon, Apr 30, 2012 at 07:51:33AM +0200, Eric Dumazet wrote:
> On Mon, 2012-04-30 at 07:38 +0200, Frank Blaschka wrote:
> > From: Frank Blaschka <frank.blaschka@de.ibm.com>
> > 
> > commit 8a83a00b0735190384a348156837918271034144 unconditionally
> > drops dst reference when skb->dev is set. This causes a regression
> > with VLAN and the qeth_l3 network driver. qeth_l3 can not get gw
> > information from the skb coming from the vlan driver. It is only
> > valid to drop the dst in case of different name spaces.
> > 
> > Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
> > ---
> >  net/core/dev.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1881,8 +1881,8 @@ EXPORT_SYMBOL(netif_device_attach);
> >  #ifdef CONFIG_NET_NS
> >  void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
> >  {
> > -	skb_dst_drop(skb);
> >  	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
> > +		skb_dst_drop(skb);
> >  		secpath_reset(skb);
> >  		nf_reset(skb);
> >  		skb_init_secmark(skb);
> > 
> 
> You forgot CC Arnd Bergmann <arnd@arndb.de> ?
> 
> But we do want to do the skb_dst_drop() in dev_forward_skb()
> 
> Your patch breaks dev_forward_skb() then.
If NET_NS is not defined is this broken too?
> 
> But apparently this forward path was alredy broken in Arnd patch...
>
Ok, until nobody comes up with an other idea I will post a patch to
change back the vlan driver (use skb->dev = dev instead of skb_set_dev)
next week. 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Eric Dumazet]

On Wed, 2012-05-02 at 07:50 +0200, Frank Blaschka wrote:
> On Mon, Apr 30, 2012 at 07:51:33AM +0200, Eric Dumazet wrote:
> > On Mon, 2012-04-30 at 07:38 +0200, Frank Blaschka wrote:
> > > From: Frank Blaschka <frank.blaschka@de.ibm.com>
> > > 
> > > commit 8a83a00b0735190384a348156837918271034144 unconditionally
> > > drops dst reference when skb->dev is set. This causes a regression
> > > with VLAN and the qeth_l3 network driver. qeth_l3 can not get gw
> > > information from the skb coming from the vlan driver. It is only
> > > valid to drop the dst in case of different name spaces.
> > > 
> > > Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
> > > ---
> > >  net/core/dev.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -1881,8 +1881,8 @@ EXPORT_SYMBOL(netif_device_attach);
> > >  #ifdef CONFIG_NET_NS
> > >  void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
> > >  {
> > > -	skb_dst_drop(skb);
> > >  	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
> > > +		skb_dst_drop(skb);
> > >  		secpath_reset(skb);
> > >  		nf_reset(skb);
> > >  		skb_init_secmark(skb);
> > > 
> > 
> > You forgot CC Arnd Bergmann <arnd@arndb.de> ?
> > 
> > But we do want to do the skb_dst_drop() in dev_forward_skb()
> > 
> > Your patch breaks dev_forward_skb() then.
> If NET_NS is not defined is this broken too?
> > 
> > But apparently this forward path was alredy broken in Arnd patch...
> >
> Ok, until nobody comes up with an other idea I will post a patch to
> change back the vlan driver (use skb->dev = dev instead of skb_set_dev)
> next week. 
> > 


If Arnd doesnt care to even reply, we can just revert his buggy patch,
instead of trying to understand and fix all issues.

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 02 May 2012 08:59:21 +0200

> If Arnd doesnt care to even reply, we can just revert his buggy
> patch, instead of trying to understand and fix all issues.

Agreed, it's the best sounding solution by far.

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Arnd Bergmann]

On Thursday 03 May 2012, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 02 May 2012 08:59:21 +0200
> 
> > If Arnd doesnt care to even reply, we can just revert his buggy
> > patch, instead of trying to understand and fix all issues.
> 
> Agreed, it's the best sounding solution by far.

I've tried to understand what I've done back then but also couldn't
remember, so I won't complain when it gets reverted.

For all I know I did this because it looked broken back then, but
I don't remember a specific way in which it could be exploited
to escape a namespace.

	Arnd

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Frank Blaschka]

On Thu, May 03, 2012 at 02:28:37AM -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 02 May 2012 08:59:21 +0200
> 
> > If Arnd doesnt care to even reply, we can just revert his buggy
> > patch, instead of trying to understand and fix all issues.
> 
> Agreed, it's the best sounding solution by far.
Hi Dave,

looks like discussion is done and everybody is ok with reverting
Arnds patch. Can you revert commit 8a83a00b0735190384a348156837918271034144 ?
Thx

Fran ?
Thx

Frank

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[David Miller]

From: Frank Blaschka <blaschka@linux.vnet.ibm.com>
Date: Wed, 9 May 2012 12:48:03 +0200

> On Thu, May 03, 2012 at 02:28:37AM -0400, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Wed, 02 May 2012 08:59:21 +0200
>> 
>> > If Arnd doesnt care to even reply, we can just revert his buggy
>> > patch, instead of trying to understand and fix all issues.
>> 
>> Agreed, it's the best sounding solution by far.
> Hi Dave,
> 
> looks like discussion is done and everybody is ok with reverting
> Arnds patch. Can you revert commit 8a83a00b0735190384a348156837918271034144 ?

Done, and I'll queue this up for -stable too.

[PATCH] Revert "net: maintain namespace isolation between vlan and real device"

This reverts commit 8a83a00b0735190384a348156837918271034144.

It causes regressions for S390 devices, because it does an
unconditional DST drop on SKBs for vlans and the QETH device
needs the neighbour entry hung off the DST for certain things
on transmit.

Arnd can't remember exactly why he even needed this change.

Conflicts:

	drivers/net/macvlan.c
	net/8021q/vlan_dev.c
	net/core/dev.c

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/macvlan.c     |    2 +-
 include/linux/netdevice.h |    9 ---------
 net/8021q/vlan_dev.c      |    2 +-
 net/core/dev.c            |   36 +++++-------------------------------
 4 files changed, 7 insertions(+), 42 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f975afd..025367a 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -259,7 +259,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
 
 xmit_world:
 	skb->ip_summed = ip_summed;
-	skb_set_dev(skb, vlan->lowerdev);
+	skb->dev = vlan->lowerdev;
 	return dev_queue_xmit(skb);
 }
 
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5cbaa20..33900a5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1403,15 +1403,6 @@ static inline bool netdev_uses_dsa_tags(struct net_device *dev)
 	return 0;
 }
 
-#ifndef CONFIG_NET_NS
-static inline void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
-{
-	skb->dev = dev;
-}
-#else /* CONFIG_NET_NS */
-void skb_set_dev(struct sk_buff *skb, struct net_device *dev);
-#endif
-
 static inline bool netdev_uses_trailer_tags(struct net_device *dev)
 {
 #ifdef CONFIG_NET_DSA_TAG_TRAILER
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 9988d4a..9757c19 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -157,7 +157,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
 		skb = __vlan_hwaccel_put_tag(skb, vlan_tci);
 	}
 
-	skb_set_dev(skb, vlan_dev_priv(dev)->real_dev);
+	skb->dev = vlan_dev_priv(dev)->real_dev;
 	len = skb->len;
 	if (netpoll_tx_running(dev))
 		return skb->dev->netdev_ops->ndo_start_xmit(skb, skb->dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index 9bb8f87..99e1d75 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1617,10 +1617,14 @@ int dev_forward_skb(struct net_device *dev, struct sk_buff *skb)
 		return NET_RX_DROP;
 	}
 	skb->skb_iif = 0;
-	skb_set_dev(skb, dev);
+	skb->dev = dev;
+	skb_dst_drop(skb);
 	skb->tstamp.tv64 = 0;
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, dev);
+	skb->mark = 0;
+	secpath_reset(skb);
+	nf_reset(skb);
 	return netif_rx(skb);
 }
 EXPORT_SYMBOL_GPL(dev_forward_skb);
@@ -1869,36 +1873,6 @@ void netif_device_attach(struct net_device *dev)
 }
 EXPORT_SYMBOL(netif_device_attach);
 
-/**
- * skb_dev_set -- assign a new device to a buffer
- * @skb: buffer for the new device
- * @dev: network device
- *
- * If an skb is owned by a device already, we have to reset
- * all data private to the namespace a device belongs to
- * before assigning it a new device.
- */
-#ifdef CONFIG_NET_NS
-void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
-{
-	skb_dst_drop(skb);
-	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
-		secpath_reset(skb);
-		nf_reset(skb);
-		skb_init_secmark(skb);
-		skb->mark = 0;
-		skb->priority = 0;
-		skb->nf_trace = 0;
-		skb->ipvs_property = 0;
-#ifdef CONFIG_NET_SCHED
-		skb->tc_index = 0;
-#endif
-	}
-	skb->dev = dev;
-}
-EXPORT_SYMBOL(skb_set_dev);
-#endif /* CONFIG_NET_NS */
-
 static void skb_warn_bad_offload(const struct sk_buff *skb)
 {
 	static const netdev_features_t null_features = 0;
-- 
1.7.10.1

Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst

[Stephen Hemminger]

On Mon, 30 Apr 2012 07:51:33 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Mon, 2012-04-30 at 07:38 +0200, Frank Blaschka wrote:
> > From: Frank Blaschka <frank.blaschka@de.ibm.com>
> > 
> > commit 8a83a00b0735190384a348156837918271034144 unconditionally
> > drops dst reference when skb->dev is set. This causes a regression
> > with VLAN and the qeth_l3 network driver. qeth_l3 can not get gw
> > information from the skb coming from the vlan driver. It is only
> > valid to drop the dst in case of different name spaces.
> > 
> > Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
> > ---
> >  net/core/dev.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -1881,8 +1881,8 @@ EXPORT_SYMBOL(netif_device_attach);
> >  #ifdef CONFIG_NET_NS
> >  void skb_set_dev(struct sk_buff *skb, struct net_device *dev)
> >  {
> > -	skb_dst_drop(skb);
> >  	if (skb->dev && !net_eq(dev_net(skb->dev), dev_net(dev))) {
> > +		skb_dst_drop(skb);
> >  		secpath_reset(skb);
> >  		nf_reset(skb);
> >  		skb_init_secmark(skb);
> > 
> 
> You forgot CC Arnd Bergmann <arnd@arndb.de> ?
> 
> But we do want to do the skb_dst_drop() in dev_forward_skb()
> 
> Your patch breaks dev_forward_skb() then.
> 
> But apparently this forward path was alredy broken in Arnd patch...

Is this related to why, PMTU discover is broken now over GRE.
The simple case of doing a TCP transfer from local host over
GRE tunnel hangs.

What happens is that skb_dst(skb) is null in ip_tunnel_xmit.
Which leads to the MTU not being updated, and the ICMP_FRAG_NEEDED
is never sent.