[PATCH net-next 0/2] flow_dissector: Fix MPLS parsing and add ext hdr support

[PATCH net-next 0/2] flow_dissector: Fix MPLS parsing and add ext hdr support

[Tom Herbert]

Need to shift label. Added parsing of dst, hop-by-hop, and routing
extension headers.


Tom Herbert (2):
  flow_dissector: Fix MPLS entropy label handling in flow dissector
  flow_dissector: add support for dst, hop-by-hop and routing ext hdrs

 net/core/flow_dissector.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

-- 
1.8.1

[PATCH net-next 1/2] flow_dissector: Fix MPLS entropy label handling in flow dissector

[Tom Herbert]

Need to shift after masking to get label value for comparison.

Fixes: b3baa0fbd02a1a9d493d8 ("mpls: Add MPLS entropy label in flow_keys")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 net/core/flow_dissector.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 77e22e4..1818cdc 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -299,8 +299,8 @@ mpls:
 		if (!hdr)
 			return false;
 
-		if ((ntohl(hdr[0].entry) & MPLS_LS_LABEL_MASK) ==
-		     MPLS_LABEL_ENTROPY) {
+		if ((ntohl(hdr[0].entry) & MPLS_LS_LABEL_MASK) >>
+		     MPLS_LS_LABEL_SHIFT == MPLS_LABEL_ENTROPY) {
 			if (skb_flow_dissector_uses_key(flow_dissector,
 							FLOW_DISSECTOR_KEY_MPLS_ENTROPY)) {
 				key_keyid = skb_flow_dissector_target(flow_dissector,
-- 
1.8.1

[PATCH net-next 2/2] flow_dissector: add support for dst, hop-by-hop and routing ext hdrs

[Tom Herbert]

If dst, hop-by-hop or routing extension headers are present determine
length of the options and skip over them in flow dissection.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 net/core/flow_dissector.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 1818cdc..22e4dff 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -327,6 +327,7 @@ mpls:
 		return false;
 	}
 
+ip_proto_again:
 	switch (ip_proto) {
 	case IPPROTO_GRE: {
 		struct gre_hdr {
@@ -383,6 +384,22 @@ mpls:
 		}
 		goto again;
 	}
+	case NEXTHDR_HOP:
+	case NEXTHDR_ROUTING:
+	case NEXTHDR_DEST: {
+		u8 _opthdr[2], *opthdr;
+
+		if (proto != htons(ETH_P_IPV6))
+			break;
+
+		opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
+					      data, hlen, &_opthdr);
+
+		ip_proto = _opthdr[0];
+		nhoff += (_opthdr[1] + 1) << 3;
+
+		goto ip_proto_again;
+	}
 	case IPPROTO_IPIP:
 		proto = htons(ETH_P_IP);
 		goto ip;
-- 
1.8.1

Re: [PATCH net-next 0/2] flow_dissector: Fix MPLS parsing and add ext hdr support

[David Miller]

From: Tom Herbert <tom@herbertland.com>
Date: Fri, 12 Jun 2015 09:01:04 -0700

> Need to shift label. Added parsing of dst, hop-by-hop, and routing
> extension headers.

Series applied, thanks Tom.

Re: [PATCH net-next 2/2] flow_dissector: add support for dst, hop-by-hop and routing ext hdrs

[Alexei Starovoitov]

On Fri, Jun 12, 2015 at 09:01:06AM -0700, Tom Herbert wrote:
> If dst, hop-by-hop or routing extension headers are present determine
> length of the options and skip over them in flow dissection.
> 
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> ---
>  net/core/flow_dissector.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 1818cdc..22e4dff 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -327,6 +327,7 @@ mpls:
>  		return false;
>  	}
>  
> +ip_proto_again:
>  	switch (ip_proto) {
>  	case IPPROTO_GRE: {
>  		struct gre_hdr {
> @@ -383,6 +384,22 @@ mpls:
>  		}
>  		goto again;
>  	}
> +	case NEXTHDR_HOP:
> +	case NEXTHDR_ROUTING:
> +	case NEXTHDR_DEST: {
> +		u8 _opthdr[2], *opthdr;
> +
> +		if (proto != htons(ETH_P_IPV6))
> +			break;
> +
> +		opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
> +					      data, hlen, &_opthdr);
> +
> +		ip_proto = _opthdr[0];
> +		nhoff += (_opthdr[1] + 1) << 3;
> +
> +		goto ip_proto_again;
> +	}

Dave,

please revert it. My server locks up during boot with:

[   32.391955] NMI watchdog: BUG: soft lockup - CPU#2 stuck for 23s! [modprobe:1550]
[   32.392043] RIP: 0010:[<ffffffff815cd8e2>]  [<ffffffff815cd8e2>] skb_copy_bits+0x12/0x260
[   32.392060] Call Trace:
[   32.392061]  <IRQ>
[   32.392063]  [<ffffffff815d9f38>] __skb_flow_dissect+0x358/0x820
[   32.392064]  [<ffffffff815da48e>] __skb_get_hash+0x8e/0x2e0
[   32.392066]  [<ffffffff815def7b>] __skb_tx_hash+0x5b/0xb0
[   32.392067]  [<ffffffff815df54a>] __netdev_pick_tx+0x18a/0x1a0
[   32.392068]  [<ffffffff815df40a>] ? __netdev_pick_tx+0x4a/0x1a0
[   32.392069]  [<ffffffff815e4db0>] ? __dev_queue_xmit+0x50/0x620
[   32.392071]  [<ffffffff815e4d0b>] netdev_pick_tx+0xcb/0x120
[   32.392072]  [<ffffffff815e4e08>] __dev_queue_xmit+0xa8/0x620
[   32.392073]  [<ffffffff815e4db0>] ? __dev_queue_xmit+0x50/0x620
[   32.392076]  [<ffffffff81698225>] ? ip6_finish_output+0xa5/0x1e0
[   32.392077]  [<ffffffff815e53a3>] dev_queue_xmit_sk+0x13/0x20
[   32.392078]  [<ffffffff81696144>] ip6_finish_output2+0x464/0x5f0
[   32.392079]  [<ffffffff81698225>] ? ip6_finish_output+0xa5/0x1e0
[   32.392081]  [<ffffffff816a5bf2>] ? ip6_mtu+0xb2/0xd0
[   32.392082]  [<ffffffff816a5b80>] ? ip6_mtu+0x40/0xd0
[   32.392083]  [<ffffffff81698225>] ip6_finish_output+0xa5/0x1e0
[   32.392084]  [<ffffffff816983be>] ip6_output+0x5e/0x1b0

Re: [PATCH net-next 2/2] flow_dissector: add support for dst, hop-by-hop and routing ext hdrs

[Eric Dumazet]

On Fri, 2015-06-12 at 18:27 -0700, Alexei Starovoitov wrote:
> On Fri, Jun 12, 2015 at 09:01:06AM -0700, Tom Herbert wrote:
> > If dst, hop-by-hop or routing extension headers are present determine
> > length of the options and skip over them in flow dissection.
> > 
> > Signed-off-by: Tom Herbert <tom@herbertland.com>
> > ---
> >  net/core/flow_dissector.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 1818cdc..22e4dff 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -327,6 +327,7 @@ mpls:
> >  		return false;
> >  	}
> >  
> > +ip_proto_again:
> >  	switch (ip_proto) {
> >  	case IPPROTO_GRE: {
> >  		struct gre_hdr {
> > @@ -383,6 +384,22 @@ mpls:
> >  		}
> >  		goto again;
> >  	}
> > +	case NEXTHDR_HOP:
> > +	case NEXTHDR_ROUTING:
> > +	case NEXTHDR_DEST: {
> > +		u8 _opthdr[2], *opthdr;
> > +
> > +		if (proto != htons(ETH_P_IPV6))
> > +			break;
> > +
> > +		opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
> > +					      data, hlen, &_opthdr);
> > +
> > +		ip_proto = _opthdr[0];
> > +		nhoff += (_opthdr[1] + 1) << 3;
> > +
> > +		goto ip_proto_again;
> > +	}
> 
> Dave,
> 
> please revert it. My server locks up during boot with:

Seems easy to fix instead ?

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 22e4dffa0c8b3b9a20a7324eae1627313e14ce30..476e5dda59e19822dba98a931369ff2666c59c0d 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -394,9 +394,11 @@ ip_proto_again:
 
 		opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
 					      data, hlen, &_opthdr);
+		if (!opthdr)
+			return false;
 
-		ip_proto = _opthdr[0];
-		nhoff += (_opthdr[1] + 1) << 3;
+		ip_proto = opthdr[0];
+		nhoff += (opthdr[1] + 1) << 3;
 
 		goto ip_proto_again;
 	}

Re: [PATCH net-next 2/2] flow_dissector: add support for dst, hop-by-hop and routing ext hdrs

[Alexei Starovoitov]

On Fri, Jun 12, 2015 at 06:37:34PM -0700, Eric Dumazet wrote:
> On Fri, 2015-06-12 at 18:27 -0700, Alexei Starovoitov wrote:
> > On Fri, Jun 12, 2015 at 09:01:06AM -0700, Tom Herbert wrote:
> > > If dst, hop-by-hop or routing extension headers are present determine
> > > length of the options and skip over them in flow dissection.
> > > 
> > > Signed-off-by: Tom Herbert <tom@herbertland.com>
> > > ---
> > >  net/core/flow_dissector.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > > index 1818cdc..22e4dff 100644
> > > --- a/net/core/flow_dissector.c
> > > +++ b/net/core/flow_dissector.c
> > > @@ -327,6 +327,7 @@ mpls:
> > >  		return false;
> > >  	}
> > >  
> > > +ip_proto_again:
> > >  	switch (ip_proto) {
> > >  	case IPPROTO_GRE: {
> > >  		struct gre_hdr {
> > > @@ -383,6 +384,22 @@ mpls:
> > >  		}
> > >  		goto again;
> > >  	}
> > > +	case NEXTHDR_HOP:
> > > +	case NEXTHDR_ROUTING:
> > > +	case NEXTHDR_DEST: {
> > > +		u8 _opthdr[2], *opthdr;
> > > +
> > > +		if (proto != htons(ETH_P_IPV6))
> > > +			break;
> > > +
> > > +		opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
> > > +					      data, hlen, &_opthdr);
> > > +
> > > +		ip_proto = _opthdr[0];
> > > +		nhoff += (_opthdr[1] + 1) << 3;
> > > +
> > > +		goto ip_proto_again;
> > > +	}
> > 
> > Dave,
> > 
> > please revert it. My server locks up during boot with:
> 
> Seems easy to fix instead ?
> 
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 22e4dffa0c8b3b9a20a7324eae1627313e14ce30..476e5dda59e19822dba98a931369ff2666c59c0d 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -394,9 +394,11 @@ ip_proto_again:
>  
>  		opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
>  					      data, hlen, &_opthdr);
> +		if (!opthdr)
> +			return false;
>  
> -		ip_proto = _opthdr[0];
> -		nhoff += (_opthdr[1] + 1) << 3;
> +		ip_proto = opthdr[0];
> +		nhoff += (opthdr[1] + 1) << 3;
>  
>  		goto ip_proto_again;
>  	}
> 

sure, that's better.
If you're going to submit it officialy, please add my Tested-by.
My server is happy now :)

Re: [PATCH net-next 2/2] flow_dissector: add support for dst, hop-by-hop and routing ext hdrs

[Eric Dumazet]

On Fri, 2015-06-12 at 18:50 -0700, Alexei Starovoitov wrote:

> 
> sure, that's better.
> If you're going to submit it officialy, please add my Tested-by.
> My server is happy now :)

Sure , will do.

I tried adding __must_check to __skb_header_pointer() but apparently had
to use W=1 to get a warning :

make W=1 net/core/
  CC      net/core/flow_dissector.o
net/core/flow_dissector.c: In function ‘__skb_flow_dissect’:
net/core/flow_dissector.c:390:19: warning: variable ‘opthdr’ set but not
used [-Wunused-but-set-variable]
   u8 _opthdr[2], *opthdr;


diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index cc612fc0a8943ec853b92e6b3516b0e5582299e2..45252c4f49e4020eec523273f23f65ee87cc0bd5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2743,8 +2743,9 @@ __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
 		    __wsum csum);
 
-static inline void *__skb_header_pointer(const struct sk_buff *skb, int offset,
-					 int len, void *data, int hlen, void *buffer)
+static inline void * __must_check
+__skb_header_pointer(const struct sk_buff *skb, int offset,
+		     int len, void *data, int hlen, void *buffer)
 {
 	if (hlen - offset >= len)
 		return data + offset;

Re: [PATCH net-next 2/2] flow_dissector: add support for dst, hop-by-hop and routing ext hdrs

[Alexei Starovoitov]

On Fri, Jun 12, 2015 at 07:11:16PM -0700, Eric Dumazet wrote:
> On Fri, 2015-06-12 at 18:50 -0700, Alexei Starovoitov wrote:
> 
> > 
> > sure, that's better.
> > If you're going to submit it officialy, please add my Tested-by.
> > My server is happy now :)
> 
> Sure , will do.
> 
> I tried adding __must_check to __skb_header_pointer() but apparently had
> to use W=1 to get a warning :

that is great idea still. At least buildbot can pick it up.

[PATCH net-next] flow_dissector: fix ipv6 dst, hop-by-hop and routing ext hdrs

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

__skb_header_pointer() returns a pointer that must be checked.

Fixes infinite loop reported by Alexei, and add __must_check to
catch these errors earlier.

Fixes: 6a74fcf426f5 ("flow_dissector: add support for dst, hop-by-hop and routing ext hdrs")
Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Tested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/linux/skbuff.h    |    9 +++++----
 net/core/flow_dissector.c |    6 ++++--
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index cc612fc0a8943ec853b92e6b3516b0e5582299e2..a7acc92aa6685d7006077510697e3d9481b02588 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2743,8 +2743,9 @@ __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
 __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
 		    __wsum csum);
 
-static inline void *__skb_header_pointer(const struct sk_buff *skb, int offset,
-					 int len, void *data, int hlen, void *buffer)
+static inline void * __must_check
+__skb_header_pointer(const struct sk_buff *skb, int offset,
+		     int len, void *data, int hlen, void *buffer)
 {
 	if (hlen - offset >= len)
 		return data + offset;
@@ -2756,8 +2757,8 @@ static inline void *__skb_header_pointer(const struct sk_buff *skb, int offset,
 	return buffer;
 }
 
-static inline void *skb_header_pointer(const struct sk_buff *skb, int offset,
-				       int len, void *buffer)
+static inline void * __must_check
+skb_header_pointer(const struct sk_buff *skb, int offset, int len, void *buffer)
 {
 	return __skb_header_pointer(skb, offset, len, skb->data,
 				    skb_headlen(skb), buffer);
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 22e4dffa0c8b3b9a20a7324eae1627313e14ce30..476e5dda59e19822dba98a931369ff2666c59c0d 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -394,9 +394,11 @@ ip_proto_again:
 
 		opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
 					      data, hlen, &_opthdr);
+		if (!opthdr)
+			return false;
 
-		ip_proto = _opthdr[0];
-		nhoff += (_opthdr[1] + 1) << 3;
+		ip_proto = opthdr[0];
+		nhoff += (opthdr[1] + 1) << 3;
 
 		goto ip_proto_again;
 	}

Re: [PATCH net-next] flow_dissector: fix ipv6 dst, hop-by-hop and routing ext hdrs

[Tom Herbert]

On Fri, Jun 12, 2015 at 7:31 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> __skb_header_pointer() returns a pointer that must be checked.
>
> Fixes infinite loop reported by Alexei, and add __must_check to
> catch these errors earlier.
>
> Fixes: 6a74fcf426f5 ("flow_dissector: add support for dst, hop-by-hop and routing ext hdrs")
> Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Tested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  include/linux/skbuff.h    |    9 +++++----
>  net/core/flow_dissector.c |    6 ++++--
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index cc612fc0a8943ec853b92e6b3516b0e5582299e2..a7acc92aa6685d7006077510697e3d9481b02588 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2743,8 +2743,9 @@ __wsum __skb_checksum(const struct sk_buff *skb, int offset, int len,
>  __wsum skb_checksum(const struct sk_buff *skb, int offset, int len,
>                     __wsum csum);
>
> -static inline void *__skb_header_pointer(const struct sk_buff *skb, int offset,
> -                                        int len, void *data, int hlen, void *buffer)
> +static inline void * __must_check
> +__skb_header_pointer(const struct sk_buff *skb, int offset,
> +                    int len, void *data, int hlen, void *buffer)
>  {
>         if (hlen - offset >= len)
>                 return data + offset;
> @@ -2756,8 +2757,8 @@ static inline void *__skb_header_pointer(const struct sk_buff *skb, int offset,
>         return buffer;
>  }
>
> -static inline void *skb_header_pointer(const struct sk_buff *skb, int offset,
> -                                      int len, void *buffer)
> +static inline void * __must_check
> +skb_header_pointer(const struct sk_buff *skb, int offset, int len, void *buffer)
>  {
>         return __skb_header_pointer(skb, offset, len, skb->data,
>                                     skb_headlen(skb), buffer);
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 22e4dffa0c8b3b9a20a7324eae1627313e14ce30..476e5dda59e19822dba98a931369ff2666c59c0d 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -394,9 +394,11 @@ ip_proto_again:
>
>                 opthdr = __skb_header_pointer(skb, nhoff, sizeof(_opthdr),
>                                               data, hlen, &_opthdr);
> +               if (!opthdr)
> +                       return false;
>
> -               ip_proto = _opthdr[0];
> -               nhoff += (_opthdr[1] + 1) << 3;
> +               ip_proto = opthdr[0];
> +               nhoff += (opthdr[1] + 1) << 3;
>
>                 goto ip_proto_again;
>         }
>
>

Acked-by: Tom Herbert <tom@herbertland.com>

Re: [PATCH net-next] flow_dissector: fix ipv6 dst, hop-by-hop and routing ext hdrs

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 12 Jun 2015 19:31:32 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> __skb_header_pointer() returns a pointer that must be checked.
> 
> Fixes infinite loop reported by Alexei, and add __must_check to
> catch these errors earlier.
> 
> Fixes: 6a74fcf426f5 ("flow_dissector: add support for dst, hop-by-hop and routing ext hdrs")
> Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Tested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.