Skip to contentSkip to conversation

cls_flower: Use extack in fl_set_key()

PATCH In review Guillaume Nault 2 participants 6 messages Mar 23, 2020 – Mar 27, 2020

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Guillaume Nault

author
#0 lore raw

cover cls_flower: Use extack in fl_set_key()

Add missing extack messages in fl_set_key(), so that users can get more meaningfull error messages when netlink attributes are rejected.

Patch 1 also extends extack in tcf_change_indev() (in pkt_cls.h) since this function is used by fl_set_key().

Guillaume Nault (4):
net: sched: refine extack messages in tcf_change_indev
cls_flower: Add extack support for mpls options
cls_flower: Add extack support for src and dst port range options
cls_flower: Add extack support for flags key

 include/net/pkt_cls.h  |  8 ++++--
 net/sched/cls_flower.c | 60 ++++++++++++++++++++++++++++++------------
 2 files changed, 49 insertions(+), 19 deletions(-)
Signature · 1 line
2.21.1
author
#1 lore raw

PATCH 1/4 net: sched: refine extack messages in tcf_change_indev

Add an error message when device wasn't found. While there, also set the bad attribute's offset in extack.

Signed-off-by Guillaume Nault <[email protected]>
---
 include/net/pkt_cls.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
include/net/pkt_cls.h +6 -2
index 1db8b27d4515..41902e10d503 100644
@@ -502,12 +502,16 @@ tcf_change_indev(struct net *net, struct nlattr *indev_tlv,
 	struct net_device *dev;
 
 	if (nla_strlcpy(indev, indev_tlv, IFNAMSIZ) >= IFNAMSIZ) {
-		NL_SET_ERR_MSG(extack, "Interface name too long");
+		NL_SET_ERR_MSG_ATTR(extack, indev_tlv,
+				    "Interface name too long");
 		return -EINVAL;
 	}
 	dev = __dev_get_by_name(net, indev);
-	if (!dev)
+	if (!dev) {
+		NL_SET_ERR_MSG_ATTR(extack, indev_tlv,
+				    "Network device not found");
 		return -ENODEV;
+	}
 	return dev->ifindex;
 }
Signature · 1 line
2.21.1
author
#2 lore raw

PATCH 2/4 cls_flower: Add extack support for mpls options

In reply to Guillaume Nault

Pass extack down to fl_set_key_mpls() and set message on error.

Signed-off-by Guillaume Nault <[email protected]>
---
 net/sched/cls_flower.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)
net/sched/cls_flower.c +18 -5
index 258dc45ab7e3..544cc7b490a3 100644
@@ -766,7 +766,8 @@ static int fl_set_key_port_range(struct nlattr **tb, struct fl_flow_key *key,
 
 static int fl_set_key_mpls(struct nlattr **tb,
 			   struct flow_dissector_key_mpls *key_val,
-			   struct flow_dissector_key_mpls *key_mask)
+			   struct flow_dissector_key_mpls *key_mask,
+			   struct netlink_ext_ack *extack)
 {
 	if (tb[TCA_FLOWER_KEY_MPLS_TTL]) {
 		key_val->mpls_ttl = nla_get_u8(tb[TCA_FLOWER_KEY_MPLS_TTL]);

@@ -775,24 +776,36 @@ static int fl_set_key_mpls(struct nlattr **tb,
 	if (tb[TCA_FLOWER_KEY_MPLS_BOS]) {
 		u8 bos = nla_get_u8(tb[TCA_FLOWER_KEY_MPLS_BOS]);
 
-		if (bos & ~MPLS_BOS_MASK)
+		if (bos & ~MPLS_BOS_MASK) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_FLOWER_KEY_MPLS_BOS],
+					    "Bottom Of Stack (BOS) must be 0 or 1");
 			return -EINVAL;
+		}
 		key_val->mpls_bos = bos;
 		key_mask->mpls_bos = MPLS_BOS_MASK;
 	}
 	if (tb[TCA_FLOWER_KEY_MPLS_TC]) {
 		u8 tc = nla_get_u8(tb[TCA_FLOWER_KEY_MPLS_TC]);
 
-		if (tc & ~MPLS_TC_MASK)
+		if (tc & ~MPLS_TC_MASK) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_FLOWER_KEY_MPLS_TC],
+					    "Traffic Class (TC) must be between 0 and 7");
 			return -EINVAL;
+		}
 		key_val->mpls_tc = tc;
 		key_mask->mpls_tc = MPLS_TC_MASK;
 	}
 	if (tb[TCA_FLOWER_KEY_MPLS_LABEL]) {
 		u32 label = nla_get_u32(tb[TCA_FLOWER_KEY_MPLS_LABEL]);
 
-		if (label & ~MPLS_LABEL_MASK)
+		if (label & ~MPLS_LABEL_MASK) {
+			NL_SET_ERR_MSG_ATTR(extack,
+					    tb[TCA_FLOWER_KEY_MPLS_LABEL],
+					    "Label must be between 0 and 1048575");
 			return -EINVAL;
+		}
 		key_val->mpls_label = label;
 		key_mask->mpls_label = MPLS_LABEL_MASK;
 	}

@@ -1364,7 +1377,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 			       sizeof(key->icmp.code));
 	} else if (key->basic.n_proto == htons(ETH_P_MPLS_UC) ||
 		   key->basic.n_proto == htons(ETH_P_MPLS_MC)) {
-		ret = fl_set_key_mpls(tb, &key->mpls, &mask->mpls);
+		ret = fl_set_key_mpls(tb, &key->mpls, &mask->mpls, extack);
 		if (ret)
 			return ret;
 	} else if (key->basic.n_proto == htons(ETH_P_ARP) ||
Signature · 1 line
2.21.1
author
#3 lore raw

PATCH 3/4 cls_flower: Add extack support for src and dst port range options

In reply to Guillaume Nault

Pass extack down to fl_set_key_port_range() and set message on error.

Both the min and max ports would qualify as invalid attributes here. Report the min one as invalid, as it's probably what makes the most sense from a user point of view.

Signed-off-by Guillaume Nault <[email protected]>
---
 net/sched/cls_flower.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)
net/sched/cls_flower.c +18 -8
index 544cc7b490a3..5811dd971ee5 100644
@@ -738,7 +738,8 @@ static void fl_set_key_val(struct nlattr **tb,
 }
 
 static int fl_set_key_port_range(struct nlattr **tb, struct fl_flow_key *key,
-				 struct fl_flow_key *mask)
+				 struct fl_flow_key *mask,
+				 struct netlink_ext_ack *extack)
 {
 	fl_set_key_val(tb, &key->tp_range.tp_min.dst,
 		       TCA_FLOWER_KEY_PORT_DST_MIN, &mask->tp_range.tp_min.dst,

@@ -753,13 +754,22 @@ static int fl_set_key_port_range(struct nlattr **tb, struct fl_flow_key *key,
 		       TCA_FLOWER_KEY_PORT_SRC_MAX, &mask->tp_range.tp_max.src,
 		       TCA_FLOWER_UNSPEC, sizeof(key->tp_range.tp_max.src));
 
-	if ((mask->tp_range.tp_min.dst && mask->tp_range.tp_max.dst &&
-	     htons(key->tp_range.tp_max.dst) <=
-		 htons(key->tp_range.tp_min.dst)) ||
-	    (mask->tp_range.tp_min.src && mask->tp_range.tp_max.src &&
-	     htons(key->tp_range.tp_max.src) <=
-		 htons(key->tp_range.tp_min.src)))
+	if (mask->tp_range.tp_min.dst && mask->tp_range.tp_max.dst &&
+	    htons(key->tp_range.tp_max.dst) <=
+	    htons(key->tp_range.tp_min.dst)) {
+		NL_SET_ERR_MSG_ATTR(extack,
+				    tb[TCA_FLOWER_KEY_PORT_DST_MIN],
+				    "Invalid destination port range (min must be strictly smaller than max)");
 		return -EINVAL;
+	}
+	if (mask->tp_range.tp_min.src && mask->tp_range.tp_max.src &&
+	    htons(key->tp_range.tp_max.src) <=
+	    htons(key->tp_range.tp_min.src)) {
+		NL_SET_ERR_MSG_ATTR(extack,
+				    tb[TCA_FLOWER_KEY_PORT_SRC_MIN],
+				    "Invalid source port range (min must be strictly smaller than max)");
+		return -EINVAL;
+	}
 
 	return 0;
 }

@@ -1402,7 +1412,7 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 	if (key->basic.ip_proto == IPPROTO_TCP ||
 	    key->basic.ip_proto == IPPROTO_UDP ||
 	    key->basic.ip_proto == IPPROTO_SCTP) {
-		ret = fl_set_key_port_range(tb, key, mask);
+		ret = fl_set_key_port_range(tb, key, mask, extack);
 		if (ret)
 			return ret;
 	}
Signature · 1 line
2.21.1
author
#4 lore raw

PATCH 4/4 cls_flower: Add extack support for flags key

In reply to Guillaume Nault

Pass extack down to fl_set_key_flags() and set message on error.

Signed-off-by Guillaume Nault <[email protected]>
---
 net/sched/cls_flower.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
net/sched/cls_flower.c +7 -4
index 5811dd971ee5..9b6acd736dc8 100644
@@ -856,14 +856,16 @@ static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
 	}
 }
 
-static int fl_set_key_flags(struct nlattr **tb,
-			    u32 *flags_key, u32 *flags_mask)
+static int fl_set_key_flags(struct nlattr **tb, u32 *flags_key,
+			    u32 *flags_mask, struct netlink_ext_ack *extack)
 {
 	u32 key, mask;
 
 	/* mask is mandatory for flags */
-	if (!tb[TCA_FLOWER_KEY_FLAGS_MASK])
+	if (!tb[TCA_FLOWER_KEY_FLAGS_MASK]) {
+		NL_SET_ERR_MSG(extack, "Missing flags mask");
 		return -EINVAL;
+	}
 
 	key = be32_to_cpu(nla_get_u32(tb[TCA_FLOWER_KEY_FLAGS]));
 	mask = be32_to_cpu(nla_get_u32(tb[TCA_FLOWER_KEY_FLAGS_MASK]));

@@ -1474,7 +1476,8 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 		return ret;
 
 	if (tb[TCA_FLOWER_KEY_FLAGS])
-		ret = fl_set_key_flags(tb, &key->control.flags, &mask->control.flags);
+		ret = fl_set_key_flags(tb, &key->control.flags,
+				       &mask->control.flags, extack);
 
 	return ret;
 }
Signature · 1 line
2.21.1
maintainer
#5 lore raw
In reply to Guillaume Nault

From: Guillaume Nault <[email protected]>
Date: Mon, 23 Mar 2020 21:48:45 +0100

Quoted 5 lines

Add missing extack messages in fl_set_key(), so that users can get more meaningfull error messages when netlink attributes are rejected.

Patch 1 also extends extack in tcf_change_indev() (in pkt_cls.h) since this function is used by fl_set_key().

Series applied, thanks.