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

[PATCH/RFC net-next 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.


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       |  47 ++++++++++++++-----
 4 files changed, 107 insertions(+), 58 deletions(-)

-- 
2.12.2.816.g2cccc81164

[PATCH/RFC net-next 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 change the absence of ports in truncated - e.g. UDP - packets
is treated the same way as the presence of ports with a value of zero.  As
a result the flower classifier is unable to differentiate between these two
cases which may lead to unexpected matching of 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() - and the 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>
---
 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 81ef53f06534..0ad9b3955829 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.12.2.816.g2cccc81164

[PATCH/RFC net-next 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 change the absence of the ICMP type and code in truncated
ICMPv4 or IPVPv6 packets is treated the same as the presence of a code and
type of value of zero.  As a result the flower classifier is unable to
differentiate between these two cases which may lead to unexpected matching
of truncated packets.

The approach taken here is to return an error if the IP protocol indicates
ICMP, and 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>
---
 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.12.2.816.g2cccc81164

[PATCH/RFC net-next 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>
---
 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 3ecf07666df3..cc6b3e7cf03b 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.12.2.816.g2cccc81164

[PATCH/RFC net-next 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 header_parse_err_action drop \
       action continue

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 include/uapi/linux/pkt_cls.h |  2 ++
 net/sched/cls_flower.c       | 46 ++++++++++++++++++++++++++++++++++----------
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index f1129e383b2a..b722a85bcfa1 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -437,6 +437,8 @@ enum {
 	TCA_FLOWER_KEY_MPLS_TC,		/* u8 - 3 bits */
 	TCA_FLOWER_KEY_MPLS_LABEL,	/* be32 - 20 bits */
 
+	TCA_FLOWER_HEADER_PARSE_ERR_ACT,
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index cc6b3e7cf03b..4d2d91b0d532 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_HEADER_PARSE_ERR_ACT] = { .type = NLA_U32 },
 };
 
 static void fl_set_key_val(struct nlattr **tb,
@@ -779,13 +781,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;
@@ -798,7 +802,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);
 
@@ -871,7 +876,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;
@@ -918,11 +923,28 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 		}
 	}
 
+	if (tb[TCA_FLOWER_HEADER_PARSE_ERR_ACT]) {
+		err_action = nla_get_u32(tb[TCA_FLOWER_HEADER_PARSE_ERR_ACT]);
+
+		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;
 
@@ -1309,6 +1331,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_HEADER_PARSE_ERR_ACT, head->err_action))
+		goto nla_put_failure;
+
 	if (tcf_exts_dump(skb, &f->exts))
 		goto nla_put_failure;
 
-- 
2.12.2.816.g2cccc81164

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

[Jamal Hadi Salim]

On 17-04-28 08:00 AM, Simon Horman wrote:
> 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()).

I think i understand the use case/need.
But would it be fair to say that the truncated vs non-truncated are two
different filter rules? Example what would offloading of 
header_parse_err_action mean?

cheers,
jamal

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

[Simon Horman]

On Fri, Apr 28, 2017 at 08:52:56AM -0400, Jamal Hadi Salim wrote:
> On 17-04-28 08:00 AM, Simon Horman wrote:
> >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()).
> 
> I think i understand the use case/need.
> But would it be fair to say that the truncated vs non-truncated are two
> different filter rules?

How would you describe such a rule? The case that is being dealt with is
one where there is a parse error and thus nothing to match on from a flower
pov.

> Example what would offloading of
> header_parse_err_action mean?

Why would it need to differ semantically to the implementation in this
patch? I feel that I am missing something.

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

[Jamal Hadi Salim]

On 17-04-28 09:11 AM, Simon Horman wrote:
> On Fri, Apr 28, 2017 at 08:52:56AM -0400, Jamal Hadi Salim wrote:
>> On 17-04-28 08:00 AM, Simon Horman wrote:
>>> Hi,
>>>
>>> this series is intended to avoid false-positives which match
>>> truncated packets against flower classifiers which match on:
>>> * zero L4 ports or;
>
> How would you describe such a rule? The case that is being dealt with is
> one where there is a parse error and thus nothing to match on from a flower
> pov.
>

A default lower prio match all on udp or icmp?

>> Example what would offloading of
>> header_parse_err_action mean?
>
> Why would it need to differ semantically to the implementation in this
> patch? I feel that I am missing something.
>

Unless I misunderstood:
Isnt the issue the dissector that confused something missing L4 ports
and said "port is zero"?

Unless the hardware has the same "bug" as the dissector seems like would
be a different semantic in the h/ware.

cheers,
jamal

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

[Simon Horman]

On Fri, Apr 28, 2017 at 09:41:00AM -0400, Jamal Hadi Salim wrote:
> On 17-04-28 09:11 AM, Simon Horman wrote:
> >On Fri, Apr 28, 2017 at 08:52:56AM -0400, Jamal Hadi Salim wrote:
> >>On 17-04-28 08:00 AM, Simon Horman wrote:
> >>>Hi,
> >>>
> >>>this series is intended to avoid false-positives which match
> >>>truncated packets against flower classifiers which match on:
> >>>* zero L4 ports or;
> >
> >How would you describe such a rule? The case that is being dealt with is
> >one where there is a parse error and thus nothing to match on from a flower
> >pov.
> >
> 
> A default lower prio match all on udp or icmp?

I'm certainly not opposed to exploring ideas here.

The way that flower currently works is that a match on ip_proto ==
UDP/TCP/SCTP/ICMP but not fields in the L4 header itself would not result in
the dissector only dissecting the packet's L4 header and thus would not
discover (or as in currently the case, silently ignore) the absence of the
ports/ICMP type and code in the L4 header.

What my patch attempts to do is to describe a policy of what to do if
a given classifier invokes the dissector (to pull out the headers needed for
the match in question) and that dissection fails. Its basically describing
the error-path.

> >>Example what would offloading of
> >>header_parse_err_action mean?
> >
> >Why would it need to differ semantically to the implementation in this
> >patch? I feel that I am missing something.
> >
> 
> Unless I misunderstood:
> Isnt the issue the dissector that confused something missing L4 ports
> and said "port is zero"?
> 
> Unless the hardware has the same "bug" as the dissector seems like would
> be a different semantic in the h/ware.

There are two issues:

1. As things stand, without this patch-set, flower does not differentiate
   between a packet truncated at the end of the IP header and a packet with
   zero ports. Likewise for icmp type and code of zero.

   The first three patches of this series address that so that a match for
   port == zero only matches if ports are present in the packet. Again,
   likewise for ICMP.

   This is a bug-fix to my way of thinking.

2. The behaviour described above, prior to this patchset, might have been
   utilised to f.e. drop packets that are either truncated or have port == 0
   (because flower didn't differentiate between these cases).

   So the question becomes if/how to provide such a feature.
   The last patch is my attempt to answer that question.

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

[Jamal Hadi Salim]

On 17-04-28 10:14 AM, Simon Horman wrote:
> On Fri, Apr 28, 2017 at 09:41:00AM -0400, Jamal Hadi Salim wrote:
>> On 17-04-28 09:11 AM, Simon Horman wrote:
[..]
>> A default lower prio match all on udp or icmp?
>
> I'm certainly not opposed to exploring ideas here.
>
> The way that flower currently works is that a match on ip_proto ==
> UDP/TCP/SCTP/ICMP but not fields in the L4 header itself would not result in
> the dissector only dissecting the packet's L4 header and thus would not
> discover (or as in currently the case, silently ignore) the absence of the
> ports/ICMP type and code in the L4 header.
>
> What my patch attempts to do is to describe a policy of what to do if
> a given classifier invokes the dissector (to pull out the headers needed for
> the match in question) and that dissection fails. Its basically describing
> the error-path.
>

Understood - I was struggling with whether error-path is the same as
"didnt match".

>
> There are two issues:
>
> 1. As things stand, without this patch-set, flower does not differentiate
>    between a packet truncated at the end of the IP header and a packet with
>    zero ports. Likewise for icmp type and code of zero.
>
>    The first three patches of this series address that so that a match for
>    port == zero only matches if ports are present in the packet. Again,
>    likewise for ICMP.
>
>    This is a bug-fix to my way of thinking.
>

Agreed to bug fix. I would have said there is never a legit packet with
TCP/UDP but I think some fingerprinting apps use it. And one would need
to distinguish between the two at classification time.
ICMP type 0 is certainly used.

minimal some flag should qualify it as "truncated".

> 2. The behaviour described above, prior to this patchset, might have been
>    utilised to f.e. drop packets that are either truncated or have port == 0
>    (because flower didn't differentiate between these cases).
>
>    So the question becomes if/how to provide such a feature.
>    The last patch is my attempt to answer that question.

It almost feels like you need metadata matching as well - one being
"truncated".

cheers,
jamal

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

[Jamal Hadi Salim]

On 17-04-30 09:51 AM, Jamal Hadi Salim wrote:

[..]

>> 1. As things stand, without this patch-set, flower does not differentiate
>>    between a packet truncated at the end of the IP header and a packet
>> with
>>    zero ports. Likewise for icmp type and code of zero.
>>
>>    The first three patches of this series address that so that a match
>> for
>>    port == zero only matches if ports are present in the packet. Again,
>>    likewise for ICMP.
>>
>>    This is a bug-fix to my way of thinking.
>>
>
> Agreed to bug fix. I would have said there is never a legit packet with
> TCP/UDP

Meant:
"never a legit packet with TCP/UDP port 0 on the wire".

cheers,
jamal

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

[Simon Horman]

On Sun, Apr 30, 2017 at 09:51:30AM -0400, Jamal Hadi Salim wrote:
> On 17-04-28 10:14 AM, Simon Horman wrote:
> >On Fri, Apr 28, 2017 at 09:41:00AM -0400, Jamal Hadi Salim wrote:
> >>On 17-04-28 09:11 AM, Simon Horman wrote:
> [..]
> >>A default lower prio match all on udp or icmp?
> >
> >I'm certainly not opposed to exploring ideas here.
> >
> >The way that flower currently works is that a match on ip_proto ==
> >UDP/TCP/SCTP/ICMP but not fields in the L4 header itself would not result in
> >the dissector only dissecting the packet's L4 header and thus would not
> >discover (or as in currently the case, silently ignore) the absence of the
> >ports/ICMP type and code in the L4 header.
> >
> >What my patch attempts to do is to describe a policy of what to do if
> >a given classifier invokes the dissector (to pull out the headers needed for
> >the match in question) and that dissection fails. Its basically describing
> >the error-path.
> >
> 
> Understood - I was struggling with whether error-path is the same as
> "didnt match".
> 
> >
> >There are two issues:
> >
> >1. As things stand, without this patch-set, flower does not differentiate
> >   between a packet truncated at the end of the IP header and a packet with
> >   zero ports. Likewise for icmp type and code of zero.
> >
> >   The first three patches of this series address that so that a match for
> >   port == zero only matches if ports are present in the packet. Again,
> >   likewise for ICMP.
> >
> >   This is a bug-fix to my way of thinking.
> 
> Agreed to bug fix. I would have said there is never a legit packet with
> TCP/UDP but I think some fingerprinting apps use it. And one would need
> to distinguish between the two at classification time.

Yes, that is basically what I thought too.

> ICMP type 0 is certainly used.

Agreed.

> minimal some flag should qualify it as "truncated".

Would changing TCA_FLOWER_HEADER_PARSE_ERR_ACT to
TCA_FLOWER_META_TRUNCATED help?

> >2. The behaviour described above, prior to this patchset, might have been
> >   utilised to f.e. drop packets that are either truncated or have port == 0
> >   (because flower didn't differentiate between these cases).
> >
> >   So the question becomes if/how to provide such a feature.
> >   The last patch is my attempt to answer that question.
> 
> It almost feels like you need metadata matching as well - one being
> "truncated".

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

[Jamal Hadi Salim]

On 17-05-01 06:36 AM, Simon Horman wrote:
> On Sun, Apr 30, 2017 at 09:51:30AM -0400, Jamal Hadi Salim wrote:
>> On 17-04-28 10:14 AM, Simon Horman wrote:

[..]

>> minimal some flag should qualify it as "truncated".
>
> Would changing TCA_FLOWER_HEADER_PARSE_ERR_ACT to
> TCA_FLOWER_META_TRUNCATED help?
>

I think so - as long as you are able to recognize the truncated
vs real 0 port/type etc.

cheers,
jamal

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

[Simon Horman]

On Mon, May 01, 2017 at 09:35:47PM -0400, Jamal Hadi Salim wrote:
> On 17-05-01 06:36 AM, Simon Horman wrote:
> >On Sun, Apr 30, 2017 at 09:51:30AM -0400, Jamal Hadi Salim wrote:
> >>On 17-04-28 10:14 AM, Simon Horman wrote:
> 
> [..]
> 
> >>minimal some flag should qualify it as "truncated".
> >
> >Would changing TCA_FLOWER_HEADER_PARSE_ERR_ACT to
> >TCA_FLOWER_META_TRUNCATED help?
> >
> 
> I think so - as long as you are able to recognize the truncated
> vs real 0 port/type etc.

That is the intention of the patchset.
I will make the change and repost.