[PATCH net-next iproute2 0/3] ip: Initial support for extack errors

[PATCH net-next iproute2 0/3] ip: Initial support for extack errors

[David Ahern]

Introduce a new function, rtnl_ack_extack, to allow commands to flip
to the new error reporting over time.

Convert iplink_modify to use the new function to display error strings
returned from ip link set commands.

David Ahern (3):
  netlink: import netlink message parsing from kernel
  netlink: Add support for extended ack to rtnl_talk
  ip link: Add extack handling for setlink

 include/libnetlink.h |  14 +++++
 include/nlattr.h     | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++
 ip/iplink.c          |  18 +++++-
 lib/Makefile         |   2 +-
 lib/libnetlink.c     |  96 ++++++++++++++++++++++++++----
 lib/nlattr.c         | 145 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 423 insertions(+), 14 deletions(-)
 create mode 100644 include/nlattr.h
 create mode 100644 lib/nlattr.c

-- 
2.1.4

[PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[David Ahern]

include/nlattr.h is pulled from include/net/netlink.h.
lib/nlattr.c is pulled from lib/nlattr.c

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 include/libnetlink.h |   8 +++
 include/nlattr.h     | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/Makefile         |   2 +-
 lib/libnetlink.c     |   4 --
 lib/nlattr.c         | 145 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 316 insertions(+), 5 deletions(-)
 create mode 100644 include/nlattr.h
 create mode 100644 lib/nlattr.c

diff --git a/include/libnetlink.h b/include/libnetlink.h
index c43ab0a2d9d9..e7c46f1870aa 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -11,6 +11,11 @@
 #include <linux/neighbour.h>
 #include <linux/netconf.h>
 #include <arpa/inet.h>
+#include "nlattr.h"
+
+#ifndef MIN
+#define MIN(a, b) ((a) < (b) ? (a) : (b))
+#endif
 
 struct rtnl_handle {
 	int			fd;
@@ -227,4 +232,7 @@ int rtnl_from_file(FILE *, rtnl_listen_filter_t handler,
  * messages from dump file */
 #define NLMSG_TSTAMP	15
 
+int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
+	      int len, const struct nla_policy *policy);
+
 #endif /* __LIBNETLINK_H__ */
diff --git a/include/nlattr.h b/include/nlattr.h
new file mode 100644
index 000000000000..0859b6ce686c
--- /dev/null
+++ b/include/nlattr.h
@@ -0,0 +1,162 @@
+#ifndef __NLATTR_H
+#define __NLATTR_H
+
+#include <linux/netlink.h>
+
+/**
+ * Standard attribute types to specify validation policy
+ */
+enum {
+	NLA_UNSPEC,
+	NLA_U8,
+	NLA_U16,
+	NLA_U32,
+	NLA_U64,
+	NLA_STRING,
+	NLA_FLAG,
+	NLA_MSECS,
+	NLA_NESTED,
+	NLA_NESTED_COMPAT,
+	NLA_NUL_STRING,
+	NLA_BINARY,
+	NLA_S8,
+	NLA_S16,
+	NLA_S32,
+	NLA_S64,
+	__NLA_TYPE_MAX,
+};
+
+#define NLA_TYPE_MAX (__NLA_TYPE_MAX - 1)
+
+/**
+ * nla_type - attribute type
+ * @nla: netlink attribute
+ */
+static inline int nla_type(const struct nlattr *nla)
+{
+	return nla->nla_type & NLA_TYPE_MASK;
+}
+
+/**
+ * nla_len - length of payload
+ * @nla: netlink attribute
+ */
+static inline int nla_len(const struct nlattr *nla)
+{
+	return nla->nla_len - NLA_HDRLEN;
+}
+
+/**
+ * struct nla_policy - attribute validation policy
+ * @type: Type of attribute or NLA_UNSPEC
+ * @len: Type specific length of payload
+ *
+ * Policies are defined as arrays of this struct, the array must be
+ * accessible by attribute type up to the highest identifier to be expected.
+ *
+ * Meaning of `len' field:
+ *    NLA_STRING           Maximum length of string
+ *    NLA_NUL_STRING       Maximum length of string (excluding NUL)
+ *    NLA_FLAG             Unused
+ *    NLA_BINARY           Maximum length of attribute payload
+ *    NLA_NESTED           Don't use `len' field -- length verification is
+ *                         done by checking len of nested header (or empty)
+ *    NLA_NESTED_COMPAT    Minimum length of structure payload
+ *    NLA_U8, NLA_U16,
+ *    NLA_U32, NLA_U64,
+ *    NLA_S8, NLA_S16,
+ *    NLA_S32, NLA_S64,
+ *    NLA_MSECS            Leaving the length field zero will verify the
+ *                         given type fits, using it verifies minimum length
+ *                         just like "All other"
+ *    All other            Minimum length of attribute payload
+ *
+ * Example:
+ * static const struct nla_policy my_policy[ATTR_MAX+1] = {
+ *      [ATTR_FOO] = { .type = NLA_U16 },
+ *      [ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
+ *      [ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ * };
+ */
+struct nla_policy {
+	__u16	type;
+	__u16	len;
+};
+
+/**
+ * nla_ok - check if the netlink attribute fits into the remaining bytes
+ * @nla: netlink attribute
+ * @remaining: number of bytes remaining in attribute stream
+ */
+static inline int nla_ok(const struct nlattr *nla, int remaining)
+{
+	return remaining >= (int) sizeof(*nla) &&
+	       nla->nla_len >= sizeof(*nla) &&
+	       nla->nla_len <= remaining;
+}
+
+/**
+ * nla_next - next netlink attribute in attribute stream
+ * @nla: netlink attribute
+ * @remaining: number of bytes remaining in attribute stream
+ *
+ * Returns the next netlink attribute in the attribute stream and
+ * decrements remaining by the size of the current attribute.
+ */
+static inline struct nlattr *nla_next(const struct nlattr *nla, int *remaining)
+{
+	unsigned int totlen = NLA_ALIGN(nla->nla_len);
+
+	*remaining -= totlen;
+	return (struct nlattr *) ((char *) nla + totlen);
+}
+
+/**
+ * nla_for_each_attr - iterate over a stream of attributes
+ * @pos: loop counter, set to current attribute
+ * @head: head of attribute stream
+ * @len: length of attribute stream
+ * @rem: initialized to len, holds bytes currently remaining in stream
+ */
+#define nla_for_each_attr(pos, head, len, rem) \
+	for (pos = head, rem = len; \
+	     nla_ok(pos, rem); \
+	     pos = nla_next(pos, &(rem)))
+
+/**
+ * nla_data - head of payload
+ * @nla: netlink attribute
+ */
+static inline void *nla_data(const struct nlattr *nla)
+{
+	return (char *) nla + NLA_HDRLEN;
+}
+
+/**
+ * nla_get_u32 - return payload of u32 attribute
+ * @nla: u32 netlink attribute
+ */
+static inline __u32 nla_get_u32(const struct nlattr *nla)
+{
+	return *(__u32 *) nla_data(nla);
+}
+
+/**
+ * nla_get_u16 - return payload of u16 attribute
+ * @nla: u16 netlink attribute
+ */
+static inline __u16 nla_get_u16(const struct nlattr *nla)
+{
+	return *(__u16 *) nla_data(nla);
+}
+
+/**
+ * nlmsg_len - length of message payload
+ * @nlh: netlink message header
+ */
+static inline int nlmsg_len(const struct nlmsghdr *nlh)
+{
+	return nlh->nlmsg_len - NLMSG_HDRLEN;
+}
+
+#endif /* __NLATTR_H */
diff --git a/lib/Makefile b/lib/Makefile
index 1d24ca24b9a3..77fac8d59446 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -8,7 +8,7 @@ CFLAGS += -fPIC
 
 UTILOBJ = utils.o rt_names.o ll_types.o ll_proto.o ll_addr.o \
 	inet_proto.o namespace.o json_writer.o \
-	names.o color.o bpf.o exec.o fs.o
+	names.o color.o bpf.o exec.o fs.o nlattr.o
 
 NLOBJ=libgenl.o ll_map.o libnetlink.o
 
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 5b75b2db4e0b..b5ee751c6b86 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -30,10 +30,6 @@
 #define SOL_NETLINK 270
 #endif
 
-#ifndef MIN
-#define MIN(a, b) ((a) < (b) ? (a) : (b))
-#endif
-
 int rcvbuf = 1024 * 1024;
 
 void rtnl_close(struct rtnl_handle *rth)
diff --git a/lib/nlattr.c b/lib/nlattr.c
new file mode 100644
index 000000000000..2a3a031fdb65
--- /dev/null
+++ b/lib/nlattr.c
@@ -0,0 +1,145 @@
+/*
+ * NETLINK      Netlink attributes
+ *
+ *		Authors:	Thomas Graf <tgraf@suug.ch>
+ *				Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
+ */
+
+#include <errno.h>
+#include "nlattr.h"
+#include "libnetlink.h"
+
+static const __u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+	[NLA_U8]	= sizeof(__u8),
+	[NLA_U16]	= sizeof(__u16),
+	[NLA_U32]	= sizeof(__u32),
+	[NLA_U64]	= sizeof(__u64),
+	[NLA_MSECS]	= sizeof(__u64),
+	[NLA_NESTED]	= NLA_HDRLEN,
+	[NLA_S8]	= sizeof(__s8),
+	[NLA_S16]	= sizeof(__s16),
+	[NLA_S32]	= sizeof(__s32),
+	[NLA_S64]	= sizeof(__s64),
+};
+
+static int validate_nla(const struct nlattr *nla, int maxtype,
+			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)
+		return 0;
+
+	pt = &policy[type];
+
+	if (pt->type > NLA_TYPE_MAX)
+		return -EINVAL;
+
+	switch (pt->type) {
+	case NLA_FLAG:
+		if (attrlen > 0)
+			return -ERANGE;
+		break;
+
+	case NLA_NUL_STRING:
+		if (pt->len)
+			minlen = MIN(attrlen, pt->len + 1);
+		else
+			minlen = attrlen;
+
+		if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL)
+			return -EINVAL;
+		/* fall through */
+
+	case NLA_STRING:
+		if (attrlen < 1)
+			return -ERANGE;
+
+		if (pt->len) {
+			char *buf = nla_data(nla);
+
+			if (buf[attrlen - 1] == '\0')
+				attrlen--;
+
+			if (attrlen > pt->len)
+				return -ERANGE;
+		}
+		break;
+
+	case NLA_BINARY:
+		if (pt->len && attrlen > pt->len)
+			return -ERANGE;
+		break;
+
+	case NLA_NESTED_COMPAT:
+		if (attrlen < pt->len)
+			return -ERANGE;
+		if (attrlen < NLA_ALIGN(pt->len))
+			break;
+		if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
+			return -ERANGE;
+		nla = nla_data(nla) + NLA_ALIGN(pt->len);
+		if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
+			return -ERANGE;
+		break;
+	case NLA_NESTED:
+		/* a nested attributes is allowed to be empty; if its not,
+		 * it must have a size of at least NLA_HDRLEN.
+		 */
+		if (attrlen == 0)
+			break;
+	default:
+		if (pt->len)
+			minlen = pt->len;
+		else if (pt->type != NLA_UNSPEC)
+			minlen = nla_attr_minlen[pt->type];
+
+		if (attrlen < minlen)
+			return -ERANGE;
+	}
+
+	return 0;
+}
+
+/**
+ * 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
+ *
+ * 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.
+ *
+ * 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)
+{
+	const struct nlattr *nla;
+	int rem, err;
+
+	memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
+
+	nla_for_each_attr(nla, head, len, rem) {
+		__u16 type = nla_type(nla);
+
+		if (type > 0 && type <= maxtype) {
+			if (policy) {
+				err = validate_nla(nla, maxtype, policy);
+				if (err < 0)
+					goto errout;
+			}
+
+			tb[type] = (struct nlattr *)nla;
+		}
+	}
+
+	err = 0;
+errout:
+	return err;
+}
-- 
2.1.4

[PATCH net-next iproute2 2/3] netlink: Add support for extended ack to rtnl_talk

[David Ahern]

Add support for extended ack error reporting.

Add a new function rtnl_talk_extack that takes a callback as an input
arg. If a netlink response contains extack attributes, the callback is
is invoked with the the err string, offset in the message and a pointer
to the message returned by the kernel.

Adding a new function allows commands to be moved over to the
extended error reporting over time.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 include/libnetlink.h |  6 ++++
 lib/libnetlink.c     | 92 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 91 insertions(+), 7 deletions(-)

diff --git a/include/libnetlink.h b/include/libnetlink.h
index e7c46f1870aa..6b031454ce2b 100644
--- a/include/libnetlink.h
+++ b/include/libnetlink.h
@@ -71,6 +71,9 @@ typedef int (*rtnl_listen_filter_t)(const struct sockaddr_nl *,
 				    struct rtnl_ctrl_data *,
 				    struct nlmsghdr *n, void *);
 
+typedef int (*nl_ext_ack_fn_t)(const char *errmsg, __u32 off,
+			       struct nlmsghdr *inner_nlh);
+
 struct rtnl_dump_filter_arg {
 	rtnl_filter_t filter;
 	void *arg1;
@@ -87,6 +90,9 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr *answer, size_t len)
 	__attribute__((warn_unused_result));
+int rtnl_talk_extack(struct rtnl_handle *rtnl, struct nlmsghdr *n,
+	      struct nlmsghdr *answer, size_t len, nl_ext_ack_fn_t errfn)
+	__attribute__((warn_unused_result));
 int rtnl_talk_suppress_rtnl_errmsg(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 				   struct nlmsghdr *answer, size_t len)
 	__attribute__((warn_unused_result));
diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index b5ee751c6b86..f6451dec1332 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -32,6 +32,61 @@
 
 int rcvbuf = 1024 * 1024;
 
+/* dump netlink extended ack error message */
+static int nl_dump_ext_err(struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
+{
+	const struct nla_policy extack_policy[NLMSGERR_ATTR_MAX + 1] = {
+		[NLMSGERR_ATTR_MSG]	= { .type = NLA_STRING },
+		[NLMSGERR_ATTR_OFFS]	= { .type = NLA_U32 },
+	};
+	struct nlattr *tb[NLMSGERR_ATTR_MAX + 1], *attr;
+	struct nlmsghdr *err_nlh = NULL;
+	struct nlmsgerr *err;
+	char *errmsg = NULL;
+	int hlen, alen;
+	__u32 off = 0;
+
+	if (!errfn)
+		return 0;
+
+	/* no TLVs, nothing to do here */
+	if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS))
+		return 0;
+
+	err = (struct nlmsgerr *)NLMSG_DATA(nlh);
+	hlen = sizeof(*err);
+
+	/* if NLM_F_CAPPED is set then the inner err msg was capped */
+	if (!(nlh->nlmsg_flags & NLM_F_CAPPED))
+		hlen += nlmsg_len(&err->msg);
+
+	attr = (struct nlattr *) ((void *) err + hlen);
+	alen = nlh->nlmsg_len - hlen;
+
+	if (nla_parse(tb, NLMSGERR_ATTR_MAX, attr, alen, extack_policy) != 0) {
+		fprintf(stderr,
+			"Failed to parse extended error attributes\n");
+		return 0;
+	}
+
+
+	if (tb[NLMSGERR_ATTR_MSG])
+		errmsg = (char *) nla_data(tb[NLMSGERR_ATTR_MSG]);
+
+	if (tb[NLMSGERR_ATTR_OFFS]) {
+		off = nla_get_u32(tb[NLMSGERR_ATTR_OFFS]);
+
+		if (off > nlh->nlmsg_len) {
+			fprintf(stderr,
+				"Invalid offset for NLMSGERR_ATTR_OFFS\n");
+			off = 0;
+		} else if (!(nlh->nlmsg_flags & NLM_F_CAPPED))
+			err_nlh = &err->msg;
+	}
+
+	return errfn(errmsg, off, err_nlh);
+}
+
 void rtnl_close(struct rtnl_handle *rth)
 {
 	if (rth->fd >= 0) {
@@ -45,6 +100,7 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned int subscriptions,
 {
 	socklen_t addr_len;
 	int sndbuf = 32768;
+	int one = 1;
 
 	memset(rth, 0, sizeof(*rth));
 
@@ -67,6 +123,11 @@ int rtnl_open_byproto(struct rtnl_handle *rth, unsigned int subscriptions,
 		return -1;
 	}
 
+	if (setsockopt(rth->fd, SOL_NETLINK, NETLINK_EXT_ACK,
+		       &one, sizeof(one)) < 0) {
+		/* debug/verbose message that it is not supported */
+	}
+
 	memset(&rth->local, 0, sizeof(rth->local));
 	rth->local.nl_family = AF_NETLINK;
 	rth->local.nl_groups = subscriptions;
@@ -413,9 +474,19 @@ int rtnl_dump_filter_nc(struct rtnl_handle *rth,
 	return rtnl_dump_filter_l(rth, a);
 }
 
+static void rtnl_talk_error(struct nlmsghdr *h, struct nlmsgerr *err,
+			    nl_ext_ack_fn_t errfn)
+{
+	if (nl_dump_ext_err(h, errfn))
+		return;
+
+	fprintf(stderr, "RTNETLINK answers: %s\n",
+		strerror(-err->error));
+}
+
 static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 		       struct nlmsghdr *answer, size_t maxlen,
-		       bool show_rtnl_err)
+		       bool show_rtnl_err, nl_ext_ack_fn_t errfn)
 {
 	int status;
 	unsigned int seq;
@@ -502,10 +573,10 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 					return 0;
 				}
 
-				if (rtnl->proto != NETLINK_SOCK_DIAG && show_rtnl_err)
-					fprintf(stderr,
-						"RTNETLINK answers: %s\n",
-						strerror(-err->error));
+				if (rtnl->proto != NETLINK_SOCK_DIAG &&
+				    show_rtnl_err)
+					rtnl_talk_error(h, err, errfn);
+
 				errno = -err->error;
 				return -1;
 			}
@@ -537,13 +608,20 @@ static int __rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 int rtnl_talk(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 	      struct nlmsghdr *answer, size_t maxlen)
 {
-	return __rtnl_talk(rtnl, n, answer, maxlen, true);
+	return __rtnl_talk(rtnl, n, answer, maxlen, true, NULL);
+}
+
+int rtnl_talk_extack(struct rtnl_handle *rtnl, struct nlmsghdr *n,
+		     struct nlmsghdr *answer, size_t maxlen,
+		     nl_ext_ack_fn_t errfn)
+{
+	return __rtnl_talk(rtnl, n, answer, maxlen, true, errfn);
 }
 
 int rtnl_talk_suppress_rtnl_errmsg(struct rtnl_handle *rtnl, struct nlmsghdr *n,
 				   struct nlmsghdr *answer, size_t maxlen)
 {
-	return __rtnl_talk(rtnl, n, answer, maxlen, false);
+	return __rtnl_talk(rtnl, n, answer, maxlen, false, NULL);
 }
 
 int rtnl_listen_all_nsid(struct rtnl_handle *rth)
-- 
2.1.4

[PATCH net-next iproute2 3/3] ip link: Add extack handling for setlink

[David Ahern]

Flip iplink_modify to rtnl_talk_extack. For this first patch only
error messages returned from the kernel are displayed to the user.

Follow on patches can add parsing of the returned message and the
error offset to show which attribute caused an error.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 ip/iplink.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/ip/iplink.c b/ip/iplink.c
index ae1c70ebcc81..aad0220a63a7 100644
--- a/ip/iplink.c
+++ b/ip/iplink.c
@@ -861,6 +861,19 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 	return ret - argc;
 }
 
+static int iplink_extack(const char *errmsg, __u32 off,
+			 struct nlmsghdr *err_nlh)
+{
+	int rc = 0;
+
+	if (errmsg) {
+		rc++;
+		fprintf(stderr, "Error: %s\n", errmsg);
+	}
+
+	return rc;
+}
+
 static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 {
 	int len;
@@ -906,7 +919,8 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 
 			req.i.ifi_index = 0;
 			addattr32(&req.n, sizeof(req), IFLA_GROUP, group);
-			if (rtnl_talk(&rth, &req.n, NULL, 0) < 0)
+			if (rtnl_talk_extack(&rth, &req.n, NULL, 0,
+					     iplink_extack) < 0)
 				return -2;
 			return 0;
 		}
@@ -1001,7 +1015,7 @@ static int iplink_modify(int cmd, unsigned int flags, int argc, char **argv)
 		return -1;
 	}
 
-	if (rtnl_talk(&rth, &req.n, NULL, 0) < 0)
+	if (rtnl_talk_extack(&rth, &req.n, NULL, 0, iplink_extack) < 0)
 		return -2;
 
 	return 0;
-- 
2.1.4

Re: [PATCH net-next iproute2 0/3] ip: Initial support for extack errors

[Jakub Kicinski]

On Mon,  1 May 2017 20:18:22 -0700, David Ahern wrote:
> Introduce a new function, rtnl_ack_extack, to allow commands to flip
> to the new error reporting over time.
> 
> Convert iplink_modify to use the new function to display error strings
> returned from ip link set commands.

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

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[Stephen Hemminger]

On Mon,  1 May 2017 20:18:23 -0700
David Ahern <dsa@cumulusnetworks.com> wrote:

> include/nlattr.h is pulled from include/net/netlink.h.
> lib/nlattr.c is pulled from lib/nlattr.c
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

No. I am not taking a third way of parsing netlink attributes.


Please either use existing netlink attribute code in libnetlink.h
(rta_getattr_u32 etc) or use libmnl like devlink.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[David Ahern]

On 5/2/17 9:25 AM, Stephen Hemminger wrote:
> Please either use existing netlink attribute code in libnetlink.h
> (rta_getattr_u32 etc) or use libmnl like devlink.

All of the existing rta_ functions take a struct rta_attr; netlink
messages use struct nlattr. It's just wrong to use rta functions for
netlink messages.

There is no existing parse function for nlattr. There is no existing
validate function - for nlattr or rta_attr. So I do not see any overlap
with existing code.

If you prefer ip to gain a dependency on libmnl, I'll take a look.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Tue, 2 May 2017 10:51:23 -0600

> On 5/2/17 9:25 AM, Stephen Hemminger wrote:
>> Please either use existing netlink attribute code in libnetlink.h
>> (rta_getattr_u32 etc) or use libmnl like devlink.
> 
> All of the existing rta_ functions take a struct rta_attr; netlink
> messages use struct nlattr. It's just wrong to use rta functions for
> netlink messages.

Agreed.

> There is no existing parse function for nlattr. There is no existing
> validate function - for nlattr or rta_attr. So I do not see any overlap
> with existing code.

Also agreed.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[Stephen Hemminger]

On Tue, 02 May 2017 13:00:32 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Tue, 2 May 2017 10:51:23 -0600
> 
> > On 5/2/17 9:25 AM, Stephen Hemminger wrote:  
> >> Please either use existing netlink attribute code in libnetlink.h
> >> (rta_getattr_u32 etc) or use libmnl like devlink.  
> > 
> > All of the existing rta_ functions take a struct rta_attr; netlink
> > messages use struct nlattr. It's just wrong to use rta functions for
> > netlink messages.  
> 
> Agreed.
> 
> > There is no existing parse function for nlattr. There is no existing
> > validate function - for nlattr or rta_attr. So I do not see any overlap
> > with existing code.  
> 
> Also agreed.

Then use libmnl it is already used in several other places in iproute2.
Eventually, I would like to use it everywhere and get rid of old netlink parser.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[David Ahern]

On 5/2/17 12:03 PM, Stephen Hemminger wrote:
> Then use libmnl it is already used in several other places in iproute2.
> Eventually, I would like to use it everywhere and get rid of old netlink parser.
> 

Why? libmnl is not going to simplify the iproute2 code.

Look at attribute validation. Importing the kernel code into iproute2,
the API is very familiar to anyone hacking on the kernel side:

+       if (nla_parse(tb, NLMSGERR_ATTR_MAX, attr, alen, extack_policy)
!= 0) {
+               fprintf(stderr,
+                       "Failed to parse extended error attributes\n");
+               return 0;
+       }
+

ie., you pass a policy to the parse routine and the checking is part of
nla_parse. The implementation of nla_parse and validate_nla are quite
easy to read.

Now take a look at what devlink has for validation - attr_cb. IMO very
unreadable and puts the burden on the app using the libmnl API.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Tue, 2 May 2017 12:39:51 -0600

> On 5/2/17 12:03 PM, Stephen Hemminger wrote:
>> Then use libmnl it is already used in several other places in iproute2.
>> Eventually, I would like to use it everywhere and get rid of old netlink parser.
>> 
> 
> Why? libmnl is not going to simplify the iproute2 code.

Agreed.

> Look at attribute validation. Importing the kernel code into iproute2,
> the API is very familiar to anyone hacking on the kernel side:
> 
> +       if (nla_parse(tb, NLMSGERR_ATTR_MAX, attr, alen, extack_policy)
> != 0) {
> +               fprintf(stderr,
> +                       "Failed to parse extended error attributes\n");
> +               return 0;
> +       }
> +
> 
> ie., you pass a policy to the parse routine and the checking is part of
> nla_parse. The implementation of nla_parse and validate_nla are quite
> easy to read.
> 
> Now take a look at what devlink has for validation - attr_cb. IMO very
> unreadable and puts the burden on the app using the libmnl API.

Also agreed.

Stephen I totally agree with David, asking him to use libmnl for this is
not reasonable nor is it even a good idea given the above.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[Stephen Hemminger]

I am not disagreeing that iproute2 should handle the extended
error format. Just want the solution to be as small as possible;
ie do no more than is absolutely necessary. And future proof
for the inevitable growth in new area.

> +
> +static const __u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
> +	[NLA_U8]	= sizeof(__u8),
> +	[NLA_U16]	= sizeof(__u16),
> +	[NLA_U32]	= sizeof(__u32),
> +	[NLA_U64]	= sizeof(__u64),
> +	[NLA_MSECS]	= sizeof(__u64),
> +	[NLA_NESTED]	= NLA_HDRLEN,
> +	[NLA_S8]	= sizeof(__s8),
> +	[NLA_S16]	= sizeof(__s16),
> +	[NLA_S32]	= sizeof(__s32),
> +	[NLA_S64]	= sizeof(__s64),
> +};
> +

This patch makes iproute2 now doing validation of netlink attributes
coming back from the kernel. What is the point, userspace should be
trusting the kernel.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[David Ahern]

On 5/2/17 1:49 PM, Stephen Hemminger wrote:
> I am not disagreeing that iproute2 should handle the extended
> error format. Just want the solution to be as small as possible;
> ie do no more than is absolutely necessary. And future proof
> for the inevitable growth in new area.

Understood. I was trying to not reinvent a wheel. nla_parse and
nla_validate have not really been touched in 10 years, and both are well
written with a good API to the rest of the code base.

>From there, I grabbed whole snippets as opposed to just taking what is
needed for the ext-ack feature. I left the name as nlattr.c for easy
diff if future code is wanted. The header file was renamed to nlattr.h
to avoid confusion with other netlink.h files. Not adding it to
libnetlink.h facilitates pulling more code via diff as needed.

In short, I think it is good to re-use code from the kernel (lib and
tools/lib) where possible and doing so in a way that makes updates as
easy as header files.

> 
>> +
>> +static const __u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
>> +	[NLA_U8]	= sizeof(__u8),
>> +	[NLA_U16]	= sizeof(__u16),
>> +	[NLA_U32]	= sizeof(__u32),
>> +	[NLA_U64]	= sizeof(__u64),
>> +	[NLA_MSECS]	= sizeof(__u64),
>> +	[NLA_NESTED]	= NLA_HDRLEN,
>> +	[NLA_S8]	= sizeof(__s8),
>> +	[NLA_S16]	= sizeof(__s16),
>> +	[NLA_S32]	= sizeof(__s32),
>> +	[NLA_S64]	= sizeof(__s64),
>> +};
>> +
> 
> This patch makes iproute2 now doing validation of netlink attributes
> coming back from the kernel. What is the point, userspace should be
> trusting the kernel.

The kernel has bugs too; userspace should verify what it sends. In this
case the policy validation is just data types -- a string was expected
and a string was returned, or an attribute should be a u32 and it is.
You can argue it is overkill for iproute2, but it is good coding practice.

And for many netlink based features iproute2 tends to be the model that
is copied into other code bases.

Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

[Stephen Hemminger]

On Tue, 2 May 2017 14:39:40 -0600
David Ahern <dsa@cumulusnetworks.com> wrote:

> On 5/2/17 1:49 PM, Stephen Hemminger wrote:
> > I am not disagreeing that iproute2 should handle the extended
> > error format. Just want the solution to be as small as possible;
> > ie do no more than is absolutely necessary. And future proof
> > for the inevitable growth in new area.  
> 
> Understood. I was trying to not reinvent a wheel. nla_parse and
> nla_validate have not really been touched in 10 years, and both are well
> written with a good API to the rest of the code base.
> 
> From there, I grabbed whole snippets as opposed to just taking what is
> needed for the ext-ack feature. I left the name as nlattr.c for easy
> diff if future code is wanted. The header file was renamed to nlattr.h
> to avoid confusion with other netlink.h files. Not adding it to
> libnetlink.h facilitates pulling more code via diff as needed.
> 
> In short, I think it is good to re-use code from the kernel (lib and
> tools/lib) where possible and doing so in a way that makes updates as
> easy as header files.

The problem with copy and paste is that the code diverges and rots.
Also the security and error model in user space are different

> >   
> >> +
> >> +static const __u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
> >> +	[NLA_U8]	= sizeof(__u8),
> >> +	[NLA_U16]	= sizeof(__u16),
> >> +	[NLA_U32]	= sizeof(__u32),
> >> +	[NLA_U64]	= sizeof(__u64),
> >> +	[NLA_MSECS]	= sizeof(__u64),
> >> +	[NLA_NESTED]	= NLA_HDRLEN,
> >> +	[NLA_S8]	= sizeof(__s8),
> >> +	[NLA_S16]	= sizeof(__s16),
> >> +	[NLA_S32]	= sizeof(__s32),
> >> +	[NLA_S64]	= sizeof(__s64),
> >> +};
> >> +  
> > 
> > This patch makes iproute2 now doing validation of netlink attributes
> > coming back from the kernel. What is the point, userspace should be
> > trusting the kernel.  
> 
> The kernel has bugs too; userspace should verify what it sends. In this
> case the policy validation is just data types -- a string was expected
> and a string was returned, or an attribute should be a u32 and it is.
> You can argue it is overkill for iproute2, but it is good coding practice.
> 
> And for many netlink based features iproute2 tends to be the model that
> is copied into other code bases.

Then why only for new code.

I am not trying to be overly picky. Just that review is the time to play
devil's advocate and look for issues.