[PATCH net-next 0/2] flower: add MPLS matching support

[PATCH net-next 0/2] flower: add MPLS matching support

[Benjamin LaHaise]

From: Benjamin LaHaise <bcrl@kvack.org>

This patch series adds support for parsing MPLS flows in the flow dissector
and the flower classifier.  Each of the MPLS TTL, BOS, TC and Label fields
can be used for matching.

v2: incorporate style feedback, move #defines to linux/include/mpls.h
Note: this omits Jiri's request to remove tabs between the type and 
field names in struct declarations.  This would be inconsistent with 
numerous other struct definitions.

Benjamin LaHaise (2):
  flow_dissector: add mpls support (v2)
  cls_flower: add support for matching MPLS fields (v2)

 include/linux/mpls.h         |  5 +++
 include/net/flow_dissector.h |  8 +++++
 include/uapi/linux/pkt_cls.h |  5 +++
 net/core/flow_dissector.c    | 25 +++++++++++++--
 net/sched/cls_flower.c       | 74 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 114 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH net-next 1/2] flow_dissector: add mpls support (v2)

[Benjamin LaHaise]

Add support for parsing MPLS flows to the flow dissector in preparation for
adding MPLS match support to cls_flower.

Signed-off-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Simon Horman <simon.horman@netronome.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Eric Dumazet <jhs@mojatatu.com>
Cc: Hadar Hen Zion <hadarh@mellanox.com>
Cc: Gao Feng <fgao@ikuai8.com>
---
 include/linux/mpls.h         |  5 +++++
 include/net/flow_dissector.h |  8 ++++++++
 net/core/flow_dissector.c    | 25 ++++++++++++++++++++++---
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/include/linux/mpls.h b/include/linux/mpls.h
index 9999145..384fb22 100644
--- a/include/linux/mpls.h
+++ b/include/linux/mpls.h
@@ -3,4 +3,9 @@
 
 #include <uapi/linux/mpls.h>
 
+#define MPLS_TTL_MASK		(MPLS_LS_TTL_MASK >> MPLS_LS_TTL_SHIFT)
+#define MPLS_BOS_MASK		(MPLS_LS_S_MASK >> MPLS_LS_S_SHIFT)
+#define MPLS_TC_MASK		(MPLS_LS_TC_MASK >> MPLS_LS_TC_SHIFT)
+#define MPLS_LABEL_MASK		(MPLS_LS_LABEL_MASK >> MPLS_LS_LABEL_SHIFT)
+
 #endif  /* _LINUX_MPLS_H */
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index ac97030..8d21d44 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -41,6 +41,13 @@ struct flow_dissector_key_vlan {
 	u16	padding;
 };
 
+struct flow_dissector_key_mpls {
+	u32	mpls_ttl:8,
+		mpls_bos:1,
+		mpls_tc:3,
+		mpls_label:20;
+};
+
 struct flow_dissector_key_keyid {
 	__be32	keyid;
 };
@@ -169,6 +176,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_ENC_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
 	FLOW_DISSECTOR_KEY_ENC_CONTROL, /* struct flow_dissector_key_control */
 	FLOW_DISSECTOR_KEY_ENC_PORTS, /* struct flow_dissector_key_ports */
+	FLOW_DISSECTOR_KEY_MPLS, /* struct flow_dissector_key_mpls */
 
 	FLOW_DISSECTOR_KEY_MAX,
 };
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index c9cf425..28d94bc 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -126,9 +126,11 @@ __skb_flow_dissect_mpls(const struct sk_buff *skb,
 {
 	struct flow_dissector_key_keyid *key_keyid;
 	struct mpls_label *hdr, _hdr[2];
+	u32 entry, label;
 
 	if (!dissector_uses_key(flow_dissector,
-				FLOW_DISSECTOR_KEY_MPLS_ENTROPY))
+				FLOW_DISSECTOR_KEY_MPLS_ENTROPY) &&
+	    !dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_MPLS))
 		return FLOW_DISSECT_RET_OUT_GOOD;
 
 	hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data,
@@ -136,8 +138,25 @@ __skb_flow_dissect_mpls(const struct sk_buff *skb,
 	if (!hdr)
 		return FLOW_DISSECT_RET_OUT_BAD;
 
-	if ((ntohl(hdr[0].entry) & MPLS_LS_LABEL_MASK) >>
-	    MPLS_LS_LABEL_SHIFT == MPLS_LABEL_ENTROPY) {
+	entry = ntohl(hdr[0].entry);
+	label = (entry & MPLS_LS_LABEL_MASK) >> MPLS_LS_LABEL_SHIFT;
+
+	if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_MPLS)) {
+		struct flow_dissector_key_mpls *key_mpls;
+
+		key_mpls = skb_flow_dissector_target(flow_dissector,
+						     FLOW_DISSECTOR_KEY_MPLS,
+						     target_container);
+		key_mpls->mpls_label = label;
+		key_mpls->mpls_ttl = (entry & MPLS_LS_TTL_MASK)
+					>> MPLS_LS_TTL_SHIFT;
+		key_mpls->mpls_tc = (entry & MPLS_LS_TC_MASK)
+					>> MPLS_LS_TC_SHIFT;
+		key_mpls->mpls_bos = (entry & MPLS_LS_S_MASK)
+					>> MPLS_LS_S_SHIFT;
+	}
+
+	if (label == MPLS_LABEL_ENTROPY) {
 		key_keyid = skb_flow_dissector_target(flow_dissector,
 						      FLOW_DISSECTOR_KEY_MPLS_ENTROPY,
 						      target_container);
-- 
2.7.4

Re: [PATCH net-next 1/2] flow_dissector: add mpls support (v2)

[Jiri Pirko]

Sat, Apr 22, 2017 at 10:52:46PM CEST, benjamin.lahaise@netronome.com wrote:
>Add support for parsing MPLS flows to the flow dissector in preparation for
>adding MPLS match support to cls_flower.
>
>Signed-off-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
>Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>

This looks odd :)


>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

[PATCH net-next 2/2] cls_flower: add support for matching MPLS fields (v2)

[Benjamin LaHaise]

Add support to the tc flower classifier to match based on fields in MPLS
labels (TTL, Bottom of Stack, TC field, Label).

Signed-off-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Simon Horman <simon.horman@netronome.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jiri Pirko <jiri@mellanox.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Hadar Hen Zion <hadarh@mellanox.com>
Cc: Gao Feng <fgao@ikuai8.com>
---
 include/uapi/linux/pkt_cls.h |  5 +++
 net/sched/cls_flower.c       | 74 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 7a69f2a..f1129e3 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -432,6 +432,11 @@ enum {
 	TCA_FLOWER_KEY_ARP_THA,		/* ETH_ALEN */
 	TCA_FLOWER_KEY_ARP_THA_MASK,	/* ETH_ALEN */
 
+	TCA_FLOWER_KEY_MPLS_TTL,	/* u8 - 8 bits */
+	TCA_FLOWER_KEY_MPLS_BOS,	/* u8 - 1 bit */
+	TCA_FLOWER_KEY_MPLS_TC,		/* u8 - 3 bits */
+	TCA_FLOWER_KEY_MPLS_LABEL,	/* be32 - 20 bits */
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 31ee340..3ecf076 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -18,6 +18,7 @@
 #include <linux/if_ether.h>
 #include <linux/in6.h>
 #include <linux/ip.h>
+#include <linux/mpls.h>
 
 #include <net/sch_generic.h>
 #include <net/pkt_cls.h>
@@ -47,6 +48,7 @@ struct fl_flow_key {
 		struct flow_dissector_key_ipv6_addrs enc_ipv6;
 	};
 	struct flow_dissector_key_ports enc_tp;
+	struct flow_dissector_key_mpls mpls;
 } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
 
 struct fl_flow_mask_range {
@@ -418,6 +420,10 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_ARP_SHA_MASK]	= { .len = ETH_ALEN },
 	[TCA_FLOWER_KEY_ARP_THA]	= { .len = ETH_ALEN },
 	[TCA_FLOWER_KEY_ARP_THA_MASK]	= { .len = ETH_ALEN },
+	[TCA_FLOWER_KEY_MPLS_TTL]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_MPLS_BOS]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_MPLS_TC]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_MPLS_LABEL]	= { .type = NLA_U32 },
 };
 
 static void fl_set_key_val(struct nlattr **tb,
@@ -433,6 +439,31 @@ static void fl_set_key_val(struct nlattr **tb,
 		memcpy(mask, nla_data(tb[mask_type]), len);
 }
 
+static void fl_set_key_mpls(struct nlattr **tb,
+			    struct flow_dissector_key_mpls *key_val,
+			    struct flow_dissector_key_mpls *key_mask)
+{
+	if (tb[TCA_FLOWER_KEY_MPLS_TTL]) {
+		key_val->mpls_ttl = nla_get_u8(tb[TCA_FLOWER_KEY_MPLS_TTL]);
+		key_mask->mpls_ttl = MPLS_TTL_MASK;
+	}
+	if (tb[TCA_FLOWER_KEY_MPLS_BOS]) {
+		key_val->mpls_bos = nla_get_u8(tb[TCA_FLOWER_KEY_MPLS_BOS]);
+		key_mask->mpls_bos = MPLS_BOS_MASK;
+	}
+	if (tb[TCA_FLOWER_KEY_MPLS_TC]) {
+		key_val->mpls_tc =
+			nla_get_u8(tb[TCA_FLOWER_KEY_MPLS_TC]) & MPLS_TC_MASK;
+		key_mask->mpls_tc = MPLS_TC_MASK;
+	}
+	if (tb[TCA_FLOWER_KEY_MPLS_LABEL]) {
+		key_val->mpls_label =
+			nla_get_u32(tb[TCA_FLOWER_KEY_MPLS_LABEL]) &
+			MPLS_LABEL_MASK;
+		key_mask->mpls_label = MPLS_LABEL_MASK;
+	}
+}
+
 static void fl_set_key_vlan(struct nlattr **tb,
 			    struct flow_dissector_key_vlan *key_val,
 			    struct flow_dissector_key_vlan *key_mask)
@@ -589,6 +620,9 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 			       &mask->icmp.code,
 			       TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
 			       sizeof(key->icmp.code));
+	} else if (key->basic.n_proto == htons(ETH_P_MPLS_UC) ||
+		   key->basic.n_proto == htons(ETH_P_MPLS_MC)) {
+		fl_set_key_mpls(tb, &key->mpls, &mask->mpls);
 	} else if (key->basic.n_proto == htons(ETH_P_ARP) ||
 		   key->basic.n_proto == htons(ETH_P_RARP)) {
 		fl_set_key_val(tb, &key->arp.sip, TCA_FLOWER_KEY_ARP_SIP,
@@ -725,6 +759,8 @@ static void fl_init_dissector(struct cls_fl_head *head,
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_ARP, arp);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
+			     FLOW_DISSECTOR_KEY_MPLS, mpls);
+	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_VLAN, vlan);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_ENC_KEYID, enc_key_id);
@@ -991,6 +1027,41 @@ static int fl_dump_key_val(struct sk_buff *skb,
 	return 0;
 }
 
+static int fl_dump_key_mpls(struct sk_buff *skb,
+			    struct flow_dissector_key_mpls *mpls_key,
+			    struct flow_dissector_key_mpls *mpls_mask)
+{
+	int err;
+
+	if (!memchr_inv(mpls_mask, 0, sizeof(*mpls_mask)))
+		return 0;
+	if (mpls_mask->mpls_ttl) {
+		err = nla_put_u8(skb, TCA_FLOWER_KEY_MPLS_TTL,
+				 mpls_key->mpls_ttl);
+		if (err)
+			return err;
+	}
+	if (mpls_mask->mpls_tc) {
+		err = nla_put_u8(skb, TCA_FLOWER_KEY_MPLS_TC,
+				 mpls_key->mpls_tc);
+		if (err)
+			return err;
+	}
+	if (mpls_mask->mpls_label) {
+		err = nla_put_u32(skb, TCA_FLOWER_KEY_MPLS_LABEL,
+				  mpls_key->mpls_label);
+		if (err)
+			return err;
+	}
+	if (mpls_mask->mpls_bos) {
+		err = nla_put_u8(skb, TCA_FLOWER_KEY_MPLS_BOS,
+				 mpls_key->mpls_bos);
+		if (err)
+			return err;
+	}
+	return 0;
+}
+
 static int fl_dump_key_vlan(struct sk_buff *skb,
 			    struct flow_dissector_key_vlan *vlan_key,
 			    struct flow_dissector_key_vlan *vlan_mask)
@@ -1096,6 +1167,9 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 			    sizeof(key->basic.n_proto)))
 		goto nla_put_failure;
 
+	if (fl_dump_key_mpls(skb, &key->mpls, &mask->mpls))
+		goto nla_put_failure;
+
 	if (fl_dump_key_vlan(skb, &key->vlan, &mask->vlan))
 		goto nla_put_failure;
 
-- 
2.7.4

Re: [PATCH net-next 2/2] cls_flower: add support for matching MPLS fields (v2)

[Jiri Pirko]

Sat, Apr 22, 2017 at 10:52:47PM CEST, benjamin.lahaise@netronome.com wrote:
>Add support to the tc flower classifier to match based on fields in MPLS
>labels (TTL, Bottom of Stack, TC field, Label).
>
>Signed-off-by: Benjamin LaHaise <benjamin.lahaise@netronome.com>
>Signed-off-by: Benjamin LaHaise <bcrl@kvack.org>
>Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[David Miller]

From: Benjamin LaHaise <benjamin.lahaise@netronome.com>
Date: Sat, 22 Apr 2017 16:52:45 -0400

> From: Benjamin LaHaise <bcrl@kvack.org>
> 
> This patch series adds support for parsing MPLS flows in the flow dissector
> and the flower classifier.  Each of the MPLS TTL, BOS, TC and Label fields
> can be used for matching.
> 
> v2: incorporate style feedback, move #defines to linux/include/mpls.h
> Note: this omits Jiri's request to remove tabs between the type and 
> field names in struct declarations.  This would be inconsistent with 
> numerous other struct definitions.

Series applied, but in the future:

1) Put the "v2", "v3", whatever in the inital [PATCH xxx] bracketed
   part of the subject line, otherwise it ends up in the GIT commit
   message header line and that's not desired.

2) Please cut it with these double signoffs, and add an appropriate
   entry to the email aliases file.

Thanks.

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jamal Hadi Salim]

On 17-04-24 02:32 PM, David Miller wrote:
> From: Benjamin LaHaise <benjamin.lahaise@netronome.com>

>
> Series applied, but in the future:
>
> 1) Put the "v2", "v3", whatever in the inital [PATCH xxx] bracketed
>    part of the subject line, otherwise it ends up in the GIT commit
>    message header line and that's not desired.
>
> 2) Please cut it with these double signoffs, and add an appropriate
>    entry to the email aliases file.

I know i should have spoken earlier, wanted to but got distracted - but
shouldnt the new rules have applied to this patch too? ;->

You have 3 TLVs, one of which is u8 that only allows use of 3 bits.
The other is a u32 which allows only 20 bits to be set.

cheers,
jamal

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Benjamin LaHaise]

On Mon, Apr 24, 2017 at 08:58:18PM -0400, Jamal Hadi Salim wrote:
> On 17-04-24 02:32 PM, David Miller wrote:
> > From: Benjamin LaHaise <benjamin.lahaise@netronome.com>
> 
> > 
> > Series applied, but in the future:
> > 
> > 1) Put the "v2", "v3", whatever in the inital [PATCH xxx] bracketed
> >    part of the subject line, otherwise it ends up in the GIT commit
> >    message header line and that's not desired.
> > 
> > 2) Please cut it with these double signoffs, and add an appropriate
> >    entry to the email aliases file.
> 
> I know i should have spoken earlier, wanted to but got distracted - but
> shouldnt the new rules have applied to this patch too? ;->
> 
> You have 3 TLVs, one of which is u8 that only allows use of 3 bits.
> The other is a u32 which allows only 20 bits to be set.

What are the new rules for TLVs -- do you want a new type added for the 
subset values that are limited to the number of bits actually used?  A 
pointed here would be helpful.

		-ben

> cheers,
> jamal

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jamal Hadi Salim]

On 17-04-24 09:05 PM, Benjamin LaHaise wrote:
> On Mon, Apr 24, 2017 at 08:58:18PM -0400, Jamal Hadi Salim wrote:
>> On 17-04-24 02:32 PM, David Miller wrote:

>> You have 3 TLVs, one of which is u8 that only allows use of 3 bits.
>> The other is a u32 which allows only 20 bits to be set.
>
> What are the new rules for TLVs -- do you want a new type added for the
> subset values that are limited to the number of bits actually used?  A
> pointed here would be helpful.
>

The rules are to check for bits that are not used.
For example in the BOS TLV it would be required to
verify that the user did not set any other bit other than the
bos field. It is required to reject the whole thing if the user
set any other of the u8 bits.

cheers,
jamal

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jakub Kicinski]

On Mon, 24 Apr 2017 20:58:18 -0400, Jamal Hadi Salim wrote:
> On 17-04-24 02:32 PM, David Miller wrote:
> > From: Benjamin LaHaise <benjamin.lahaise@netronome.com>  
> 
> >
> > Series applied, but in the future:
> >
> > 1) Put the "v2", "v3", whatever in the inital [PATCH xxx] bracketed
> >    part of the subject line, otherwise it ends up in the GIT commit
> >    message header line and that's not desired.
> >
> > 2) Please cut it with these double signoffs, and add an appropriate
> >    entry to the email aliases file.  
> 
> I know i should have spoken earlier, wanted to but got distracted - but
> shouldnt the new rules have applied to this patch too? ;->
> 
> You have 3 TLVs, one of which is u8 that only allows use of 3 bits.
> The other is a u32 which allows only 20 bits to be set.

I don't think we will ever reuse bits in a field which is called
MPLS_LABEL for anything else than an MPLS label.  My understanding of
the conclusions from recent discussions was to either (a) make
attributes single purpose (i.e. separate u8 per flag); or (b) make
attributes u32/word size but validate the unused bits are zero.  This
patch would fall under (a).

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jamal Hadi Salim]

On 17-04-24 09:20 PM, Jakub Kicinski wrote:
> On Mon, 24 Apr 2017 20:58:18 -0400, Jamal Hadi Salim wrote:
>> On 17-04-24 02:32 PM, David Miller wrote:

>> You have 3 TLVs, one of which is u8 that only allows use of 3 bits.
>> The other is a u32 which allows only 20 bits to be set.
>
> I don't think we will ever reuse bits in a field which is called
> MPLS_LABEL for anything else than an MPLS label.

That is true. So maybe bad example. It also helps the mpls disector
wont for example allow copying of more than 20 bits for a label.

cheers,
jamal

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jamal Hadi Salim]

On 17-04-24 09:48 PM, Jamal Hadi Salim wrote:
> On 17-04-24 09:20 PM, Jakub Kicinski wrote:
>> On Mon, 24 Apr 2017 20:58:18 -0400, Jamal Hadi Salim wrote:
>>> On 17-04-24 02:32 PM, David Miller wrote:
>
>>> You have 3 TLVs, one of which is u8 that only allows use of 3 bits.
>>> The other is a u32 which allows only 20 bits to be set.
>>
>> I don't think we will ever reuse bits in a field which is called
>> MPLS_LABEL for anything else than an MPLS label.
>
> That is true. So maybe bad example. It also helps the mpls disector
> wont for example allow copying of more than 20 bits for a label.

Hrm. maybe I am wrong.
Lets say user sets all of the 8 bits in BOS,
what does setting
key_val->mpls_bos = nla_get_u8 do?

Same with the 20 bits for the label in the u32
or 3 bit bits in the u8 tc.

cheers,
jamal

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jamal Hadi Salim]

On 17-04-24 10:00 PM, Jamal Hadi Salim wrote:
> On 17-04-24 09:48 PM, Jamal Hadi Salim wrote:

>
> Hrm. maybe I am wrong.
> Lets say user sets all of the 8 bits in BOS,
> what does setting
> key_val->mpls_bos = nla_get_u8 do?
>
> Same with the 20 bits for the label in the u32
> or 3 bit bits in the u8 tc.

The label and tc are masked - maybe just the BOS
needs something similar?

cheers,
jamal

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jakub Kicinski]

On Mon, 24 Apr 2017 22:06:08 -0400, Jamal Hadi Salim wrote:
> On 17-04-24 10:00 PM, Jamal Hadi Salim wrote:
> > On 17-04-24 09:48 PM, Jamal Hadi Salim wrote:  
> 
> >
> > Hrm. maybe I am wrong.
> > Lets say user sets all of the 8 bits in BOS,
> > what does setting
> > key_val->mpls_bos = nla_get_u8 do?
> >
> > Same with the 20 bits for the label in the u32
> > or 3 bit bits in the u8 tc.  
> 
> The label and tc are masked - maybe just the BOS
> needs something similar?

Indeed, good catch!

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Simon Horman]

On Mon, Apr 24, 2017 at 07:07:43PM -0700, Jakub Kicinski wrote:
> On Mon, 24 Apr 2017 22:06:08 -0400, Jamal Hadi Salim wrote:
> > On 17-04-24 10:00 PM, Jamal Hadi Salim wrote:
> > > On 17-04-24 09:48 PM, Jamal Hadi Salim wrote:  
> > 
> > >
> > > Hrm. maybe I am wrong.
> > > Lets say user sets all of the 8 bits in BOS,
> > > what does setting
> > > key_val->mpls_bos = nla_get_u8 do?
> > >
> > > Same with the 20 bits for the label in the u32
> > > or 3 bit bits in the u8 tc.  
> > 
> > The label and tc are masked - maybe just the BOS
> > needs something similar?
> 
> Indeed, good catch!

I agree something should be done wrt BOS. If the LABEL and TC are to
be left as-is then I think a similar treatment of BOS - that is masking it
- makes sense.

I also agree with statements made earlier in the thread that it is unlikely
that the unused bits of these attributes will be used - as opposed to a
bitmask of flag values which seems ripe for re-use for future flags.

I would like to add to the discussion that I think in future it would
be good to expand the features provided by this patch to support supplying
a mask as part of the match - as flower supports for other fields such
as IP addresses. But I think the current scheme of masking out invalid bits
should also work in conjunction with user-supplied masks.

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Jamal Hadi Salim]

On 17-04-25 07:55 AM, Simon Horman wrote:
[..]
>
> I agree something should be done wrt BOS. If the LABEL and TC are to
> be left as-is then I think a similar treatment of BOS - that is masking it
> - makes sense.
>
> I also agree with statements made earlier in the thread that it is unlikely
> that the unused bits of these attributes will be used - as opposed to a
> bitmask of flag values which seems ripe for re-use for future flags.
>

For your use case, I think you are fine if you just do the mask in the
kernel. A mask  to a user value implies "I am ignoring the rest
of these bits - I dont  care if you set them "

> I would like to add to the discussion that I think in future it would
> be good to expand the features provided by this patch to support supplying
> a mask as part of the match - as flower supports for other fields such
> as IP addresses. But I think the current scheme of masking out invalid bits
> should also work in conjunction with user-supplied masks.
>

The challenge we have right now is "users do stoopid or malicious
things". So are you going to accept the wrong bitmap + mask?

cheers,
jamal

Re: [PATCH net-next 0/2] flower: add MPLS matching support

[Benjamin LaHaise]

On Tue, Apr 25, 2017 at 08:47:00AM -0400, Jamal Hadi Salim wrote:
> On 17-04-25 07:55 AM, Simon Horman wrote:
> [..]
> > 
> > I agree something should be done wrt BOS. If the LABEL and TC are to
> > be left as-is then I think a similar treatment of BOS - that is masking it
> > - makes sense.
> > 
> > I also agree with statements made earlier in the thread that it is unlikely
> > that the unused bits of these attributes will be used - as opposed to a
> > bitmask of flag values which seems ripe for re-use for future flags.
> > 
> 
> For your use case, I think you are fine if you just do the mask in the
> kernel. A mask  to a user value implies "I am ignoring the rest
> of these bits - I dont  care if you set them "
> 
> > I would like to add to the discussion that I think in future it would
> > be good to expand the features provided by this patch to support supplying
> > a mask as part of the match - as flower supports for other fields such
> > as IP addresses. But I think the current scheme of masking out invalid bits
> > should also work in conjunction with user-supplied masks.
> > 
> 
> The challenge we have right now is "users do stoopid or malicious
> things". So are you going to accept the wrong bitmap + mask?

I think rejecting bits in a mask that clearly cannot be set (like the 
bits above the lower 20 bits in an MPLS label) makes perfect sense.  It 
doesn't impact usability for the tools since they shouldn't be set (and 
actually can't be in the iproute2 changes).

		-ben

> cheers,
> jamal
>