netlink: strict attribute checking option

RFC PATCH In review Jiri Benc 5 participants 23 messages Oct 15, 2015 – Oct 19, 2015

Subsystems (15)

Networking Network drivers Library code netfilter +11 more subsystems

lore

Share & tools

lore

Jiri Benc

author

2015-10-15 16:39 UTC #0 lore raw

cover netlink: strict attribute checking option

When sending a netlink request (NLM_F_REQUEST), any unknown attributes are ignored. This behavior is problematic in some situations. For example if the user asks for a particular config, the request finishes successfully, yet the configuraton applied does not work because some of the attributes are not supported by the kernel (e.g. because it's older than the user space tool).

We're hitting this when configuring metadata based lwtunnels from user space
applications: it would be surely nice if iproute2 warned about this, as the resulting tunnel silently won't work with encap routes. Worse, it's quite crucial for openvswitch and similar programs when deciding what kernel API (lwtunnel vs. legacy) to use.

This patchset tries to solve this. Everything should be compatible with old kernels that don't have this patchset.

Before using this new facility, the application should check whether it is supported by the kernel. This is done by sending a NLMSG_NOOP message with NLM_F_REQUEST | NLM_F_STRICT | NLM_F_ACK flags set. If the returned message has NLM_F_STRICT set, the kernel does support NLM_F_STRICT flag; otherwise, the flag should not be used.

When NLM_F_STRICT is set in a request, the request undergoes stricter checking for compatibility with the current kernel. Presence of any attribute unknown to the kernel will mean the message will be rejected and error returned. In case of success, the request has been performed in the same way as if NLM_F_STRICT was not set.

However, the sole fact that NLM_F_STRICT is supported by the kernel does not yet mean it is supported for the particular netlink family and/or message type. If EPROTO is returned in an error reply, it means that the particular netlink family/message does not support NLM_F_STRICT (yet). In such case, the application could fall back to operation without using NLM_F_STRICT (which is needed for older kernels anyway), possibly warning users that some options may have been ignored by the kernel, if appropriate.

Another option for using this facility is just set NLM_F_STRICT with NLM_F_ACK in a request. There are four results that could happen:

1. Request succeeds and NLM_F_STRICT is set in the reply. Everything is good and verified in this case.
2. Request succeeds but NLM_F_STRICT is not set in the reply. The request was processed but some attributes might (or might not) be ignored because the kernel does not understand NLM_F_STRICT. Warn user if appropriate.
3. Request fails with EPROTO. This means NLM_F_STRICT is not supported for this request. Depending on the needs of the application, either fail hard or try again without NLM_F_STRICT, warning user if appropriate.
4. Request fails with other error code. The request did not succeed.

Applications with hard depency on new kernel features that implement strict checking may choose to abort if NLM_F_STRICT is not set in the reply to NLMSG_NOOP, always use NLM_F_STRICT and treat EPROTO as any other error code.

Note that this RFC patchset is completely untested and may not compile on all configs. I'd like to get feedback on whether this makes sense. For example, I'm not thrilled about the EPROTO error code. Suggestions for a better one are welcome - it needs to be one that is never returned by any netlink operation, though.

Jiri Benc (9):
netlink: add NLM_F_STRICT for strict attribute checking
netlink: remove unnecesary goto's
netlink: strict attribute parsing
netlink: strict attribute validation
rtnetlink: support strict attribute checking
rtnetlink: add strict parameter to validate callbacks
rtnetlink: add strict parameter to validate_link_af
rtnetlink: support strict checking for newlink, setlink and dellink
veth: validate nested attributes

 crypto/crypto_user.c               |   2 +-
 drivers/infiniband/core/netlink.c  |   2 +-
 drivers/net/bonding/bond_netlink.c |   3 +-
 drivers/net/dummy.c                |   3 +-
 drivers/net/geneve.c               |   3 +-
 drivers/net/ifb.c                  |   3 +-
 drivers/net/ipvlan/ipvlan_main.c   |   3 +-
 drivers/net/macvlan.c              |   3 +-
 drivers/net/nlmon.c                |   3 +-
 drivers/net/team/team.c            |   3 +-
 drivers/net/tun.c                  |   3 +-
 drivers/net/veth.c                 |  13 +++-
 drivers/net/vrf.c                  |   3 +-
 drivers/net/vxlan.c                |   3 +-
 include/net/netlink.h              | 144 ++++++++++++++++++++++++++++++++-----
 include/net/rtnetlink.h            |  37 ++++++++--
 include/uapi/linux/netlink.h       |   1 +
 lib/nlattr.c                       |  56 +++++++++------
 net/8021q/vlan_netlink.c           |  12 ++--
 net/bridge/br_netlink.c            |   3 +-
 net/core/rtnetlink.c               | 143 ++++++++++++++++++++++--------------
 net/core/sock_diag.c               |   2 +-
 net/ieee802154/6lowpan/core.c      |   3 +-
 net/ipv4/devinet.c                 |   6 +-
 net/ipv4/ip_gre.c                  |   8 ++-
 net/ipv4/ip_vti.c                  |   3 +-
 net/ipv6/addrconf.c                |   6 +-
 net/ipv6/ip6_gre.c                 |   8 ++-
 net/ipv6/ip6_tunnel.c              |   3 +-
 net/ipv6/ip6_vti.c                 |   3 +-
 net/ipv6/sit.c                     |   3 +-
 net/netfilter/nfnetlink.c          |   6 +-
 net/netlink/af_netlink.c           |  14 +++-
 net/netlink/genetlink.c            |   2 +-
 net/xfrm/xfrm_user.c               |   2 +-
 35 files changed, 371 insertions(+), 144 deletions(-)

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #1 lore raw

RFC PATCH 1/9 netlink: add NLM_F_STRICT for strict attribute checking

When NLM_F_STRICT flag is set in a request, the message should undergo stricter check for compatibility with the current kernel, or EPROTO should be returned if such check could not be performed. In addition, NLM_F_STRICT is set in error/ack reply to a request with this flag set.

For now, always return EPROTO.

Signed-off-by Jiri Benc <[email protected]>

---
 crypto/crypto_user.c              |  2 +-
 drivers/infiniband/core/netlink.c |  2 +-
 include/net/netlink.h             |  2 +-
 include/uapi/linux/netlink.h      |  1 +
 net/core/rtnetlink.c              |  2 +-
 net/core/sock_diag.c              |  2 +-
 net/netfilter/nfnetlink.c         |  6 +++++-
 net/netlink/af_netlink.c          | 14 +++++++++++---
 net/netlink/genetlink.c           |  2 +-
 net/xfrm/xfrm_user.c              |  2 +-
 10 files changed, 24 insertions(+), 11 deletions(-)

crypto/crypto_user.c +1 -1

index d94d99ffe8b9..8d46d3b69bee 100644
@@ -526,7 +526,7 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 static void crypto_netlink_rcv(struct sk_buff *skb)
 {
 	mutex_lock(&crypto_cfg_mutex);
-	netlink_rcv_skb(skb, &crypto_user_rcv_msg);
+	netlink_rcv_skb(skb, false, &crypto_user_rcv_msg);
 	mutex_unlock(&crypto_cfg_mutex);
 }

drivers/infiniband/core/netlink.c +1 -1

index d47df9356779..1429c0cf583b 100644
@@ -223,7 +223,7 @@ static void ibnl_rcv(struct sk_buff *skb)
 {
 	mutex_lock(&ibnl_mutex);
 	ibnl_rcv_reply_skb(skb);
-	netlink_rcv_skb(skb, &ibnl_rcv_msg);
+	netlink_rcv_skb(skb, false, &ibnl_rcv_msg);
 	mutex_unlock(&ibnl_mutex);
 }

include/net/netlink.h +1 -1

index 0e3172751755..160e98cea304 100644
@@ -228,7 +228,7 @@ struct nl_info {
 	u32			portid;
 };
 
-int netlink_rcv_skb(struct sk_buff *skb,
+int netlink_rcv_skb(struct sk_buff *skb, bool strict_supported,
 		    int (*cb)(struct sk_buff *, struct nlmsghdr *));
 int nlmsg_notify(struct sock *sk, struct sk_buff *skb, u32 portid,
 		 unsigned int group, int report, gfp_t flags);

include/uapi/linux/netlink.h +1 -0

index f095155d8749..c001551fcedd 100644
@@ -55,6 +55,7 @@ struct nlmsghdr {
 #define NLM_F_ECHO		8	/* Echo this request 		*/
 #define NLM_F_DUMP_INTR		16	/* Dump was inconsistent due to sequence change */
 #define NLM_F_DUMP_FILTERED	32	/* Dump was filtered as requested */
+#define NLM_F_STRICT		64	/* Fail if an attr is unsupported */
 
 /* Modifiers to GET request */
 #define NLM_F_ROOT	0x100	/* specify tree	root	*/

net/core/rtnetlink.c +1 -1

index 24775953fa68..afb41c7492b4 100644
@@ -3356,7 +3356,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 static void rtnetlink_rcv(struct sk_buff *skb)
 {
 	rtnl_lock();
-	netlink_rcv_skb(skb, &rtnetlink_rcv_msg);
+	netlink_rcv_skb(skb, false, &rtnetlink_rcv_msg);
 	rtnl_unlock();
 }

net/core/sock_diag.c +1 -1

index 0c1d58d43f67..3aac827d1d67 100644
@@ -272,7 +272,7 @@ static DEFINE_MUTEX(sock_diag_mutex);
 static void sock_diag_rcv(struct sk_buff *skb)
 {
 	mutex_lock(&sock_diag_mutex);
-	netlink_rcv_skb(skb, &sock_diag_rcv_msg);
+	netlink_rcv_skb(skb, false, &sock_diag_rcv_msg);
 	mutex_unlock(&sock_diag_mutex);
 }

net/netfilter/nfnetlink.c +5 -1

index f1d9e887f5b1..f7588dec4d23 100644
@@ -339,6 +339,10 @@ replay:
 			err = -EINVAL;
 			goto ack;
 		}
+		if (nlh->nlmsg_flags & NLM_F_STRICT) {
+			err = -EPROTO;
+			goto ack;
+		}
 
 		type = nlh->nlmsg_type;
 		if (type == NFNL_MSG_BATCH_BEGIN) {

@@ -476,7 +480,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 			res_id = ntohs(nfgenmsg->res_id);
 		nfnetlink_rcv_batch(skb, nlh, res_id);
 	} else {
-		netlink_rcv_skb(skb, &nfnetlink_rcv_msg);
+		netlink_rcv_skb(skb, false, &nfnetlink_rcv_msg);
 	}
 }

net/netlink/af_netlink.c +11 -3

index 8f060d7f9a0e..d59d02038128 100644
@@ -2944,6 +2944,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 	struct nlmsgerr *errmsg;
 	size_t payload = sizeof(*errmsg);
 	struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
+	unsigned int flags = 0;
 
 	/* Error messages get the original request appened, unless the user
 	 * requests to cap the error message.

@@ -2967,8 +2968,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 		return;
 	}
 
+	if (nlh->nlmsg_flags & NLM_F_STRICT)
+		flags |= NLM_F_STRICT;
 	rep = __nlmsg_put(skb, NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
-			  NLMSG_ERROR, payload, 0);
+			  NLMSG_ERROR, payload, flags);
 	errmsg = nlmsg_data(rep);
 	errmsg->error = err;
 	memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh));

@@ -2976,8 +2979,8 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 }
 EXPORT_SYMBOL(netlink_ack);
 
-int netlink_rcv_skb(struct sk_buff *skb, int (*cb)(struct sk_buff *,
-						     struct nlmsghdr *))
+int netlink_rcv_skb(struct sk_buff *skb, bool strict_supported,
+		    int (*cb)(struct sk_buff *, struct nlmsghdr *))
 {
 	struct nlmsghdr *nlh;
 	int err;

@@ -2995,6 +2998,11 @@ int netlink_rcv_skb(struct sk_buff *skb, int (*cb)(struct sk_buff *,
 		if (!(nlh->nlmsg_flags & NLM_F_REQUEST))
 			goto ack;
 
+		if (!strict_supported && (nlh->nlmsg_flags & NLM_F_STRICT)) {
+			err = -EPROTO;
+			goto ack;
+		}
+
 		/* Skip control messages */
 		if (nlh->nlmsg_type < NLMSG_MIN_TYPE)
 			goto ack;

net/netlink/genetlink.c +1 -1

index bc0e504f33a6..d7bf14420c9e 100644
@@ -667,7 +667,7 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 static void genl_rcv(struct sk_buff *skb)
 {
 	down_read(&cb_lock);
-	netlink_rcv_skb(skb, &genl_rcv_msg);
+	netlink_rcv_skb(skb, false, &genl_rcv_msg);
 	up_read(&cb_lock);
 }

net/xfrm/xfrm_user.c +1 -1

index a8de9e300200..bcde073f59db 100644
@@ -2491,7 +2491,7 @@ static void xfrm_netlink_rcv(struct sk_buff *skb)
 	struct net *net = sock_net(skb->sk);
 
 	mutex_lock(&net->xfrm.xfrm_cfg_mutex);
-	netlink_rcv_skb(skb, &xfrm_user_rcv_msg);
+	netlink_rcv_skb(skb, false, &xfrm_user_rcv_msg);
 	mutex_unlock(&net->xfrm.xfrm_cfg_mutex);
 }

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #2 lore raw

RFC PATCH 2/9 netlink: remove unnecesary goto's

In reply to Jiri Benc

Jumping to the return makes the code less readable. The compiler will optimize this just fine.

Signed-off-by Jiri Benc <[email protected]>

---
 lib/nlattr.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

lib/nlattr.c +4 -8

index f5907d23272d..b35c3e6c8e81 100644
@@ -128,12 +128,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
 	nla_for_each_attr(nla, head, len, rem) {
 		err = validate_nla(nla, maxtype, policy);
 		if (err < 0)
-			goto errout;
+			return err;
 	}
 
-	err = 0;
-errout:
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(nla_validate);

@@ -194,7 +192,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 			if (policy) {
 				err = validate_nla(nla, maxtype, policy);
 				if (err < 0)
-					goto errout;
+					return err;
 			}
 
 			tb[type] = (struct nlattr *)nla;

@@ -205,9 +203,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 		pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n",
 				    rem, current->comm);
 
-	err = 0;
-errout:
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(nla_parse);

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #3 lore raw

RFC PATCH 3/9 netlink: strict attribute parsing

In reply to Jiri Benc

Enhance the attribute parsing functions to support strict attribute checking. Keep the semantics of the current nla_parse and nlmsg_parse functions, it's better then changing hundreds of call sites all around the kernel.

Signed-off-by Jiri Benc <[email protected]>

---
 include/net/netlink.h | 75 ++++++++++++++++++++++++++++++++++++++++++++-------
 lib/nlattr.c          | 20 +++++++++-----
 2 files changed, 80 insertions(+), 15 deletions(-)

include/net/netlink.h +66 -9

index 160e98cea304..dcca6853913d 100644
@@ -235,8 +235,9 @@ int nlmsg_notify(struct sock *sk, struct sk_buff *skb, u32 portid,
 
 int nla_validate(const struct nlattr *head, int len, int maxtype,
 		 const struct nla_policy *policy);
-int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
-	      int len, const struct nla_policy *policy);
+int nla_strict_parse(struct nlattr **tb, int maxtype, bool strict,
+		     const struct nlattr *head, int len,
+		     const struct nla_policy *policy);
 int nla_policy_len(const struct nla_policy *, int);
 struct nlattr *nla_find(const struct nlattr *head, int len, int attrtype);
 size_t nla_strlcpy(char *dst, const struct nlattr *nla, size_t dstsize);

@@ -356,6 +357,30 @@ nlmsg_next(const struct nlmsghdr *nlh, int *remaining)
 }
 
 /**
+ * nlmsg_strict_parse - parse attributes of a netlink message
+ * @nlh: netlink message header
+ * @hdrlen: length of family specific header
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @strict: whether to perform strict checking
+ * @policy: validation policy
+ *
+ * See nla_strict_parse().
+ */
+static inline int nlmsg_strict_parse(const struct nlmsghdr *nlh, int hdrlen,
+				     struct nlattr *tb[], int maxtype,
+				     bool strict,
+				     const struct nla_policy *policy)
+{
+	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
+		return -EINVAL;
+
+	return nla_strict_parse(tb, maxtype, strict,
+				nlmsg_attrdata(nlh, hdrlen),
+				nlmsg_attrlen(nlh, hdrlen), policy);
+}
+
+/**
  * nlmsg_parse - parse attributes of a netlink message
  * @nlh: netlink message header
  * @hdrlen: length of family specific header

@@ -363,17 +388,13 @@ nlmsg_next(const struct nlmsghdr *nlh, int *remaining)
  * @maxtype: maximum attribute type to be expected
  * @policy: validation policy
  *
- * See nla_parse()
+ * See nla_strict_parse(). Strict checking is not performed.
  */
 static inline int nlmsg_parse(const struct nlmsghdr *nlh, int hdrlen,
 			      struct nlattr *tb[], int maxtype,
 			      const struct nla_policy *policy)
 {
-	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
-		return -EINVAL;
-
-	return nla_parse(tb, maxtype, nlmsg_attrdata(nlh, hdrlen),
-			 nlmsg_attrlen(nlh, hdrlen), policy);
+	return nlmsg_strict_parse(nlh, hdrlen, tb, maxtype, false, policy);
 }
 
 /**

@@ -722,13 +743,49 @@ nla_find_nested(const struct nlattr *nla, int attrtype)
 }
 
 /**
+ * nla_parse - Parse a stream of attributes into a tb buffer
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @policy: validation policy
+ *
+ * See nla_strict_parse(). Strict checking is not performed.
+ */
+static inline int nla_parse(struct nlattr **tb, int maxtype,
+			    const struct nlattr *head, int len,
+			    const struct nla_policy *policy)
+{
+	return nla_strict_parse(tb, maxtype, false, head, len, policy);
+}
+
+/**
+ * nla_strict_parse_nested - parse nested attributes
+ * @tb: destination array with maxtype+1 elements
+ * @maxtype: maximum attribute type to be expected
+ * @strict: whether to perform strict checking
+ * @nla: attribute containing the nested attributes
+ * @policy: validation policy
+ *
+ * See nla_strict_parse(). Strict checking is not performed.
+ */
+static inline int nla_strict_parse_nested(struct nlattr *tb[], int maxtype,
+					  bool strict,
+					  const struct nlattr *nla,
+					  const struct nla_policy *policy)
+{
+	return nla_strict_parse(tb, maxtype, strict, nla_data(nla),
+				nla_len(nla), policy);
+}
+
+/**
  * nla_parse_nested - parse nested attributes
  * @tb: destination array with maxtype+1 elements
  * @maxtype: maximum attribute type to be expected
  * @nla: attribute containing the nested attributes
  * @policy: validation policy
  *
- * See nla_parse()
+ * See nla_strict_parse()
  */
 static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
 				   const struct nlattr *nla,

lib/nlattr.c +14 -6

index b35c3e6c8e81..2b36388eb3a9 100644
@@ -163,9 +163,10 @@ nla_policy_len(const struct nla_policy *p, int n)
 EXPORT_SYMBOL(nla_policy_len);
 
 /**
- * nla_parse - Parse a stream of attributes into a tb buffer
+ * nla_strict_parse - Parse a stream of attributes into a tb buffer
  * @tb: destination array with maxtype+1 elements
  * @maxtype: maximum attribute type to be expected
+ * @strict: whether to perform strict checking
  * @head: head of attribute stream
  * @len: length of attribute stream
  * @policy: validation policy

@@ -173,12 +174,14 @@ EXPORT_SYMBOL(nla_policy_len);
  * Parses a stream of attributes and stores a pointer to each attribute in
  * the tb array accessible via the attribute type. Attributes with a type
  * exceeding maxtype will be silently ignored for backwards compatibility
- * reasons. policy may be set to NULL if no validation is required.
+ * reasons, unless strict is set. policy may be set to NULL if no validation
+ * is required.
  *
  * Returns 0 on success or a negative error code.
  */
-int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
-	      int len, const struct nla_policy *policy)
+int nla_strict_parse(struct nlattr **tb, int maxtype, bool strict,
+		     const struct nlattr *head, int len,
+		     const struct nla_policy *policy)
 {
 	const struct nlattr *nla;
 	int rem, err;

@@ -196,16 +199,21 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 			}
 
 			tb[type] = (struct nlattr *)nla;
+		} else if (strict) {
+			return -EINVAL;
 		}
 	}
 
-	if (unlikely(rem > 0))
+	if (unlikely(rem > 0)) {
 		pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n",
 				    rem, current->comm);
+		if (strict)
+			return -EINVAL;
+	}
 
 	return 0;
 }
-EXPORT_SYMBOL(nla_parse);
+EXPORT_SYMBOL(nla_strict_parse);
 
 /**
  * nla_find - Find a specific attribute in a stream of attributes

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #4 lore raw

RFC PATCH 4/9 netlink: strict attribute validation

In reply to Jiri Benc

Enhance the attribute validation functions to support strict attribute checking. Keep the semantics of the current nla_validate and nlmsg_validate functions.

Signed-off-by Jiri Benc <[email protected]>

---
 include/net/netlink.h | 67 ++++++++++++++++++++++++++++++++++++++++++++-------
 lib/nlattr.c          | 24 +++++++++++-------
 2 files changed, 73 insertions(+), 18 deletions(-)

include/net/netlink.h +58 -9

index dcca6853913d..233a68e1c955 100644
@@ -233,8 +233,8 @@ int netlink_rcv_skb(struct sk_buff *skb, bool strict_supported,
 int nlmsg_notify(struct sock *sk, struct sk_buff *skb, u32 portid,
 		 unsigned int group, int report, gfp_t flags);
 
-int nla_validate(const struct nlattr *head, int len, int maxtype,
-		 const struct nla_policy *policy);
+int nla_strict_validate(const struct nlattr *head, int len, int maxtype,
+			bool strict, const struct nla_policy *policy);
 int nla_strict_parse(struct nlattr **tb, int maxtype, bool strict,
 		     const struct nlattr *head, int len,
 		     const struct nla_policy *policy);

@@ -413,6 +413,26 @@ static inline struct nlattr *nlmsg_find_attr(const struct nlmsghdr *nlh,
 }
 
 /**
+ * nlmsg_strict_validate - validate a netlink message including attributes
+ * @nlh: netlinket message header
+ * @hdrlen: length of familiy specific header
+ * @maxtype: maximum attribute type to be expected
+ * @strict: whether to perform strict checking
+ * @policy: validation policy
+ */
+static inline int nlmsg_strict_validate(const struct nlmsghdr *nlh,
+					int hdrlen, int maxtype, bool strict,
+					const struct nla_policy *policy)
+{
+	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
+		return -EINVAL;
+
+	return nla_strict_validate(nlmsg_attrdata(nlh, hdrlen),
+				   nlmsg_attrlen(nlh, hdrlen), maxtype,
+				   strict, policy);
+}
+
+/**
  * nlmsg_validate - validate a netlink message including attributes
  * @nlh: netlinket message header
  * @hdrlen: length of familiy specific header

@@ -423,11 +443,7 @@ static inline int nlmsg_validate(const struct nlmsghdr *nlh,
 				 int hdrlen, int maxtype,
 				 const struct nla_policy *policy)
 {
-	if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
-		return -EINVAL;
-
-	return nla_validate(nlmsg_attrdata(nlh, hdrlen),
-			    nlmsg_attrlen(nlh, hdrlen), maxtype, policy);
+	return nlmsg_strict_validate(nlh, hdrlen, maxtype, false, policy);
 }
 
 /**

@@ -1270,17 +1286,50 @@ static inline void nla_nest_cancel(struct sk_buff *skb, struct nlattr *start)
 }
 
 /**
- * nla_validate_nested - Validate a stream of nested attributes
+ * nla_validate - Validate a stream of attributes
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ *
+ * See nla_strict_validate(). Strict checking is not performed.
+ */
+static inline int nla_validate(const struct nlattr *head, int len,
+			       int maxtype, const struct nla_policy *policy)
+{
+	return nla_strict_validate(head, len, maxtype, false, policy);
+}
+
+/**
+ * nla_strict_validate_nested - Validate a stream of nested attributes
  * @start: container attribute
  * @maxtype: maximum attribute type to be expected
+ * @strict: whether to perform strict checking
  * @policy: validation policy
  *
  * Validates all attributes in the nested attribute stream against the
  * specified policy. Attributes with a type exceeding maxtype will be
- * ignored. See documenation of struct nla_policy for more details.
+ * ignored, unless strict is set. See documenation of struct nla_policy for
+ * more details.
  *
  * Returns 0 on success or a negative error code.
  */
+static inline int nla_strict_validate_nested(const struct nlattr *start,
+					     int maxtype, bool strict,
+					     const struct nla_policy *policy)
+{
+	return nla_strict_validate(nla_data(start), nla_len(start), maxtype,
+				   strict, policy);
+}
+
+/**
+ * nla_validate_nested - Validate a stream of nested attributes
+ * @start: container attribute
+ * @maxtype: maximum attribute type to be expected
+ * @policy: validation policy
+ *
+ * See nla_strict_validate_nested(). Strict checking is not performed.
+ */
 static inline int nla_validate_nested(const struct nlattr *start, int maxtype,
 				      const struct nla_policy *policy)
 {

lib/nlattr.c +15 -9

index 2b36388eb3a9..86bc0662caee 100644
@@ -27,14 +27,17 @@ static const u16 nla_attr_minlen[NLA_TYPE_MAX+1] = {
 	[NLA_S64]	= sizeof(s64),
 };
 
-static int validate_nla(const struct nlattr *nla, int maxtype,
+static int validate_nla(const struct nlattr *nla, int maxtype, bool strict,
 			const struct nla_policy *policy)
 {
 	const struct nla_policy *pt;
 	int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
 
-	if (type <= 0 || type > maxtype)
+	if (type <= 0 || type > maxtype) {
+		if (strict)
+			return -EINVAL;
 		return 0;
+	}
 
 	pt = &policy[type];

@@ -107,33 +110,35 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 }
 
 /**
- * nla_validate - Validate a stream of attributes
+ * nla_strict_validate - Validate a stream of attributes
  * @head: head of attribute stream
  * @len: length of attribute stream
  * @maxtype: maximum attribute type to be expected
+ * @strict: whether to perform strict checking
  * @policy: validation policy
  *
  * Validates all attributes in the specified attribute stream against the
  * specified policy. Attributes with a type exceeding maxtype will be
- * ignored. See documenation of struct nla_policy for more details.
+ * ignored, unless strict is set. See documenation of struct nla_policy for
+ * more details.
  *
  * Returns 0 on success or a negative error code.
  */
-int nla_validate(const struct nlattr *head, int len, int maxtype,
-		 const struct nla_policy *policy)
+int nla_strict_validate(const struct nlattr *head, int len, int maxtype,
+			bool strict, const struct nla_policy *policy)
 {
 	const struct nlattr *nla;
 	int rem, err;
 
 	nla_for_each_attr(nla, head, len, rem) {
-		err = validate_nla(nla, maxtype, policy);
+		err = validate_nla(nla, maxtype, strict, policy);
 		if (err < 0)
 			return err;
 	}
 
 	return 0;
 }
-EXPORT_SYMBOL(nla_validate);
+EXPORT_SYMBOL(nla_strict_validate);
 
 /**
  * nla_policy_len - Determin the max. length of a policy

@@ -193,7 +198,8 @@ int nla_strict_parse(struct nlattr **tb, int maxtype, bool strict,
 
 		if (type > 0 && type <= maxtype) {
 			if (policy) {
-				err = validate_nla(nla, maxtype, policy);
+				err = validate_nla(nla, maxtype, strict,
+						   policy);
 				if (err < 0)
 					return err;
 			}

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #5 lore raw

RFC PATCH 5/9 rtnetlink: support strict attribute checking

In reply to Jiri Benc

Allow rtnl callbacks to support strict attribute checking. They can do so by passing new RTNL_F_STRICT flag to rtnl_register_flags. The semantics of the current rtnl_register and __rtnl_register functions were preserved in order not to change almost hundred of the call sites.

Signed-off-by Jiri Benc <[email protected]>

---
 include/net/rtnetlink.h | 27 ++++++++++++++++++++++----
 net/core/rtnetlink.c    | 51 +++++++++++++++++++++++++++++++------------------
 2 files changed, 55 insertions(+), 23 deletions(-)

include/net/rtnetlink.h +23 -4

index aff6ceb891a9..58bf1f5ad6a1 100644
@@ -8,13 +8,32 @@ typedef int (*rtnl_doit_func)(struct sk_buff *, struct nlmsghdr *);
 typedef int (*rtnl_dumpit_func)(struct sk_buff *, struct netlink_callback *);
 typedef u16 (*rtnl_calcit_func)(struct sk_buff *, struct nlmsghdr *);
 
-int __rtnl_register(int protocol, int msgtype,
-		    rtnl_doit_func, rtnl_dumpit_func, rtnl_calcit_func);
-void rtnl_register(int protocol, int msgtype,
-		   rtnl_doit_func, rtnl_dumpit_func, rtnl_calcit_func);
+#define RTNL_F_STRICT	1
+
+int __rtnl_register_flags(int protocol, int msgtype,
+			  rtnl_doit_func, rtnl_dumpit_func, rtnl_calcit_func,
+			  unsigned int flags);
+void rtnl_register_flags(int protocol, int msgtype,
+			 rtnl_doit_func, rtnl_dumpit_func, rtnl_calcit_func,
+			 unsigned int flags);
 int rtnl_unregister(int protocol, int msgtype);
 void rtnl_unregister_all(int protocol);
 
+static inline int __rtnl_register(int protocol, int msgtype,
+				  rtnl_doit_func doit, rtnl_dumpit_func dumpit,
+				  rtnl_calcit_func calcit)
+{
+	return __rtnl_register_flags(protocol, msgtype,
+				     doit, dumpit, calcit, 0);
+}
+
+static inline void rtnl_register(int protocol, int msgtype,
+				 rtnl_doit_func doit, rtnl_dumpit_func dumpit,
+				 rtnl_calcit_func calcit)
+{
+	rtnl_register_flags(protocol, msgtype, doit, dumpit, calcit, 0);
+}
+
 static inline int rtnl_msg_family(const struct nlmsghdr *nlh)
 {
 	if (nlmsg_len(nlh) >= sizeof(struct rtgenmsg))

net/core/rtnetlink.c +32 ~~-19~~

129 lines not on this page · raw message

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #6 lore raw

RFC PATCH 6/9 rtnetlink: add strict parameter to validate callbacks

In reply to Jiri Benc

This is in preparation for rtnetlink to be able to handle strict attribute checking.

The only rtnl_link_ops users that can have nested attributes (and thus need to know whether we're in strict validation mode or not) are 8021q and veth. veth will be handled in a separate patch. 8021q is trivial, it just passes the parameter through to nla_strict_validate_nested.

Signed-off-by Jiri Benc <[email protected]>

---
 drivers/net/bonding/bond_netlink.c |  3 ++-
 drivers/net/dummy.c                |  3 ++-
 drivers/net/geneve.c               |  3 ++-
 drivers/net/ifb.c                  |  3 ++-
 drivers/net/ipvlan/ipvlan_main.c   |  3 ++-
 drivers/net/macvlan.c              |  3 ++-
 drivers/net/nlmon.c                |  3 ++-
 drivers/net/team/team.c            |  3 ++-
 drivers/net/tun.c                  |  3 ++-
 drivers/net/veth.c                 |  5 +++--
 drivers/net/vrf.c                  |  3 ++-
 drivers/net/vxlan.c                |  3 ++-
 include/net/rtnetlink.h            |  6 ++++--
 net/8021q/vlan_netlink.c           | 12 +++++++-----
 net/bridge/br_netlink.c            |  3 ++-
 net/core/rtnetlink.c               |  5 +++--
 net/ieee802154/6lowpan/core.c      |  3 ++-
 net/ipv4/ip_gre.c                  |  8 +++++---
 net/ipv4/ip_vti.c                  |  3 ++-
 net/ipv6/ip6_gre.c                 |  8 +++++---
 net/ipv6/ip6_tunnel.c              |  3 ++-
 net/ipv6/ip6_vti.c                 |  3 ++-
 net/ipv6/sit.c                     |  3 ++-
 23 files changed, 61 insertions(+), 34 deletions(-)

drivers/net/bonding/bond_netlink.c +2 -1

index db760e84119f..71c7bfeb96f3 100644
@@ -118,7 +118,8 @@ static const struct nla_policy bond_slave_policy[IFLA_BOND_SLAVE_MAX + 1] = {
 	[IFLA_BOND_SLAVE_QUEUE_ID]	= { .type = NLA_U16 },
 };
 
-static int bond_validate(struct nlattr *tb[], struct nlattr *data[])
+static int bond_validate(struct nlattr *tb[], struct nlattr *data[],
+			 bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

drivers/net/dummy.c +2 -1

index 815eb94990f5..45601fb973e3 100644
@@ -152,7 +152,8 @@ static void dummy_setup(struct net_device *dev)
 	eth_hw_addr_random(dev);
 }
 
-static int dummy_validate(struct nlattr *tb[], struct nlattr *data[])
+static int dummy_validate(struct nlattr *tb[], struct nlattr *data[],
+			  bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

drivers/net/geneve.c +2 -1

index 8f5c02eed47d..1b3e7bc649c7 100644
@@ -765,7 +765,8 @@ static const struct nla_policy geneve_policy[IFLA_GENEVE_MAX + 1] = {
 	[IFLA_GENEVE_COLLECT_METADATA]	= { .type = NLA_FLAG },
 };
 
-static int geneve_validate(struct nlattr *tb[], struct nlattr *data[])
+static int geneve_validate(struct nlattr *tb[], struct nlattr *data[],
+			   bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

drivers/net/ifb.c +2 -1

index cc56fac3c3f8..ae2636e9cd7a 100644
@@ -276,7 +276,8 @@ static int ifb_open(struct net_device *dev)
 	return 0;
 }
 
-static int ifb_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ifb_validate(struct nlattr *tb[], struct nlattr *data[],
+			bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

drivers/net/ipvlan/ipvlan_main.c +2 -1

index a9268db4e349..51e9353d0459 100644
@@ -404,7 +404,8 @@ static size_t ipvlan_nl_getsize(const struct net_device *dev)
 		);
 }
 
-static int ipvlan_nl_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ipvlan_nl_validate(struct nlattr *tb[], struct nlattr *data[],
+			      bool strict)
 {
 	if (data && data[IFLA_IPVLAN_MODE]) {
 		u16 mode = nla_get_u16(data[IFLA_IPVLAN_MODE]);

drivers/net/macvlan.c +2 -1

index 86f6c6292c27..60b3fb556144 100644
@@ -1121,7 +1121,8 @@ static void macvlan_port_destroy(struct net_device *dev)
 	kfree_rcu(port, rcu);
 }
 
-static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[])
+static int macvlan_validate(struct nlattr *tb[], struct nlattr *data[],
+			    bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

drivers/net/nlmon.c +2 -1

index 7b7c70e2341e..3dd893cd0291 100644
@@ -147,7 +147,8 @@ static void nlmon_setup(struct net_device *dev)
 	dev->mtu = NLMSG_GOODSIZE;
 }
 
-static int nlmon_validate(struct nlattr *tb[], struct nlattr *data[])
+static int nlmon_validate(struct nlattr *tb[], struct nlattr *data[],
+			  bool strict)
 {
 	if (tb[IFLA_ADDRESS])
 		return -EINVAL;

drivers/net/team/team.c +2 -1

index 651d35ea22c5..e36e14db6f48 100644
@@ -2086,7 +2086,8 @@ static int team_newlink(struct net *src_net, struct net_device *dev,
 	return register_netdevice(dev);
 }
 
-static int team_validate(struct nlattr *tb[], struct nlattr *data[])
+static int team_validate(struct nlattr *tb[], struct nlattr *data[],
+			 bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

drivers/net/tun.c +2 -1

index b1878faea397..610bafb3b1c2 100644
@@ -1468,7 +1468,8 @@ static void tun_setup(struct net_device *dev)
 /* Trivial set of netlink ops to allow deleting tun or tap
  * device with netlink.
  */
-static int tun_validate(struct nlattr *tb[], struct nlattr *data[])
+static int tun_validate(struct nlattr *tb[], struct nlattr *data[],
+			bool strict)
 {
 	return -EINVAL;
 }

drivers/net/veth.c +3 -2

index 0ef4a5ad5557..af0bf39147ba 100644
@@ -327,7 +327,8 @@ static void veth_setup(struct net_device *dev)
  * netlink interface
  */
 
-static int veth_validate(struct nlattr *tb[], struct nlattr *data[])
+static int veth_validate(struct nlattr *tb[], struct nlattr *data[],
+			 bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

@@ -370,7 +371,7 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 		if (err < 0)
 			return err;
 
-		err = veth_validate(peer_tb, NULL);
+		err = veth_validate(peer_tb, NULL, false);
 		if (err < 0)
 			return err;

drivers/net/vrf.c +2 -1

index 92fa3e1ea65c..a3f9b4703a93 100644
@@ -887,7 +887,8 @@ static void vrf_setup(struct net_device *dev)
 	dev->features |= NETIF_F_NETNS_LOCAL;
 }
 
-static int vrf_validate(struct nlattr *tb[], struct nlattr *data[])
+static int vrf_validate(struct nlattr *tb[], struct nlattr *data[],
+			bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

drivers/net/vxlan.c +2 -1

index ce704df7681b..764eaac91c97 100644
@@ -2478,7 +2478,8 @@ static const struct nla_policy vxlan_policy[IFLA_VXLAN_MAX + 1] = {
 	[IFLA_VXLAN_REMCSUM_NOPARTIAL]	= { .type = NLA_FLAG },
 };
 
-static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[])
+static int vxlan_validate(struct nlattr *tb[], struct nlattr *data[],
+			  bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN) {

include/net/rtnetlink.h +4 -2

index 58bf1f5ad6a1..9f730f2395de 100644
@@ -78,7 +78,8 @@ struct rtnl_link_ops {
 	int			maxtype;
 	const struct nla_policy	*policy;
 	int			(*validate)(struct nlattr *tb[],
-					    struct nlattr *data[]);
+					    struct nlattr *data[],
+					    bool strict);
 
 	int			(*newlink)(struct net *src_net,
 					   struct net_device *dev,

@@ -103,7 +104,8 @@ struct rtnl_link_ops {
 	int			slave_maxtype;
 	const struct nla_policy	*slave_policy;
 	int			(*slave_validate)(struct nlattr *tb[],
-						  struct nlattr *data[]);
+						  struct nlattr *data[],
+						  bool strict);
 	int			(*slave_changelink)(struct net_device *dev,
 						    struct net_device *slave_dev,
 						    struct nlattr *tb[],

net/8021q/vlan_netlink.c +7 -5

index c92b52f37d38..ba69c408312d 100644
@@ -31,14 +31,16 @@ static const struct nla_policy vlan_map_policy[IFLA_VLAN_QOS_MAX + 1] = {
 };
 
 
-static inline int vlan_validate_qos_map(struct nlattr *attr)
+static inline int vlan_validate_qos_map(struct nlattr *attr, bool strict)
 {
 	if (!attr)
 		return 0;
-	return nla_validate_nested(attr, IFLA_VLAN_QOS_MAX, vlan_map_policy);
+	return nla_strict_validate_nested(attr, IFLA_VLAN_QOS_MAX, strict,
+					  vlan_map_policy);
 }
 
-static int vlan_validate(struct nlattr *tb[], struct nlattr *data[])
+static int vlan_validate(struct nlattr *tb[], struct nlattr *data[],
+			 bool strict)
 {
 	struct ifla_vlan_flags *flags;
 	u16 id;

@@ -77,10 +79,10 @@ static int vlan_validate(struct nlattr *tb[], struct nlattr *data[])
 			return -EINVAL;
 	}
 
-	err = vlan_validate_qos_map(data[IFLA_VLAN_INGRESS_QOS]);
+	err = vlan_validate_qos_map(data[IFLA_VLAN_INGRESS_QOS], strict);
 	if (err < 0)
 		return err;
-	err = vlan_validate_qos_map(data[IFLA_VLAN_EGRESS_QOS]);
+	err = vlan_validate_qos_map(data[IFLA_VLAN_EGRESS_QOS], strict);
 	if (err < 0)
 		return err;
 	return 0;

net/bridge/br_netlink.c +2 -1

index 94b4de8c4646..f144344a3f38 100644
@@ -746,7 +746,8 @@ int br_dellink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags)
 
 	return err;
 }
-static int br_validate(struct nlattr *tb[], struct nlattr *data[])
+static int br_validate(struct nlattr *tb[], struct nlattr *data[],
+		       bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)

net/core/rtnetlink.c +3 -2

index dedc539b960c..205b7acbd6bf 100644
@@ -2197,7 +2197,7 @@ replay:
 				data = attr;
 			}
 			if (ops->validate) {
-				err = ops->validate(tb, data);
+				err = ops->validate(tb, data, false);
 				if (err < 0)
 					return err;
 			}

@@ -2215,7 +2215,8 @@ replay:
 				slave_data = slave_attr;
 			}
 			if (m_ops->slave_validate) {
-				err = m_ops->slave_validate(tb, slave_data);
+				err = m_ops->slave_validate(tb, slave_data,
+							    false);
 				if (err < 0)
 					return err;
 			}

net/ieee802154/6lowpan/core.c +2 -1

index 20c49c724ba0..145c1d4256e1 100644
@@ -112,7 +112,8 @@ static void lowpan_setup(struct net_device *ldev)
 	ldev->features		|= NETIF_F_NETNS_LOCAL;
 }
 
-static int lowpan_validate(struct nlattr *tb[], struct nlattr *data[])
+static int lowpan_validate(struct nlattr *tb[], struct nlattr *data[],
+			   bool strict)
 {
 	if (tb[IFLA_ADDRESS]) {
 		if (nla_len(tb[IFLA_ADDRESS]) != IEEE802154_ADDR_LEN)

net/ipv4/ip_gre.c +5 -3

index bd0679d90519..dd2b9801ed7a 100644
@@ -881,7 +881,8 @@ static struct pernet_operations ipgre_net_ops = {
 	.size = sizeof(struct ip_tunnel_net),
 };
 
-static int ipgre_tunnel_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ipgre_tunnel_validate(struct nlattr *tb[], struct nlattr *data[],
+				 bool strict)
 {
 	__be16 flags;

@@ -899,7 +900,8 @@ static int ipgre_tunnel_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
-static int ipgre_tap_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ipgre_tap_validate(struct nlattr *tb[], struct nlattr *data[],
+			      bool strict)
 {
 	__be32 daddr;

@@ -920,7 +922,7 @@ static int ipgre_tap_validate(struct nlattr *tb[], struct nlattr *data[])
 	}
 
 out:
-	return ipgre_tunnel_validate(tb, data);
+	return ipgre_tunnel_validate(tb, data, strict);
 }
 
 static void ipgre_netlink_parms(struct net_device *dev,

net/ipv4/ip_vti.c +2 -1

index 4d8f0b698777..1e48c1fd74db 100644
@@ -435,7 +435,8 @@ static struct pernet_operations vti_net_ops = {
 	.size = sizeof(struct ip_tunnel_net),
 };
 
-static int vti_tunnel_validate(struct nlattr *tb[], struct nlattr *data[])
+static int vti_tunnel_validate(struct nlattr *tb[], struct nlattr *data[],
+			       bool strict)
 {
 	return 0;
 }

net/ipv6/ip6_gre.c +5 -3

index 3c7b9310b33f..58281ec6cc27 100644
@@ -1392,7 +1392,8 @@ static struct pernet_operations ip6gre_net_ops = {
 	.size = sizeof(struct ip6gre_net),
 };
 
-static int ip6gre_tunnel_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ip6gre_tunnel_validate(struct nlattr *tb[], struct nlattr *data[],
+				  bool strict)
 {
 	__be16 flags;

@@ -1410,7 +1411,8 @@ static int ip6gre_tunnel_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
-static int ip6gre_tap_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ip6gre_tap_validate(struct nlattr *tb[], struct nlattr *data[],
+			       bool strict)
 {
 	struct in6_addr daddr;

@@ -1431,7 +1433,7 @@ static int ip6gre_tap_validate(struct nlattr *tb[], struct nlattr *data[])
 	}
 
 out:
-	return ip6gre_tunnel_validate(tb, data);
+	return ip6gre_tunnel_validate(tb, data, strict);
 }

net/ipv6/ip6_tunnel.c +2 -1

index eabffbb89795..3fb0fcc50b47 100644
@@ -1683,7 +1683,8 @@ static int __net_init ip6_fb_tnl_dev_init(struct net_device *dev)
 	return 0;
 }
 
-static int ip6_tnl_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ip6_tnl_validate(struct nlattr *tb[], struct nlattr *data[],
+			    bool strict)
 {
 	u8 proto;

net/ipv6/ip6_vti.c +2 -1

index 0a8610b33d79..abde61ddf9fa 100644
@@ -899,7 +899,8 @@ static int __net_init vti6_fb_tnl_dev_init(struct net_device *dev)
 	return 0;
 }
 
-static int vti6_validate(struct nlattr *tb[], struct nlattr *data[])
+static int vti6_validate(struct nlattr *tb[], struct nlattr *data[],
+			 bool strict)
 {
 	return 0;
 }

net/ipv6/sit.c +2 -1

index 94428fd85b2f..10b858a80183 100644
@@ -1424,7 +1424,8 @@ static int __net_init ipip6_fb_tunnel_init(struct net_device *dev)
 	return 0;
 }
 
-static int ipip6_validate(struct nlattr *tb[], struct nlattr *data[])
+static int ipip6_validate(struct nlattr *tb[], struct nlattr *data[],
+			  bool strict)
 {
 	u8 proto;

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #7 lore raw

RFC PATCH 7/9 rtnetlink: add strict parameter to validate_link_af

In reply to Jiri Benc

This is in preparation for rtnetlink to be able to handle strict attribute checking.

Nothing complicated here, just pass the parameter through.

Signed-off-by Jiri Benc <[email protected]>

---
 include/net/rtnetlink.h | 3 ++-
 net/core/rtnetlink.c    | 3 ++-
 net/ipv4/devinet.c      | 6 ++++--
 net/ipv6/addrconf.c     | 6 ++++--
 4 files changed, 12 insertions(+), 6 deletions(-)

include/net/rtnetlink.h +2 -1

index 9f730f2395de..71c49f5af9a0 100644
@@ -148,7 +148,8 @@ struct rtnl_af_ops {
 	size_t			(*get_link_af_size)(const struct net_device *dev);
 
 	int			(*validate_link_af)(const struct net_device *dev,
-						    const struct nlattr *attr);
+						    const struct nlattr *attr,
+						    bool strict);
 	int			(*set_link_af)(struct net_device *dev,
 					       const struct nlattr *attr);
 };

net/core/rtnetlink.c +2 -1

index 205b7acbd6bf..9d08ad6ee5c3 100644
@@ -1491,7 +1491,8 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
 				return -EOPNOTSUPP;
 
 			if (af_ops->validate_link_af) {
-				err = af_ops->validate_link_af(dev, af);
+				err = af_ops->validate_link_af(dev, af,
+							       false);
 				if (err < 0)
 					return err;
 			}

net/ipv4/devinet.c +4 -2

index 735008472844..9ea6bc6683ec 100644
@@ -1679,7 +1679,8 @@ static const struct nla_policy inet_af_policy[IFLA_INET_MAX+1] = {
 };
 
 static int inet_validate_link_af(const struct net_device *dev,
-				 const struct nlattr *nla)
+				 const struct nlattr *nla,
+				 bool strict)
 {
 	struct nlattr *a, *tb[IFLA_INET_MAX+1];
 	int err, rem;

@@ -1687,7 +1688,8 @@ static int inet_validate_link_af(const struct net_device *dev,
 	if (dev && !__in_dev_get_rtnl(dev))
 		return -EAFNOSUPPORT;
 
-	err = nla_parse_nested(tb, IFLA_INET_MAX, nla, inet_af_policy);
+	err = nla_strict_parse_nested(tb, IFLA_INET_MAX, strict, nla,
+				      inet_af_policy);
 	if (err < 0)
 		return err;

net/ipv6/addrconf.c +4 -2

index f0326aae7a02..b29f117cde72 100644
@@ -4876,14 +4876,16 @@ static const struct nla_policy inet6_af_policy[IFLA_INET6_MAX + 1] = {
 };
 
 static int inet6_validate_link_af(const struct net_device *dev,
-				  const struct nlattr *nla)
+				  const struct nlattr *nla,
+				  bool strict)
 {
 	struct nlattr *tb[IFLA_INET6_MAX + 1];
 
 	if (dev && !__in6_dev_get(dev))
 		return -EAFNOSUPPORT;
 
-	return nla_parse_nested(tb, IFLA_INET6_MAX, nla, inet6_af_policy);
+	return nla_strict_parse_nested(tb, IFLA_INET6_MAX, strict, nla,
+				       inet6_af_policy);
 }
 
 static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #8 lore raw

RFC PATCH 8/9 rtnetlink: support strict checking for newlink, setlink and dellink

In reply to Jiri Benc

Signed-off-by Jiri Benc <[email protected]>

---
 net/core/rtnetlink.c | 84 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 33 deletions(-)

net/core/rtnetlink.c +51 ~~-33~~

index 9d08ad6ee5c3..1af929e468cf 100644
@@ -1465,7 +1465,8 @@ struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
 }
 EXPORT_SYMBOL(rtnl_link_get_net);
 
-static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
+static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[],
+			    bool strict)
 {
 	if (dev) {
 		if (tb[IFLA_ADDRESS] &&

@@ -1492,7 +1493,7 @@ static int validate_linkmsg(struct net_device *dev, struct nlattr *tb[])
 
 			if (af_ops->validate_link_af) {
 				err = af_ops->validate_link_af(dev, af,
-							       false);
+							       strict);
 				if (err < 0)
 					return err;
 			}

@@ -1637,7 +1638,8 @@ static int do_set_master(struct net_device *dev, int ifindex)
 #define DO_SETLINK_NOTIFY	0x03
 static int do_setlink(const struct sk_buff *skb,
 		      struct net_device *dev, struct ifinfomsg *ifm,
-		      struct nlattr **tb, char *ifname, int status)
+		      struct nlattr **tb, char *ifname, int status,
+		      bool strict)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	int err;

@@ -1799,8 +1801,9 @@ static int do_setlink(const struct sk_buff *skb,
 				err = -EINVAL;
 				goto errout;
 			}
-			err = nla_parse_nested(vfinfo, IFLA_VF_MAX, attr,
-					       ifla_vf_policy);
+			err = nla_strict_parse_nested(vfinfo, IFLA_VF_MAX,
+						      strict, attr,
+						      ifla_vf_policy);
 			if (err < 0)
 				goto errout;
 			err = do_setvfinfo(dev, vfinfo);

@@ -1827,8 +1830,9 @@ static int do_setlink(const struct sk_buff *skb,
 				err = -EINVAL;
 				goto errout;
 			}
-			err = nla_parse_nested(port, IFLA_PORT_MAX, attr,
-					       ifla_port_policy);
+			err = nla_strict_parse_nested(port, IFLA_PORT_MAX,
+						      strict, attr,
+						      ifla_port_policy);
 			if (err < 0)
 				goto errout;
 			if (!port[IFLA_PORT_VF]) {

@@ -1847,8 +1851,9 @@ static int do_setlink(const struct sk_buff *skb,
 	if (tb[IFLA_PORT_SELF]) {
 		struct nlattr *port[IFLA_PORT_MAX+1];
 
-		err = nla_parse_nested(port, IFLA_PORT_MAX,
-			tb[IFLA_PORT_SELF], ifla_port_policy);
+		err = nla_strict_parse_nested(port, IFLA_PORT_MAX, strict,
+					      tb[IFLA_PORT_SELF],
+					      ifla_port_policy);
 		if (err < 0)
 			goto errout;

@@ -1908,8 +1913,10 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh)
 	int err;
 	struct nlattr *tb[IFLA_MAX+1];
 	char ifname[IFNAMSIZ];
+	bool strict = nlh->nlmsg_flags & NLM_F_STRICT;
 
-	err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy);
+	err = nlmsg_strict_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, strict,
+				 ifla_policy);
 	if (err < 0)
 		goto errout;

@@ -1932,11 +1939,11 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh)
 		goto errout;
 	}
 
-	err = validate_linkmsg(dev, tb);
+	err = validate_linkmsg(dev, tb, strict);
 	if (err < 0)
 		goto errout;
 
-	err = do_setlink(skb, dev, ifm, tb, ifname, 0);
+	err = do_setlink(skb, dev, ifm, tb, ifname, 0, strict);
 errout:
 	return err;
 }

@@ -2000,9 +2007,11 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct ifinfomsg *ifm;
 	char ifname[IFNAMSIZ];
 	struct nlattr *tb[IFLA_MAX+1];
+	bool strict = nlh->nlmsg_flags & NLM_F_STRICT;
 	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy);
+	err = nlmsg_strict_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, strict,
+				 ifla_policy);
 	if (err < 0)
 		return err;

@@ -2102,14 +2111,14 @@ EXPORT_SYMBOL(rtnl_create_link);
 static int rtnl_group_changelink(const struct sk_buff *skb,
 		struct net *net, int group,
 		struct ifinfomsg *ifm,
-		struct nlattr **tb)
+		struct nlattr **tb, bool strict)
 {
 	struct net_device *dev, *aux;
 	int err;
 
 	for_each_netdev_safe(net, dev, aux) {
 		if (dev->group == group) {
-			err = do_setlink(skb, dev, ifm, tb, NULL, 0);
+			err = do_setlink(skb, dev, ifm, tb, NULL, 0, strict);
 			if (err < 0)
 				return err;
 		}

@@ -2131,12 +2140,14 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct nlattr *tb[IFLA_MAX+1];
 	struct nlattr *linkinfo[IFLA_INFO_MAX+1];
 	unsigned char name_assign_type = NET_NAME_USER;
+	bool strict = nlh->nlmsg_flags & NLM_F_STRICT;
 	int err;
 
 #ifdef CONFIG_MODULES
 replay:
 #endif
-	err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy);
+	err = nlmsg_strict_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, strict,
+				 ifla_policy);
 	if (err < 0)
 		return err;

@@ -2161,13 +2172,14 @@ replay:
 			m_ops = master_dev->rtnl_link_ops;
 	}
 
-	err = validate_linkmsg(dev, tb);
+	err = validate_linkmsg(dev, tb, strict);
 	if (err < 0)
 		return err;
 
 	if (tb[IFLA_LINKINFO]) {
-		err = nla_parse_nested(linkinfo, IFLA_INFO_MAX,
-				       tb[IFLA_LINKINFO], ifla_info_policy);
+		err = nla_strict_parse_nested(linkinfo, IFLA_INFO_MAX,
+					      strict, tb[IFLA_LINKINFO],
+					      ifla_info_policy);
 		if (err < 0)
 			return err;
 	} else

@@ -2190,15 +2202,16 @@ replay:
 
 		if (ops) {
 			if (ops->maxtype && linkinfo[IFLA_INFO_DATA]) {
-				err = nla_parse_nested(attr, ops->maxtype,
-						       linkinfo[IFLA_INFO_DATA],
-						       ops->policy);
+				err = nla_strict_parse_nested(attr, ops->maxtype,
+							      strict,
+							      linkinfo[IFLA_INFO_DATA],
+							      ops->policy);
 				if (err < 0)
 					return err;
 				data = attr;
 			}
 			if (ops->validate) {
-				err = ops->validate(tb, data, false);
+				err = ops->validate(tb, data, strict);
 				if (err < 0)
 					return err;
 			}

@@ -2207,17 +2220,18 @@ replay:
 		if (m_ops) {
 			if (m_ops->slave_maxtype &&
 			    linkinfo[IFLA_INFO_SLAVE_DATA]) {
-				err = nla_parse_nested(slave_attr,
-						       m_ops->slave_maxtype,
-						       linkinfo[IFLA_INFO_SLAVE_DATA],
-						       m_ops->slave_policy);
+				err = nla_strict_parse_nested(slave_attr,
+							      m_ops->slave_maxtype,
+							      strict,
+							      linkinfo[IFLA_INFO_SLAVE_DATA],
+							      m_ops->slave_policy);
 				if (err < 0)
 					return err;
 				slave_data = slave_attr;
 			}
 			if (m_ops->slave_validate) {
 				err = m_ops->slave_validate(tb, slave_data,
-							    false);
+							    strict);
 				if (err < 0)
 					return err;
 			}

@@ -2253,14 +2267,15 @@ replay:
 				status |= DO_SETLINK_NOTIFY;
 			}
 
-			return do_setlink(skb, dev, ifm, tb, ifname, status);
+			return do_setlink(skb, dev, ifm, tb, ifname, status,
+					  strict);
 		}
 
 		if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
 			if (ifm->ifi_index == 0 && tb[IFLA_GROUP])
 				return rtnl_group_changelink(skb, net,
 						nla_get_u32(tb[IFLA_GROUP]),
-						ifm, tb);
+						ifm, tb, strict);
 			return -ENODEV;
 		}

@@ -3443,9 +3458,12 @@ void __init rtnetlink_init(void)
 
 	rtnl_register(PF_UNSPEC, RTM_GETLINK, rtnl_getlink,
 		      rtnl_dump_ifinfo, rtnl_calcit);
-	rtnl_register(PF_UNSPEC, RTM_SETLINK, rtnl_setlink, NULL, NULL);
-	rtnl_register(PF_UNSPEC, RTM_NEWLINK, rtnl_newlink, NULL, NULL);
-	rtnl_register(PF_UNSPEC, RTM_DELLINK, rtnl_dellink, NULL, NULL);
+	rtnl_register_flags(PF_UNSPEC, RTM_SETLINK, rtnl_setlink, NULL, NULL,
+			    RTNL_F_STRICT);
+	rtnl_register_flags(PF_UNSPEC, RTM_NEWLINK, rtnl_newlink, NULL, NULL,
+			    RTNL_F_STRICT);
+	rtnl_register_flags(PF_UNSPEC, RTM_DELLINK, rtnl_dellink, NULL, NULL,
+			    RTNL_F_STRICT);
 
 	rtnl_register(PF_UNSPEC, RTM_GETADDR, NULL, rtnl_dump_all, NULL);
 	rtnl_register(PF_UNSPEC, RTM_GETROUTE, NULL, rtnl_dump_all, NULL);

Signature · 1 line

1.8.3.1

Jiri Benc

author

2015-10-15 16:39 UTC #9 lore raw

RFC PATCH 9/9 veth: validate nested attributes

In reply to Jiri Benc

For strict attribute checking, it's necessary to validate nested attributes in the validate rtnl_link_ops callback.

Signed-off-by Jiri Benc <[email protected]>

---
 drivers/net/veth.c      | 8 ++++++++
 include/net/rtnetlink.h | 1 +
 net/core/rtnetlink.c    | 6 ++++++
 3 files changed, 15 insertions(+)

drivers/net/veth.c +8 -0

index af0bf39147ba..86a68bd191b6 100644
@@ -340,6 +340,14 @@ static int veth_validate(struct nlattr *tb[], struct nlattr *data[],
 		if (!is_valid_veth_mtu(nla_get_u32(tb[IFLA_MTU])))
 			return -EINVAL;
 	}
+	if (data != NULL && data[VETH_INFO_PEER] != NULL) {
+		struct nlattr *nla_peer = data[VETH_INFO_PEER];
+
+		return rtnl_nla_validate_ifla(
+				nla_data(nla_peer) + sizeof(struct ifinfomsg),
+				nla_len(nla_peer) - sizeof(struct ifinfomsg),
+				strict);
+	}
 	return 0;
 }

include/net/rtnetlink.h +1 -0

index 71c49f5af9a0..be75457e4cc5 100644
@@ -168,6 +168,7 @@ int rtnl_delete_link(struct net_device *dev);
 int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm);
 
 int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len);
+int rtnl_nla_validate_ifla(const struct nlattr *head, int len, bool strict);
 
 #define MODULE_ALIAS_RTNL_LINK(kind) MODULE_ALIAS("rtnl-link-" kind)

net/core/rtnetlink.c +6 -0

index 1af929e468cf..89e7f0a6da44 100644
@@ -1449,6 +1449,12 @@ int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len)
 }
 EXPORT_SYMBOL(rtnl_nla_parse_ifla);
 
+int rtnl_nla_validate_ifla(const struct nlattr *head, int len, bool strict)
+{
+	return nla_strict_validate(head, len, IFLA_MAX, strict, ifla_policy);
+}
+EXPORT_SYMBOL(rtnl_nla_validate_ifla);
+
 struct net *rtnl_link_get_net(struct net *src_net, struct nlattr *tb[])
 {
 	struct net *net;

Signature · 1 line

1.8.3.1

Hannes Frederic Sowa

2015-10-15 22:07 UTC #10 lore raw

In reply to Jiri Benc

Quoted 5 lines · Jiri Benc

Before using this new facility, the application should check whether it is supported by the kernel. This is done by sending a NLMSG_NOOP message with NLM_F_REQUEST | NLM_F_STRICT | NLM_F_ACK flags set. If the returned message has NLM_F_STRICT set, the kernel does support NLM_F_STRICT flag; otherwise, the flag should not be used.

Do you plan to update rfc3549, too? :}

Bye,
Hannes

Jiri Benc

author

2015-10-16 07:49 UTC #11 lore raw

Quoted 1 line · Hannes Frederic Sowa

Do you plan to update rfc3549, too? :}

No.

While I think we need to have a detailed documentation on netlink, I believe it should reside in Documentation/ inside the kernel tree to be easily updated, not in an outside document collection with much more heavyweight processes around getting something updated.

I would offer taking the RFC 3549 into the kernel tree and extending it but my understanding of the license is that I'm not allowed to do that (as I would modify the "document itself").

Jiri

Signature · 1 line

Jiri Benc

David Miller

maintainer

2015-10-16 07:52 UTC #12 lore raw

From: Jiri Benc <[email protected]>
Date: Fri, 16 Oct 2015 09:49:51 +0200

Quoted 9 lines · Hannes Frederic Sowa

Quoted 1 line
Do you plan to update rfc3549, too? :}
No.
While I think we need to have a detailed documentation on netlink, I believe it should reside in Documentation/ inside the kernel tree to be easily updated, not in an outside document collection with much more heavyweight processes around getting something updated.

I think his point is that you'll be making changes to an RFC specified protocol.

But I think we need a better solution to this.

Jiri Benc

author

2015-10-16 08:08 UTC #13 lore raw

Quoted 2 lines · David Miller

I think his point is that you'll be making changes to an RFC specified protocol.

The RFC is only informational, it has never became an Internet standard (under the RFC terms). And no other OS picked it up. Keeping kernel uAPI documentation in IETF database sounds rather inefficient.

Quoted 1 line

But I think we need a better solution to this.

I'm not opposed to a better solution. I just don't see any that's not a gross hack. Suggestions are welcome.

Jiri

Signature · 1 line

Jiri Benc

Hannes Frederic Sowa

2015-10-16 09:40 UTC #14 lore raw

Hi,

Quoted 7 lines · Jiri Benc

Quoted 2 lines · David Miller
I think his point is that you'll be making changes to an RFC specified protocol.
The RFC is only informational, it has never became an Internet standard (under the RFC terms). And no other OS picked it up. Keeping kernel uAPI documentation in IETF database sounds rather inefficient.

I would suggest you simply add a description to the rfc via the errata process here: <https://www.rfc-editor.org/errata_report.php>

The original cover letter should be enough to post there. I am also not sure if people do really implement this spec, at least they have a pointer to it, then. AFAIK the IESG will look into that then and even non-approved erratas will be listed on the rfc page.

Bye,
Hannes

Jiri Benc

author

2015-10-16 10:00 UTC #15 lore raw

Quoted 7 lines · Hannes Frederic Sowa

I would suggest you simply add a description to the rfc via the errata process here: <https://www.rfc-editor.org/errata_report.php>
The original cover letter should be enough to post there. I am also not sure if people do really implement this spec, at least they have a pointer to it, then. AFAIK the IESG will look into that then and even non-approved erratas will be listed on the rfc page.

Okay, we may do that. But let's postpone this discussion after we decide what route we'll go. We'll definitely need some form of documentation but it's preliminary to discuss what exact form when we haven't decided whether we'll implement something like this at all.

Jiri

Signature · 1 line

Jiri Benc

David Miller

maintainer

2015-10-16 06:33 UTC #16 lore raw

In reply to Jiri Benc

From: Jiri Benc <[email protected]>
Date: Thu, 15 Oct 2015 18:39:05 +0200

Quoted 6 lines

When sending a netlink request (NLM_F_REQUEST), any unknown attributes are ignored. This behavior is problematic in some situations. For example if the user asks for a particular config, the request finishes successfully, yet the configuraton applied does not work because some of the attributes are not supported by the kernel (e.g. because it's older than the user space tool).

Although we are probably stuck with this, it was probably a bad idea to have this behavior as the default.

Better would have been to always error on unrecognized attributes, and in the ACK give some indication of which attribute was problematic.

But anyhow we are stuck with what we have. However, I will say I am disappointed that it is so hard to simply detect that lwtunnel support is present, which as I understand is what this patch set is trying to accomplish.

And this is quite an intrusive patch series, and therefore not suitable for -stable backports. And that's exactly where you actually are going to need these changes right? Older kernels that lack lwtunnel support.

Jiri Benc

author

2015-10-16 07:39 UTC #17 lore raw

Quoted 2 lines · David Miller

Although we are probably stuck with this, it was probably a bad idea to have this behavior as the default.

I agree. But we have what we have.

Quoted 2 lines

Better would have been to always error on unrecognized attributes, and in the ACK give some indication of which attribute was problematic.

Should I try to extend the patches to return such information back to the user? It would mean more intrusiveness, as there's no space in ack to store such information (it would have been better to use attributes even in the error responses instead of a fixed structure...).

Quoted 4 lines

But anyhow we are stuck with what we have. However, I will say I am disappointed that it is so hard to simply detect that lwtunnel support is present, which as I understand is what this patch set is trying to accomplish.

Yes, main motivation is lwtunnel support. However, I'd say this is useful outside of it, too.

Quoted 4 lines

And this is quite an intrusive patch series, and therefore not suitable for -stable backports. And that's exactly where you actually are going to need these changes right? Older kernels that lack lwtunnel support.

I'm targeting net-next only and don't intend to bring this to older kernels. The patchset is designed in the way that it's possible to detect that the kernel does not support strict attribute checking. When this is detected, the tools will just assume that lwtunnel support is not there. That's completely okay, it will just mean that the 4.3 kernel will be treated as not having lwtunnel support; everything will work correctly, including openvswitch (which will use its compat code).

Jiri

Signature · 1 line

Jiri Benc

David Miller

maintainer

2015-10-16 07:50 UTC #18 lore raw

From: Jiri Benc <[email protected]>
Date: Fri, 16 Oct 2015 09:39:45 +0200

Quoted 5 lines

I'm targeting net-next only and don't intend to bring this to older kernels. The patchset is designed in the way that it's possible to detect that the kernel does not support strict attribute checking. When this is detected, the tools will just assume that lwtunnel support is not there.

No, it's definitely not OK, because lwtunnel support exists in Linus's tree.

And tools should be able to work on all kernels where lwtunnel support is available.

Jiri Benc

author

2015-10-16 08:02 UTC #19 lore raw

Quoted 5 lines · David Miller

No, it's definitely not OK, because lwtunnel support exists in Linus's tree.
And tools should be able to work on all kernels where lwtunnel support is available.

You can consider the lwtunnels feature as not finished in the current Linus's tree. It works, it won't change (thus anything using it in its current form will continue to work in all the future kernels), but mainstream tools won't make use of it until a kernel version later which will get some additional support.

I don't think it's much of a problem and I don't think it is the first time this would happen.

I'm afraid I don't have any solution that could do better.

Jiri

Signature · 1 line

Jiri Benc

Thomas Graf

2015-10-16 08:09 UTC #20 lore raw

Quoted 13 lines · Jiri Benc

Quoted 5 lines · David Miller
No, it's definitely not OK, because lwtunnel support exists in Linus's tree.
And tools should be able to work on all kernels where lwtunnel support is available.
You can consider the lwtunnels feature as not finished in the current Linus's tree. It works, it won't change (thus anything using it in its current form will continue to work in all the future kernels), but mainstream tools won't make use of it until a kernel version later which will get some additional support.

Quoted 4 lines

I don't think it's much of a problem and I don't think it is the first time this would happen.
I'm afraid I don't have any solution that could do better.

Maybe we should be more precise here. The detection mechanism is only for tools which want to know whether lwt or another feature is supported without going through the pain of creating an object, checking its attributes and then deleting the object again if no full support was detected. The existing flow of configuration through iproute2 will be supported just like for any other new feature that we have introduced.

Therefore, I consider this a future optimization.

This is very much related to the offload problematic we had or still have of "only program in HW if you can do the full thing vs best effort" so this flag will be of use in a wider context.

David Ahern

2015-10-16 15:57 UTC #21 lore raw

In reply to Jiri Benc

Quoted 13 lines · Jiri Benc

Quoted 5 lines · David Miller
No, it's definitely not OK, because lwtunnel support exists in Linus's tree.
And tools should be able to work on all kernels where lwtunnel support is available.
You can consider the lwtunnels feature as not finished in the current Linus's tree. It works, it won't change (thus anything using it in its current form will continue to work in all the future kernels), but mainstream tools won't make use of it until a kernel version later which will get some additional support.

Quoted 4 lines

I don't think it's much of a problem and I don't think it is the first time this would happen.
I'm afraid I don't have any solution that could do better.

What about a flag that requests the version from the relevant kernel subsystem?

Generate the same initial netlink message -- e.g., type set to RTM_NEWROUTE for example -- but with no attributes and the NLM_F_VERSION flag set. Send the message using whatever socket protocol is relevant for the command/query.

Then kernel side the message makes its way to the relevant handler, the handler sees the flag asking for version and responds with its version. The version would be local to the handler and is a means for telling userspace what attributes it understands.

David Miller

maintainer

2015-10-19 02:13 UTC #22 lore raw

From: David Ahern <[email protected]>
Date: Fri, 16 Oct 2015 09:57:33 -0600

Quoted 14 lines · Jiri Benc

Quoted 13 lines · David Miller
Quoted 5 lines
No, it's definitely not OK, because lwtunnel support exists in Linus's tree.
And tools should be able to work on all kernels where lwtunnel support is available.
You can consider the lwtunnels feature as not finished in the current Linus's tree. It works, it won't change (thus anything using it in its current form will continue to work in all the future kernels), but mainstream tools won't make use of it until a kernel version later which will get some additional support.

Quoted 7 lines

Quoted 4 lines
I don't think it's much of a problem and I don't think it is the first time this would happen.
I'm afraid I don't have any solution that could do better.
What about a flag that requests the version from the relevant kernel subsystem?

The whole point of having an easily extensible protocol like netlink with attributes and whatnot was to avoid messy schemes like versioning.

The big mistake was ignoring unknown attributes. I mean seriously, think about it, what if the new attribute specified by the user was some security setting or something? I'm sure even more dangerous examples can be imagined.

I'm beginning to wonder if we can just change this unilaterally to not ignore unrecognized attributes.

I am increasingly certain that things that would "break" we wouldn't want to succeed anyways.