[net-next PATCH 0/5] Flow dissector fixes and improvements

[net-next PATCH 0/5] Flow dissector fixes and improvements

[Alexander Duyck]

This patch series is meant to fix and/or improve a number of items within
the flow dissector code.  The main change out of all of this is that IPv4
and IPv6 fragmentation should now be handled better than it was.  As a
result we should see an improvement when handling things like IP fragment
reassembly as the skbs should now only have header data in the linear
portion of the buffer while the fragments will only hold payload data.

---

Alexander Duyck (5):
      flow_dissector: Check for IP fragmentation even if not using IPv4 address
      flow_dissector: Fix fragment handling for header length computation
      flow_dissector: Correctly handle parsing FCoE
      flow_dissector: Use same pointer for IPv4 and IPv6 addresses
      eth: Pull header from first fragment via eth_get_headlen


 net/core/flow_dissector.c |   45 ++++++++++++++++++++++++++-------------------
 net/ethernet/eth.c        |    3 ++-
 2 files changed, 28 insertions(+), 20 deletions(-)

[net-next PATCH 1/5] flow_dissector: Check for IP fragmentation even if not using IPv4 address

[Alexander Duyck]

This patch corrects the logic for the IPv4 parsing so that it is consistent
with how we handle IPv6.  Specifically if we do not have the flow key
indicating we want the addresses we still may need to take a look at the IP
fragmentation bits and to see if we should stop after we have recognized
the L3 header.

Fixes: 807e165dc44f ("flow_dissector: Add control/reporting of fragmentation")
Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/flow_dissector.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 12e700332010..1f88f8280280 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -178,15 +178,16 @@ ip:
 
 		ip_proto = iph->protocol;
 
-		if (!dissector_uses_key(flow_dissector,
-					FLOW_DISSECTOR_KEY_IPV4_ADDRS))
-			break;
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_IPV4_ADDRS)) {
+			key_addrs = skb_flow_dissector_target(flow_dissector,
+							      FLOW_DISSECTOR_KEY_IPV4_ADDRS,
+							      target_container);
 
-		key_addrs = skb_flow_dissector_target(flow_dissector,
-			      FLOW_DISSECTOR_KEY_IPV4_ADDRS, target_container);
-		memcpy(&key_addrs->v4addrs, &iph->saddr,
-		       sizeof(key_addrs->v4addrs));
-		key_control->addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
+			memcpy(&key_addrs->v4addrs, &iph->saddr,
+			       sizeof(key_addrs->v4addrs));
+			key_control->addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
+		}
 
 		if (ip_is_fragment(iph)) {
 			key_control->flags |= FLOW_DIS_IS_FRAGMENT;

[net-next PATCH 2/5] flow_dissector: Fix fragment handling for header length computation

[Alexander Duyck]

It turns out that for IPv4 we were reporting the ip_proto of the fragment,
and for IPv6 we were not.  This patch updates that behavior so that we
always report the IP protocol of the fragment.  In addition it takes the
steps of updating the payload offset code so that we will determine the
start of the payload not including the L4 header for any fragment after the
first.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/flow_dissector.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 1f88f8280280..8bd745f72734 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -448,13 +448,12 @@ ip_proto_again:
 		key_control->flags |= FLOW_DIS_IS_FRAGMENT;
 
 		nhoff += sizeof(_fh);
+		ip_proto = fh->nexthdr;
 
 		if (!(fh->frag_off & htons(IP6_OFFSET))) {
 			key_control->flags |= FLOW_DIS_FIRST_FRAG;
-			if (flags & FLOW_DISSECTOR_F_PARSE_1ST_FRAG) {
-				ip_proto = fh->nexthdr;
+			if (flags & FLOW_DISSECTOR_F_PARSE_1ST_FRAG)
 				goto ip_proto_again;
-			}
 		}
 		goto out_good;
 	}
@@ -741,6 +740,11 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
 {
 	u32 poff = keys->control.thoff;
 
+	/* skip L4 headers for fragments after the first */
+	if ((keys->control.flags & FLOW_DIS_IS_FRAGMENT) &&
+	    !(keys->control.flags & FLOW_DIS_FIRST_FRAG))
+		return poff;
+
 	switch (keys->basic.ip_proto) {
 	case IPPROTO_TCP: {
 		/* access doff as u8 to avoid unaligned access */

[net-next PATCH 3/5] flow_dissector: Correctly handle parsing FCoE

[Alexander Duyck]

The flow dissector bits handling FCoE didn't bother to actually validate
that the space there was enough for the FCoE header.  So we need to update
things so that if there is room we add the header and report a good result,
otherwise we do not add the header, and report the bad result.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/flow_dissector.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 8bd745f72734..6288153d7f36 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -340,8 +340,11 @@ mpls:
 	}
 
 	case htons(ETH_P_FCOE):
-		key_control->thoff = (u16)(nhoff + FCOE_HEADER_LEN);
-		/* fall through */
+		if ((hlen - nhoff) < FCOE_HEADER_LEN)
+			goto out_bad;
+
+		nhoff += FCOE_HEADER_LEN;
+		goto out_good;
 	default:
 		goto out_bad;
 	}

[net-next PATCH 4/5] flow_dissector: Use same pointer for IPv4 and IPv6 addresses

[Alexander Duyck]

The IPv6 parsing was using a local pointer when it could use the same
pointer as the IPv4 portion of the code since the key_addrs can support
both IPv4 and IPv6 as it is just a pointer.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/core/flow_dissector.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 6288153d7f36..7c7b8739b8b8 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -220,13 +220,12 @@ ipv6:
 
 		if (dissector_uses_key(flow_dissector,
 				       FLOW_DISSECTOR_KEY_IPV6_ADDRS)) {
-			struct flow_dissector_key_ipv6_addrs *key_ipv6_addrs;
-
-			key_ipv6_addrs = skb_flow_dissector_target(flow_dissector,
-								   FLOW_DISSECTOR_KEY_IPV6_ADDRS,
-								   target_container);
+			key_addrs = skb_flow_dissector_target(flow_dissector,
+							      FLOW_DISSECTOR_KEY_IPV6_ADDRS,
+							      target_container);
 
-			memcpy(key_ipv6_addrs, &iph->saddr, sizeof(*key_ipv6_addrs));
+			memcpy(&key_addrs->v6addrs, &iph->saddr,
+			       sizeof(key_addrs->v6addrs));
 			key_control->addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
 		}
 

[net-next PATCH 5/5] eth: Pull header from first fragment via eth_get_headlen

[Alexander Duyck]

We want to try and pull the L4 header in if it is available in the first
fragment.  As such add the flag to indicate we want to pull the headers on
the first fragment in.

Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
---
 net/ethernet/eth.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 103871784e50..66dff5e3d772 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -125,6 +125,7 @@ EXPORT_SYMBOL(eth_header);
  */
 u32 eth_get_headlen(void *data, unsigned int len)
 {
+	const unsigned int flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG;
 	const struct ethhdr *eth = (const struct ethhdr *)data;
 	struct flow_keys keys;
 
@@ -134,7 +135,7 @@ u32 eth_get_headlen(void *data, unsigned int len)
 
 	/* parse any remaining L2/L3 headers, check for L4 */
 	if (!skb_flow_dissect_flow_keys_buf(&keys, data, eth->h_proto,
-					    sizeof(*eth), len, 0))
+					    sizeof(*eth), len, flags))
 		return max_t(u32, keys.control.thoff, sizeof(*eth));
 
 	/* parse for any L4 headers */

Re: [net-next PATCH 1/5] flow_dissector: Check for IP fragmentation even if not using IPv4 address

[Tom Herbert]

On Wed, Feb 24, 2016 at 9:29 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
> This patch corrects the logic for the IPv4 parsing so that it is consistent
> with how we handle IPv6.  Specifically if we do not have the flow key
> indicating we want the addresses we still may need to take a look at the IP
> fragmentation bits and to see if we should stop after we have recognized
> the L3 header.
>
> Fixes: 807e165dc44f ("flow_dissector: Add control/reporting of fragmentation")
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

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

> ---
>  net/core/flow_dissector.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 12e700332010..1f88f8280280 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -178,15 +178,16 @@ ip:
>
>                 ip_proto = iph->protocol;
>
> -               if (!dissector_uses_key(flow_dissector,
> -                                       FLOW_DISSECTOR_KEY_IPV4_ADDRS))
> -                       break;
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_IPV4_ADDRS)) {
> +                       key_addrs = skb_flow_dissector_target(flow_dissector,
> +                                                             FLOW_DISSECTOR_KEY_IPV4_ADDRS,
> +                                                             target_container);
>
> -               key_addrs = skb_flow_dissector_target(flow_dissector,
> -                             FLOW_DISSECTOR_KEY_IPV4_ADDRS, target_container);
> -               memcpy(&key_addrs->v4addrs, &iph->saddr,
> -                      sizeof(key_addrs->v4addrs));
> -               key_control->addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> +                       memcpy(&key_addrs->v4addrs, &iph->saddr,
> +                              sizeof(key_addrs->v4addrs));
> +                       key_control->addr_type = FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> +               }
>
>                 if (ip_is_fragment(iph)) {
>                         key_control->flags |= FLOW_DIS_IS_FRAGMENT;
>

Re: [net-next PATCH 2/5] flow_dissector: Fix fragment handling for header length computation

[Tom Herbert]

On Wed, Feb 24, 2016 at 9:29 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
> It turns out that for IPv4 we were reporting the ip_proto of the fragment,
> and for IPv6 we were not.  This patch updates that behavior so that we
> always report the IP protocol of the fragment.  In addition it takes the
> steps of updating the payload offset code so that we will determine the
> start of the payload not including the L4 header for any fragment after the
> first.
>
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

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

> ---
>  net/core/flow_dissector.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 1f88f8280280..8bd745f72734 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -448,13 +448,12 @@ ip_proto_again:
>                 key_control->flags |= FLOW_DIS_IS_FRAGMENT;
>
>                 nhoff += sizeof(_fh);
> +               ip_proto = fh->nexthdr;
>
>                 if (!(fh->frag_off & htons(IP6_OFFSET))) {
>                         key_control->flags |= FLOW_DIS_FIRST_FRAG;
> -                       if (flags & FLOW_DISSECTOR_F_PARSE_1ST_FRAG) {
> -                               ip_proto = fh->nexthdr;
> +                       if (flags & FLOW_DISSECTOR_F_PARSE_1ST_FRAG)
>                                 goto ip_proto_again;
> -                       }
>                 }
>                 goto out_good;
>         }
> @@ -741,6 +740,11 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
>  {
>         u32 poff = keys->control.thoff;
>
> +       /* skip L4 headers for fragments after the first */
> +       if ((keys->control.flags & FLOW_DIS_IS_FRAGMENT) &&
> +           !(keys->control.flags & FLOW_DIS_FIRST_FRAG))
> +               return poff;
> +
>         switch (keys->basic.ip_proto) {
>         case IPPROTO_TCP: {
>                 /* access doff as u8 to avoid unaligned access */
>

Re: [net-next PATCH 3/5] flow_dissector: Correctly handle parsing FCoE

[Tom Herbert]

On Wed, Feb 24, 2016 at 9:29 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
> The flow dissector bits handling FCoE didn't bother to actually validate
> that the space there was enough for the FCoE header.  So we need to update
> things so that if there is room we add the header and report a good result,
> otherwise we do not add the header, and report the bad result.
>
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

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

> ---
>  net/core/flow_dissector.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 8bd745f72734..6288153d7f36 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -340,8 +340,11 @@ mpls:
>         }
>
>         case htons(ETH_P_FCOE):
> -               key_control->thoff = (u16)(nhoff + FCOE_HEADER_LEN);
> -               /* fall through */
> +               if ((hlen - nhoff) < FCOE_HEADER_LEN)
> +                       goto out_bad;
> +
> +               nhoff += FCOE_HEADER_LEN;
> +               goto out_good;
>         default:
>                 goto out_bad;
>         }
>

Re: [net-next PATCH 4/5] flow_dissector: Use same pointer for IPv4 and IPv6 addresses

[Tom Herbert]

On Wed, Feb 24, 2016 at 9:29 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
> The IPv6 parsing was using a local pointer when it could use the same
> pointer as the IPv4 portion of the code since the key_addrs can support
> both IPv4 and IPv6 as it is just a pointer.
>
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

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

> ---
>  net/core/flow_dissector.c |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 6288153d7f36..7c7b8739b8b8 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -220,13 +220,12 @@ ipv6:
>
>                 if (dissector_uses_key(flow_dissector,
>                                        FLOW_DISSECTOR_KEY_IPV6_ADDRS)) {
> -                       struct flow_dissector_key_ipv6_addrs *key_ipv6_addrs;
> -
> -                       key_ipv6_addrs = skb_flow_dissector_target(flow_dissector,
> -                                                                  FLOW_DISSECTOR_KEY_IPV6_ADDRS,
> -                                                                  target_container);
> +                       key_addrs = skb_flow_dissector_target(flow_dissector,
> +                                                             FLOW_DISSECTOR_KEY_IPV6_ADDRS,
> +                                                             target_container);
>
> -                       memcpy(key_ipv6_addrs, &iph->saddr, sizeof(*key_ipv6_addrs));
> +                       memcpy(&key_addrs->v6addrs, &iph->saddr,
> +                              sizeof(key_addrs->v6addrs));
>                         key_control->addr_type = FLOW_DISSECTOR_KEY_IPV6_ADDRS;
>                 }
>
>

Re: [net-next PATCH 5/5] eth: Pull header from first fragment via eth_get_headlen

[Tom Herbert]

On Wed, Feb 24, 2016 at 9:30 AM, Alexander Duyck <aduyck@mirantis.com> wrote:
> We want to try and pull the L4 header in if it is available in the first
> fragment.  As such add the flag to indicate we want to pull the headers on
> the first fragment in.
>
> Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

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

> ---
>  net/ethernet/eth.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
> index 103871784e50..66dff5e3d772 100644
> --- a/net/ethernet/eth.c
> +++ b/net/ethernet/eth.c
> @@ -125,6 +125,7 @@ EXPORT_SYMBOL(eth_header);
>   */
>  u32 eth_get_headlen(void *data, unsigned int len)
>  {
> +       const unsigned int flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG;
>         const struct ethhdr *eth = (const struct ethhdr *)data;
>         struct flow_keys keys;
>
> @@ -134,7 +135,7 @@ u32 eth_get_headlen(void *data, unsigned int len)
>
>         /* parse any remaining L2/L3 headers, check for L4 */
>         if (!skb_flow_dissect_flow_keys_buf(&keys, data, eth->h_proto,
> -                                           sizeof(*eth), len, 0))
> +                                           sizeof(*eth), len, flags))
>                 return max_t(u32, keys.control.thoff, sizeof(*eth));
>
>         /* parse for any L4 headers */
>