[PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[Paolo Abeni]

Before the commit 0dfe17823945 ("net: vlan: goto another_round
instead of calling __netif_receive_skb"), on tagged skb ingress,
ptype specific protocol matches were delivered only to the
related vlan device, if any.
After said commit, jumping back to the 'another_round' label, allows
the later ptype specific check to match both orig_dev and skb->dev,
delivering the skb to both the vlan device and the underlying
device.
This cause i.e. packet sockets bound to a specific protocol type on
one of said devices to receive also frames really targeting the
other device.
This commit resets orig_dev before performing another round due to
vlan processing, allowing the skb to be delivered once again only
to the vlan specific ptypes.

Fixes: 0dfe17823945 ("net: vlan: goto another_round instead of calling __netif_receive_skb")
Reported-by: Ryan Liu <Ryan.Liu@alcatel-lucent.com>
Reported-by: Cliff Chen <Cliff.Chen@alcatel-lucent.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/core/dev.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 904ff43..9d08dd6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4144,10 +4144,15 @@ ncls:
 			ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = NULL;
 		}
-		if (vlan_do_receive(&skb))
+		if (vlan_do_receive(&skb)) {
+			/* avoid delivering to ptype registered on
+			 * vlan's underlying device only
+			 */
+			orig_dev = skb->dev;
 			goto another_round;
-		else if (unlikely(!skb))
+		} else if (unlikely(!skb)) {
 			goto out;
+		}
 	}
 
 	rx_handler = rcu_dereference(skb->dev->rx_handler);
-- 
1.8.3.1

Re: [PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[Jiri Pirko]

Wed, Jun 22, 2016 at 12:25:15PM CEST, pabeni@redhat.com wrote:
>Before the commit 0dfe17823945 ("net: vlan: goto another_round
>instead of calling __netif_receive_skb"), on tagged skb ingress,
>ptype specific protocol matches were delivered only to the
>related vlan device, if any.
>After said commit, jumping back to the 'another_round' label, allows
>the later ptype specific check to match both orig_dev and skb->dev,
>delivering the skb to both the vlan device and the underlying
>device.
>This cause i.e. packet sockets bound to a specific protocol type on
>one of said devices to receive also frames really targeting the
>other device.
>This commit resets orig_dev before performing another round due to
>vlan processing, allowing the skb to be delivered once again only
>to the vlan specific ptypes.

I don't get why vlan should behave differently in this comparing to
other stacked devices like bond/team/br etc.

Could you please explain?


>
>Fixes: 0dfe17823945 ("net: vlan: goto another_round instead of calling __netif_receive_skb")
>Reported-by: Ryan Liu <Ryan.Liu@alcatel-lucent.com>
>Reported-by: Cliff Chen <Cliff.Chen@alcatel-lucent.com>
>Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>---
> net/core/dev.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 904ff43..9d08dd6 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -4144,10 +4144,15 @@ ncls:
> 			ret = deliver_skb(skb, pt_prev, orig_dev);
> 			pt_prev = NULL;
> 		}
>-		if (vlan_do_receive(&skb))
>+		if (vlan_do_receive(&skb)) {
>+			/* avoid delivering to ptype registered on
>+			 * vlan's underlying device only
>+			 */
>+			orig_dev = skb->dev;
> 			goto another_round;
>-		else if (unlikely(!skb))
>+		} else if (unlikely(!skb)) {
> 			goto out;
>+		}
> 	}
> 
> 	rx_handler = rcu_dereference(skb->dev->rx_handler);
>-- 
>1.8.3.1
>

Re: [PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[Paolo Abeni]

On Wed, 2016-06-22 at 12:55 +0200, Jiri Pirko wrote:
> Wed, Jun 22, 2016 at 12:25:15PM CEST, pabeni@redhat.com wrote:
> >Before the commit 0dfe17823945 ("net: vlan: goto another_round
> >instead of calling __netif_receive_skb"), on tagged skb ingress,
> >ptype specific protocol matches were delivered only to the
> >related vlan device, if any.
> >After said commit, jumping back to the 'another_round' label, allows
> >the later ptype specific check to match both orig_dev and skb->dev,
> >delivering the skb to both the vlan device and the underlying
> >device.
> >This cause i.e. packet sockets bound to a specific protocol type on
> >one of said devices to receive also frames really targeting the
> >other device.
> >This commit resets orig_dev before performing another round due to
> >vlan processing, allowing the skb to be delivered once again only
> >to the vlan specific ptypes.
> 
> I don't get why vlan should behave differently in this comparing to
> other stacked devices like bond/team/br etc.
> 
> Could you please explain?

vlan behavior has changed with the commit 0dfe17823945 (which is old,
but still...), this patch is trying to restore the old one.

The new behavior fouls existing applications which where expecting ptype
specific match to be delivered only on the relevant device. 

AFAIC jumping back to the 'another_round' is possible for team, bond,
macvlan, macsec and ipvlan device, beyond vlan devices. I think the
latter should be treated differently, beyond history reasons, because
the vlan device processing, stripping the vlan tag, actually changes the
ethernet type present into the ethernet header in respect to the wire
packet layout; strict match on that value should give different results
before and after vlan stripping.

Paolo

Re: [PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[Jiri Pirko]

Wed, Jun 22, 2016 at 02:21:29PM CEST, pabeni@redhat.com wrote:
>On Wed, 2016-06-22 at 12:55 +0200, Jiri Pirko wrote:
>> Wed, Jun 22, 2016 at 12:25:15PM CEST, pabeni@redhat.com wrote:
>> >Before the commit 0dfe17823945 ("net: vlan: goto another_round
>> >instead of calling __netif_receive_skb"), on tagged skb ingress,
>> >ptype specific protocol matches were delivered only to the
>> >related vlan device, if any.
>> >After said commit, jumping back to the 'another_round' label, allows
>> >the later ptype specific check to match both orig_dev and skb->dev,
>> >delivering the skb to both the vlan device and the underlying
>> >device.
>> >This cause i.e. packet sockets bound to a specific protocol type on
>> >one of said devices to receive also frames really targeting the
>> >other device.
>> >This commit resets orig_dev before performing another round due to
>> >vlan processing, allowing the skb to be delivered once again only
>> >to the vlan specific ptypes.
>> 
>> I don't get why vlan should behave differently in this comparing to
>> other stacked devices like bond/team/br etc.
>> 
>> Could you please explain?
>
>vlan behavior has changed with the commit 0dfe17823945 (which is old,
>but still...), this patch is trying to restore the old one.
>
>The new behavior fouls existing applications which where expecting ptype
>specific match to be delivered only on the relevant device. 

The patch only unified the behaviour so everyone know what to expect
regardless of the device type.


>
>AFAIC jumping back to the 'another_round' is possible for team, bond,
>macvlan, macsec and ipvlan device, beyond vlan devices. I think the
>latter should be treated differently, beyond history reasons, because
>the vlan device processing, stripping the vlan tag, actually changes the
>ethernet type present into the ethernet header in respect to the wire
>packet layout; strict match on that value should give different results
>before and after vlan stripping.

The vlan header stripping happens before all of this, either in HW or
skb_vlan_untag, so you can never match on that.

Re: [PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 22 Jun 2016 14:40:50 +0200

> The patch only unified the behaviour so everyone know what to expect
> regardless of the device type.

+1

And reverting this behavior back is going to hurt more people than it
will help, so I am not going to apply this, sorry.

Re: [PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[Paolo Abeni]

On Sun, 2016-06-26 at 16:06 -0400, David Miller wrote:
> From: Jiri Pirko <jiri@resnulli.us>
> Date: Wed, 22 Jun 2016 14:40:50 +0200
> 
> > The patch only unified the behaviour so everyone know what to expect
> > regardless of the device type.
> 
> +1
> 
> And reverting this behavior back is going to hurt more people than it
> will help, so I am not going to apply this, sorry.

Would a /proc/net/vlan sysctl to control the switch from the new to the
old behavior (with default to the new) make this acceptable ?

Recent users will not be affected, and users of the old behavior could
manage kernel upgrade without breaking their applications. 

Thank you,

Paolo

Re: [PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[Jiri Pirko]

Mon, Jun 27, 2016 at 11:28:32AM CEST, pabeni@redhat.com wrote:
>On Sun, 2016-06-26 at 16:06 -0400, David Miller wrote:
>> From: Jiri Pirko <jiri@resnulli.us>
>> Date: Wed, 22 Jun 2016 14:40:50 +0200
>> 
>> > The patch only unified the behaviour so everyone know what to expect
>> > regardless of the device type.
>> 
>> +1
>> 
>> And reverting this behavior back is going to hurt more people than it
>> will help, so I am not going to apply this, sorry.
>
>Would a /proc/net/vlan sysctl to control the switch from the new to the
>old behavior (with default to the new) make this acceptable ?

Ugh, please don't :(

>
>Recent users will not be affected, and users of the old behavior could
>manage kernel upgrade without breaking their applications. 
>
>Thank you,
>
>Paolo
>
>

Re: [PATCH net] net: avoid vlan ptype specific match to be leaked on real device

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Mon, 27 Jun 2016 11:28:32 +0200

> On Sun, 2016-06-26 at 16:06 -0400, David Miller wrote:
>> From: Jiri Pirko <jiri@resnulli.us>
>> Date: Wed, 22 Jun 2016 14:40:50 +0200
>> 
>> > The patch only unified the behaviour so everyone know what to expect
>> > regardless of the device type.
>> 
>> +1
>> 
>> And reverting this behavior back is going to hurt more people than it
>> will help, so I am not going to apply this, sorry.
> 
> Would a /proc/net/vlan sysctl to control the switch from the new to the
> old behavior (with default to the new) make this acceptable ?

No, sorry.  Please fix your applications while you have your attention
on the issue and have the opportunity and impetus to do so.