[PATCH/RFC net-next v2 0/4] net/sched: cls_flower: avoid false matching of truncated packets

[PATCH/RFC net-next v2 0/4] net/sched: cls_flower: avoid false matching of truncated packets

[Simon Horman]

Hi,

this series is intended to avoid false-positives which match
truncated packets against flower classifiers which match on:
* zero L4 ports or;
* zero ICMP code or type

This requires updating the flow dissector to return an error in such cases
and updating flower to not match on the result of a failed dissection.

In the case of UDP this results in a behavioural change to users of
flow_keys_dissector_keys[] and flow_keys_dissector_symmetric_keys[] -
dissection will fail on truncated packets where the IP protocol of the
packets indicates ports should be present (according to skb_flow_get_ports()).

The last patch of the series builds on the above to allow users to specify
a policy for how to handle packets whose dissection fails.

I will separately provide RFC patches to iproute2 to allow exercising the
last patch.

Changes between RFCv1 and RFCv2
* Rename new attribute in last path TCA_FLOWER_META_TRUNCATED
  after discussion with Jamal.
* Update changelog for "flow dissector" patches to make it clearer what
  the before and after behaviours are.

Simon Horman (4):
  flow dissector: return error on port dissection under-run
  flow dissector: return error on icmp dissection under-run
  net/sched: cls_flower: do not match if dissection fails
  net/sched: cls_flower: allow control of tree traversal on packet parse
    errors

 include/linux/skbuff.h       |  11 +++--
 include/uapi/linux/pkt_cls.h |   2 +
 net/core/flow_dissector.c    | 105 ++++++++++++++++++++++++-------------------
 net/sched/cls_flower.c       |  46 ++++++++++++++-----
 4 files changed, 106 insertions(+), 58 deletions(-)

-- 
2.1.4

[PATCH/RFC net-next v2 1/4] flow dissector: return error on port dissection under-run

[Simon Horman]

Return an error from __skb_flow_dissect() if insufficient packet data is
present when dissecting layer 4 ports.

Without this patch the absence of ports in truncated - e.g. UDP - packets
is treated the same way by the flow dissector as the presence of ports with
a value of zero. And without this patch the flower classifier is unable to
differentiate between these two cases which may lead to unexpected matching
of truncated packets.

With this patch the flow dissector and in turn the flower classifier can
differentiate between packets with zero L4 ports and truncated packets.

The approach taken here is to only return an error if the offset of ports
for the previously dissected IP protocol is known - a non error return from
proto_ports_offset() - but port data is not present in the packet - an
error return value from __skb_header_pointer().

The behaviour for callers of __skb_flow_get_ports() is changed but the only
callers are skb_flow_get_ports() and the flow dissector.  The former has
been updated so that its behaviour is unchanged.  Behavioural change of the
latter is the intended purpose of this patch but will only take effect with
a separate patch to have it refuse to match if dissection fails.

This change will lead to behavioural changes of the users of the dissector
with FLOW_DISSECTOR_KEY_PORTS - flower, and users of
flow_keys_dissector_keys[] and flow_keys_dissector_symmetric_keys[].  The
behavioural change for *_keys[] changes seem reasonable as the change will
should only be for truncated packets.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
---
rfc2
* Update changelog
---
 include/linux/skbuff.h    | 11 ++++++++---
 net/core/flow_dissector.c | 40 ++++++++++++++++++++++++++--------------
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a098d95b3d84..eb1b06dd0c8f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1108,13 +1108,18 @@ u32 __skb_get_hash_symmetric(const struct sk_buff *skb);
 u32 skb_get_poff(const struct sk_buff *skb);
 u32 __skb_get_poff(const struct sk_buff *skb, void *data,
 		   const struct flow_keys *keys, int hlen);
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen_proto);
+bool __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
+			  void *data, int hlen_proto, __be32 *ports);
 
 static inline __be32 skb_flow_get_ports(const struct sk_buff *skb,
 					int thoff, u8 ip_proto)
 {
-	return __skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0);
+	__be32 ports;
+
+	if (__skb_flow_get_ports(skb, thoff, ip_proto, NULL, 0, &ports))
+		return ports;
+	else
+		return 0;
 }
 
 void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 28d94bce4df8..b3bf4886f71f 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -86,30 +86,41 @@ static __be16 skb_flow_get_be16(const struct sk_buff *skb, int poff,
  * @ip_proto: protocol for which to get port offset
  * @data: raw buffer pointer to the packet, if NULL use skb->data
  * @hlen: packet header length, if @data is NULL use skb_headlen(skb)
+ * @ports: pointer to return ports in
  *
  * The function will try to retrieve the ports at offset thoff + poff where poff
- * is the protocol port offset returned from proto_ports_offset
+ * is the protocol port offset returned from proto_ports_offset.
+ *
+ * Returns false on error, true otherwise.
  */
-__be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
-			    void *data, int hlen)
+bool __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
+			  void *data, int hlen, __be32 *ports)
 {
 	int poff = proto_ports_offset(ip_proto);
+	__be32 *p, _p;
+
+	/* proto_ports_offset returning an error indicates that ip_proto is
+	 * not known to have ports. This is not considered an error here.
+	 * Rather it is considered that the flow key of the caller may use
+	 * the default value of port fields: 0.
+	 */
+	if (poff < 0) {
+		*ports = 0;
+		return true;
+	}
 
 	if (!data) {
 		data = skb->data;
 		hlen = skb_headlen(skb);
 	}
 
-	if (poff >= 0) {
-		__be32 *ports, _ports;
+	p = __skb_header_pointer(skb, thoff + poff, sizeof(_p),
+				 data, hlen, &_p);
+	if (!p)
+		return false;
+	*ports = *p;
 
-		ports = __skb_header_pointer(skb, thoff + poff,
-					     sizeof(_ports), data, hlen, &_ports);
-		if (ports)
-			return *ports;
-	}
-
-	return 0;
+	return true;
 }
 EXPORT_SYMBOL(__skb_flow_get_ports);
 
@@ -692,8 +703,9 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		key_ports = skb_flow_dissector_target(flow_dissector,
 						      FLOW_DISSECTOR_KEY_PORTS,
 						      target_container);
-		key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
-							data, hlen);
+		if (!__skb_flow_get_ports(skb, nhoff, ip_proto, data, hlen,
+					  &key_ports->ports))
+			goto out_bad;
 	}
 
 	if (dissector_uses_key(flow_dissector,
-- 
2.1.4

[PATCH/RFC net-next v2 2/4] flow dissector: return error on icmp dissection under-run

[Simon Horman]

Return an error from __skb_flow_dissect() if insufficient packet data is
present when dissecting icmp type and code.

Without this patch the absence of the ICMP type and code in truncated
ICMPv4 or IPVPv6 packets is treated the way same as the presence of a code
and type of value of zero.  And without this patch the flower classifier is
unable to differentiate between these two cases which may lead to
unexpected matching of truncated packets.

With this patch the flow dissector and in turn the flower classifier can
differentiate between packets with zero ICMP type and code, and truncated
packets.

The approach taken here is to return an error if the IP protocol indicates
ICMP but the type and code data is not present in the packet - an error
return value from __skb_header_pointer().

This should only effect the flower classifier as it is the only user of
W_DISSECTOR_KEY_ICMP.  The behavioural update for flower only takes effect
with a separate patch to have it refuse to match if dissection fails.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
---
rfc2
* Update changelog
---
 net/core/flow_dissector.c | 65 +++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index b3bf4886f71f..496afd7b3051 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -58,28 +58,6 @@ void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
 EXPORT_SYMBOL(skb_flow_dissector_init);
 
 /**
- * skb_flow_get_be16 - extract be16 entity
- * @skb: sk_buff to extract from
- * @poff: offset to extract at
- * @data: raw buffer pointer to the packet
- * @hlen: packet header length
- *
- * The function will try to retrieve a be32 entity at
- * offset poff
- */
-static __be16 skb_flow_get_be16(const struct sk_buff *skb, int poff,
-				void *data, int hlen)
-{
-	__be16 *u, _u;
-
-	u = __skb_header_pointer(skb, poff, sizeof(_u), data, hlen, &_u);
-	if (u)
-		return *u;
-
-	return 0;
-}
-
-/**
  * __skb_flow_get_ports - extract the upper layer ports and return them
  * @skb: sk_buff to extract the ports from
  * @thoff: transport header offset
@@ -353,6 +331,29 @@ __skb_flow_dissect_gre(const struct sk_buff *skb,
 	return FLOW_DISSECT_RET_OUT_PROTO_AGAIN;
 }
 
+static enum flow_dissect_ret
+__skb_flow_dissect_icmp(const struct sk_buff *skb,
+			struct flow_dissector *flow_dissector,
+			void *target_container, void *data, int nhoff, int hlen)
+{
+	struct flow_dissector_key_icmp *key_icmp;
+	__be16 *u, _u;
+
+	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_ICMP))
+		return FLOW_DISSECT_RET_OUT_GOOD;
+
+	u = __skb_header_pointer(skb, nhoff, sizeof(_u), data, hlen, &_u);
+	if (!u)
+		return FLOW_DISSECT_RET_OUT_BAD;
+
+	key_icmp = skb_flow_dissector_target(flow_dissector,
+					     FLOW_DISSECTOR_KEY_ICMP,
+					     target_container);
+	key_icmp->icmp = *u;
+
+	return FLOW_DISSECT_RET_OUT_GOOD;
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @skb: sk_buff to extract the flow from, can be NULL if the rest are specified
@@ -379,7 +380,6 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	struct flow_dissector_key_basic *key_basic;
 	struct flow_dissector_key_addrs *key_addrs;
 	struct flow_dissector_key_ports *key_ports;
-	struct flow_dissector_key_icmp *key_icmp;
 	struct flow_dissector_key_tags *key_tags;
 	struct flow_dissector_key_vlan *key_vlan;
 	bool skip_vlan = false;
@@ -694,6 +694,17 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	case IPPROTO_MPLS:
 		proto = htons(ETH_P_MPLS_UC);
 		goto mpls;
+	case IPPROTO_ICMP:
+	case NEXTHDR_ICMP:
+		switch (__skb_flow_dissect_icmp(skb, flow_dissector,
+						target_container, data,
+						nhoff, hlen)) {
+		case FLOW_DISSECT_RET_OUT_GOOD:
+			goto out_good;
+		case FLOW_DISSECT_RET_OUT_BAD:
+		default:
+			goto out_bad;
+		}
 	default:
 		break;
 	}
@@ -708,14 +719,6 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 			goto out_bad;
 	}
 
-	if (dissector_uses_key(flow_dissector,
-			       FLOW_DISSECTOR_KEY_ICMP)) {
-		key_icmp = skb_flow_dissector_target(flow_dissector,
-						     FLOW_DISSECTOR_KEY_ICMP,
-						     target_container);
-		key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data, hlen);
-	}
-
 out_good:
 	ret = true;
 
-- 
2.1.4

[PATCH/RFC net-next v2 3/4] net/sched: cls_flower: do not match if dissection fails

[Simon Horman]

If the flow skb_flow_dissect() returns an error it indicates that
dissection was incomplete for some reason. Matching using the result of an
incomplete dissection may cause unexpected results. For example:

* A match on zero layer 4 ports will also match packets truncated at
  the end of the IP header; that is packets where ports are missing are
  treated the same way as packets with zero ports.
* Likewise, a match on zero ICMP code or type will also match packets
  truncated at the end of the IP header; that is packets where the ICMP
  type and code are missing will be treated the same way as packets with
  zero ICMP code and type.

Separate patches to the flow dissector are required in order for it to
return errors in the above cases.

Fixes: 77b9900ef53a ("tc: introduce Flower classifier")
Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
---
 net/sched/cls_flower.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index ca526c0881bd..90bfd003176b 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -187,7 +187,8 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 	 * so do it rather here.
 	 */
 	skb_key.basic.n_proto = skb->protocol;
-	skb_flow_dissect(skb, &head->dissector, &skb_key, 0);
+	if (!skb_flow_dissect(skb, &head->dissector, &skb_key, 0))
+		return -1;
 
 	fl_set_masked_key(&skb_mkey, &skb_key, &head->mask);
 
-- 
2.1.4

[PATCH/RFC net-next v2 4/4] net/sched: cls_flower: allow control of tree traversal on packet parse errors

[Simon Horman]

Allow control how the tree of qdisc, classes and filters is further
traversed if an error is encountered when parsing the packet in order to
match the cls_flower filters at a particular prio.

By default continue to the next filter, the behaviour without this patch.

A use-case for this is to allow configuration of dropping of packets with
truncated headers.

For example, the following drops IPv4 packets that cannot be parsed by the
flow dissector up to the end of the UDP ports - e.g. because they are
truncated, and instantiates a continue action based on the port for packets
that can be parsed.

 # tc qdisc del dev eth0 ingress; tc qdisc add dev eth0 ingress
 # tc filter add dev eth0 protocol ip parent ffff: flower \
       indev eth0 ip_proto udp dst_port 80 truncated drop action continue

Signed-off-by: Simon Horman <simon.horman@netronome.com>
Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
---
rfc2
* s/TCA_FLOWER_HEADER_PARSE_ERR_ACT/TCA_FLOWER_META_TRUNCATED/
  after discussion with Jamal
* Clean up whitespace
---
 include/uapi/linux/pkt_cls.h |  2 ++
 net/sched/cls_flower.c       | 45 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index d613be3b3239..6474d6e55110 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -450,6 +450,8 @@ enum {
 	TCA_FLOWER_KEY_MPLS_TC,		/* u8 - 3 bits */
 	TCA_FLOWER_KEY_MPLS_LABEL,	/* be32 - 20 bits */
 
+	TCA_FLOWER_META_TRUNCATED,
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 90bfd003176b..000d0e3f44f9 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -67,13 +67,14 @@ struct cls_fl_head {
 	struct fl_flow_mask mask;
 	struct flow_dissector dissector;
 	u32 hgen;
-	bool mask_assigned;
+	bool assigned;
 	struct list_head filters;
 	struct rhashtable_params ht_params;
 	union {
 		struct work_struct work;
 		struct rcu_head	rcu;
 	};
+	int err_action;
 };
 
 struct cls_fl_filter {
@@ -188,7 +189,7 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 	 */
 	skb_key.basic.n_proto = skb->protocol;
 	if (!skb_flow_dissect(skb, &head->dissector, &skb_key, 0))
-		return -1;
+		return head->err_action;
 
 	fl_set_masked_key(&skb_mkey, &skb_key, &head->mask);
 
@@ -317,7 +318,7 @@ static void fl_destroy_sleepable(struct work_struct *work)
 {
 	struct cls_fl_head *head = container_of(work, struct cls_fl_head,
 						work);
-	if (head->mask_assigned)
+	if (head->assigned)
 		rhashtable_destroy(&head->ht);
 	kfree(head);
 	module_put(THIS_MODULE);
@@ -425,6 +426,7 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_MPLS_BOS]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_MPLS_TC]	= { .type = NLA_U8 },
 	[TCA_FLOWER_KEY_MPLS_LABEL]	= { .type = NLA_U32 },
+	[TCA_FLOWER_META_TRUNCATED]	= { .type = NLA_U32 },
 };
 
 static void fl_set_key_val(struct nlattr **tb,
@@ -791,13 +793,15 @@ static void fl_init_dissector(struct cls_fl_head *head,
 	skb_flow_dissector_init(&head->dissector, keys, cnt);
 }
 
-static int fl_check_assign_mask(struct cls_fl_head *head,
-				struct fl_flow_mask *mask)
+static int fl_check_assign_mask_and_err_action(struct cls_fl_head *head,
+					       struct fl_flow_mask *mask,
+					       int err_action)
 {
 	int err;
 
-	if (head->mask_assigned) {
-		if (!fl_mask_eq(&head->mask, mask))
+	if (head->assigned) {
+		if (!fl_mask_eq(&head->mask, mask) ||
+		    head->err_action != err_action)
 			return -EINVAL;
 		else
 			return 0;
@@ -810,7 +814,8 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
 	if (err)
 		return err;
 	memcpy(&head->mask, mask, sizeof(head->mask));
-	head->mask_assigned = true;
+	head->assigned = true;
+	head->err_action = err_action;
 
 	fl_init_dissector(head, mask);
 
@@ -883,7 +888,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 	struct cls_fl_filter *fnew;
 	struct nlattr **tb;
 	struct fl_flow_mask mask = {};
-	int err;
+	int err, err_action;
 
 	if (!tca[TCA_OPTIONS])
 		return -EINVAL;
@@ -930,11 +935,27 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 		}
 	}
 
+	if (tb[TCA_FLOWER_META_TRUNCATED]) {
+		err_action = nla_get_u32(tb[TCA_FLOWER_META_TRUNCATED]);
+
+		switch (err_action) {
+		case TC_ACT_UNSPEC:
+		case TC_ACT_OK:
+		case TC_ACT_SHOT:
+			break;
+		default:
+			err = -EINVAL;
+			goto errout;
+		}
+	} else {
+		err_action = TC_ACT_UNSPEC;
+	}
+
 	err = fl_set_parms(net, tp, fnew, &mask, base, tb, tca[TCA_RATE], ovr);
 	if (err)
 		goto errout;
 
-	err = fl_check_assign_mask(head, &mask);
+	err = fl_check_assign_mask_and_err_action(head, &mask, err_action);
 	if (err)
 		goto errout;
 
@@ -1321,6 +1342,10 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	if (f->flags && nla_put_u32(skb, TCA_FLOWER_FLAGS, f->flags))
 		goto nla_put_failure;
 
+	if (head->err_action != TC_ACT_UNSPEC &&
+	    nla_put_u32(skb, TCA_FLOWER_META_TRUNCATED, head->err_action))
+		goto nla_put_failure;
+
 	if (tcf_exts_dump(skb, &f->exts))
 		goto nla_put_failure;
 
-- 
2.1.4

Re: [PATCH/RFC net-next v2 4/4] net/sched: cls_flower: allow control of tree traversal on packet parse errors

[Cong Wang]

On Fri, May 5, 2017 at 5:47 AM, Simon Horman <simon.horman@netronome.com> wrote:
>
>  # tc qdisc del dev eth0 ingress; tc qdisc add dev eth0 ingress
>  # tc filter add dev eth0 protocol ip parent ffff: flower \
>        indev eth0 ip_proto udp dst_port 80 truncated drop action continue
>
[...]
> @@ -188,7 +189,7 @@ static int fl_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>          */
>         skb_key.basic.n_proto = skb->protocol;
>         if (!skb_flow_dissect(skb, &head->dissector, &skb_key, 0))
> -               return -1;
> +               return head->err_action;

This design looks odd, if you consider matching truncated packets as
a filter like other normal filters, then you should rely on the action
appended to return the action code, not within the filter itself.

Re: [PATCH/RFC net-next v2 1/4] flow dissector: return error on port dissection under-run

[Jamal Hadi Salim]

On 17-05-05 08:47 AM, Simon Horman wrote:
> Return an error from __skb_flow_dissect() if insufficient packet data is
> present when dissecting layer 4 ports.
>
> Without this patch the absence of ports in truncated - e.g. UDP - packets
> is treated the same way by the flow dissector as the presence of ports with
> a value of zero. And without this patch the flower classifier is unable to
> differentiate between these two cases which may lead to unexpected matching
> of truncated packets.
>
> With this patch the flow dissector and in turn the flower classifier can
> differentiate between packets with zero L4 ports and truncated packets.
>
> The approach taken here is to only return an error if the offset of ports
> for the previously dissected IP protocol is known - a non error return from
> proto_ports_offset() - but port data is not present in the packet - an
> error return value from __skb_header_pointer().
>
> The behaviour for callers of __skb_flow_get_ports() is changed but the only
> callers are skb_flow_get_ports() and the flow dissector.  The former has
> been updated so that its behaviour is unchanged.  Behavioural change of the
> latter is the intended purpose of this patch but will only take effect with
> a separate patch to have it refuse to match if dissection fails.
>
> This change will lead to behavioural changes of the users of the dissector
> with FLOW_DISSECTOR_KEY_PORTS - flower, and users of
> flow_keys_dissector_keys[] and flow_keys_dissector_symmetric_keys[].  The
> behavioural change for *_keys[] changes seem reasonable as the change will
> should only be for truncated packets.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH/RFC net-next v2 2/4] flow dissector: return error on icmp dissection under-run

[Jamal Hadi Salim]

On 17-05-05 08:47 AM, Simon Horman wrote:
> Return an error from __skb_flow_dissect() if insufficient packet data is
> present when dissecting icmp type and code.
>
> Without this patch the absence of the ICMP type and code in truncated
> ICMPv4 or IPVPv6 packets is treated the way same as the presence of a code
> and type of value of zero.  And without this patch the flower classifier is
> unable to differentiate between these two cases which may lead to
> unexpected matching of truncated packets.
>
> With this patch the flow dissector and in turn the flower classifier can
> differentiate between packets with zero ICMP type and code, and truncated
> packets.
>
> The approach taken here is to return an error if the IP protocol indicates
> ICMP but the type and code data is not present in the packet - an error
> return value from __skb_header_pointer().
>
> This should only effect the flower classifier as it is the only user of
> W_DISSECTOR_KEY_ICMP.  The behavioural update for flower only takes effect
> with a separate patch to have it refuse to match if dissection fails.
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>

Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH/RFC net-next v2 3/4] net/sched: cls_flower: do not match if dissection fails

[Jamal Hadi Salim]

On 17-05-05 08:47 AM, Simon Horman wrote:
> If the flow skb_flow_dissect() returns an error it indicates that
> dissection was incomplete for some reason. Matching using the result of an
> incomplete dissection may cause unexpected results. For example:
>
> * A match on zero layer 4 ports will also match packets truncated at
>   the end of the IP header; that is packets where ports are missing are
>   treated the same way as packets with zero ports.
> * Likewise, a match on zero ICMP code or type will also match packets
>   truncated at the end of the IP header; that is packets where the ICMP
>   type and code are missing will be treated the same way as packets with
>   zero ICMP code and type.
>
> Separate patches to the flow dissector are required in order for it to
> return errors in the above cases.
>
> Fixes: 77b9900ef53a ("tc: introduce Flower classifier")
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [PATCH/RFC net-next v2 4/4] net/sched: cls_flower: allow control of tree traversal on packet parse errors

[Jamal Hadi Salim]

On 17-05-05 08:47 AM, Simon Horman wrote:
> Allow control how the tree of qdisc, classes and filters is further
> traversed if an error is encountered when parsing the packet in order to
> match the cls_flower filters at a particular prio.
>
> By default continue to the next filter, the behaviour without this patch.
>
> A use-case for this is to allow configuration of dropping of packets with
> truncated headers.
>
> For example, the following drops IPv4 packets that cannot be parsed by the
> flow dissector up to the end of the UDP ports - e.g. because they are
> truncated, and instantiates a continue action based on the port for packets
> that can be parsed.
>
>  # tc qdisc del dev eth0 ingress; tc qdisc add dev eth0 ingress
>  # tc filter add dev eth0 protocol ip parent ffff: flower \
>        indev eth0 ip_proto udp dst_port 80 truncated drop action continue
>
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>

I agree with Cong on this. The default should be "didnt match" (which
is accomplished by returning -1). The user could enter an explicit
rule to override this behavior. i.e something like:

tc filter add dev eth0 protocol ip parent ffff: flower \
         indev eth0 ip_proto udp dst_port 80 truncated action continue

cheers,
jamal

Re: [PATCH/RFC net-next v2 4/4] net/sched: cls_flower: allow control of tree traversal on packet parse errors

[Simon Horman]

On Mon, May 08, 2017 at 07:32:02AM -0400, Jamal Hadi Salim wrote:
> On 17-05-05 08:47 AM, Simon Horman wrote:
> >Allow control how the tree of qdisc, classes and filters is further
> >traversed if an error is encountered when parsing the packet in order to
> >match the cls_flower filters at a particular prio.
> >
> >By default continue to the next filter, the behaviour without this patch.
> >
> >A use-case for this is to allow configuration of dropping of packets with
> >truncated headers.
> >
> >For example, the following drops IPv4 packets that cannot be parsed by the
> >flow dissector up to the end of the UDP ports - e.g. because they are
> >truncated, and instantiates a continue action based on the port for packets
> >that can be parsed.
> >
> > # tc qdisc del dev eth0 ingress; tc qdisc add dev eth0 ingress
> > # tc filter add dev eth0 protocol ip parent ffff: flower \
> >       indev eth0 ip_proto udp dst_port 80 truncated drop action continue
> >
> >Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >Reviewed-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
> 
> I agree with Cong on this. The default should be "didnt match" (which
> is accomplished by returning -1).

The default value for err_action is TC_ACT_UNSPEC (-1).
So I think we are in agreement there.

> The user could enter an explicit
> rule to override this behavior. i.e something like:
> 
> tc filter add dev eth0 protocol ip parent ffff: flower \
>         indev eth0 ip_proto udp dst_port 80 truncated action continue

This part I am struggling with but I will see what I can do
by making truncated part of the flow key .