[RFC 0/3] netlink: extended error reporting

[RFC 0/3] netlink: extended error reporting

[Johannes Berg]

So this is my first draft of what we'd talked about at netconf.
I'm not super happy with the way we have to pass the extended
error struct, but I don't see a way to implement reporting any
dynamic information (like error offsets) in any other way.

Alexander Shishkin had a nice way of reporting static extended
error data, but that isn't really suitable for reporting the
offset or even reporting the broken attribute from nla_parse().

Speaking of nla_parse(), that'll be somewhat complicated to do
since we'll have to track the offsets of where we're parsing,
but it might be possible since the nlattrs are just pointers
into the message, so (optionally?) passing the skb as well can
allow us to fill the offset information.

johannes

[RFC 1/3] netlink: extended error reporting

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 crypto/crypto_user.c              |  3 +-
 drivers/infiniband/core/netlink.c |  3 +-
 drivers/scsi/scsi_netlink.c       |  2 +-
 include/linux/netlink.h           | 10 ++++++-
 include/net/netlink.h             |  3 +-
 include/uapi/linux/netlink.h      | 26 +++++++++++++++++
 kernel/audit.c                    |  2 +-
 net/core/rtnetlink.c              |  3 +-
 net/core/sock_diag.c              |  3 +-
 net/hsr/hsr_netlink.c             |  4 +--
 net/netfilter/ipset/ip_set_core.c |  2 +-
 net/netfilter/nfnetlink.c         | 18 ++++++------
 net/netlink/af_netlink.c          | 60 ++++++++++++++++++++++++++++++++++-----
 net/netlink/genetlink.c           |  3 +-
 net/xfrm/xfrm_user.c              |  3 +-
 15 files changed, 117 insertions(+), 28 deletions(-)

diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index a90404a0c5ff..4d4433e80866 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -483,7 +483,8 @@ static const struct crypto_link {
 	[CRYPTO_MSG_DELRNG	- CRYPTO_MSG_BASE] = { .doit = crypto_del_rng },
 };
 
-static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			       struct netlink_ext_err *exterr)
 {
 	struct nlattr *attrs[CRYPTOCFGA_MAX+1];
 	const struct crypto_link *link;
diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index 10469b0088b5..679d65430113 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -146,7 +146,8 @@ int ibnl_put_attr(struct sk_buff *skb, struct nlmsghdr *nlh,
 }
 EXPORT_SYMBOL(ibnl_put_attr);
 
-static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			struct netlink_ext_err *exterr)
 {
 	struct ibnl_client *client;
 	int type = nlh->nlmsg_type;
diff --git a/drivers/scsi/scsi_netlink.c b/drivers/scsi/scsi_netlink.c
index 109802f776ed..50e624fb8307 100644
--- a/drivers/scsi/scsi_netlink.c
+++ b/drivers/scsi/scsi_netlink.c
@@ -111,7 +111,7 @@ scsi_nl_rcv_msg(struct sk_buff *skb)
 
 next_msg:
 		if ((err) || (nlh->nlmsg_flags & NLM_F_ACK))
-			netlink_ack(skb, nlh, err);
+			netlink_ack(skb, nlh, err, NULL);
 
 		skb_pull(skb, rlen);
 	}
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index da14ab61f363..662f343dc68b 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -62,11 +62,19 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
 	return __netlink_kernel_create(net, unit, THIS_MODULE, cfg);
 }
 
+struct netlink_ext_err {
+	const char *msg;
+	u32 ext_code;
+	u32 msg_offset;
+	u16 attr;
+};
+
 extern void netlink_kernel_release(struct sock *sk);
 extern int __netlink_change_ngroups(struct sock *sk, unsigned int groups);
 extern int netlink_change_ngroups(struct sock *sk, unsigned int groups);
 extern void __netlink_clear_multicast_users(struct sock *sk, unsigned int group);
-extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
+extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
+			const struct netlink_ext_err *exterr);
 extern int netlink_has_listeners(struct sock *sk, unsigned int group);
 
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock);
diff --git a/include/net/netlink.h b/include/net/netlink.h
index b239fcd33d80..f06c2e00ebc3 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -233,7 +233,8 @@ struct nl_info {
 };
 
 int netlink_rcv_skb(struct sk_buff *skb,
-		    int (*cb)(struct sk_buff *, struct nlmsghdr *));
+		    int (*cb)(struct sk_buff *, struct nlmsghdr *,
+			      struct netlink_ext_err *));
 int nlmsg_notify(struct sock *sk, struct sk_buff *skb, u32 portid,
 		 unsigned int group, int report, gfp_t flags);
 
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index b2c9c26ea30f..0ef970e6f9f5 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -101,6 +101,31 @@ struct nlmsghdr {
 struct nlmsgerr {
 	int		error;
 	struct nlmsghdr msg;
+	/*
+	 * followed by the message contents unless NETLINK_CAP_ACK was set,
+	 * message length is aligned with NLMSG_ALIGN()
+	 */
+	/*
+	 * followed by TLVs defined in enum nlmsgerr_attrs
+	 * if NETLINK_EXT_ACK was set
+	 */
+};
+
+/**
+ * enum nlmsgerr_attrs - netlink error message attributes
+ * @NLMSGERR_ATTR_UNUSED: unused
+ * @NLMSGERR_ATTR_MSG: error message string (string)
+ * @NLMSGERR_ATTR_OFFS: error offset in the original message (u32)
+ * @NLMSGERR_ATTR_CODE: extended per-subsystem error code (u32)
+ * @NLMSGERR_ATTR_ATTR: top-level attribute that caused the error
+ *	(or is missing, u16)
+ */
+enum nlmsgerr_attrs {
+	NLMSGERR_ATTR_UNUSED,
+	NLMSGERR_ATTR_MSG,
+	NLMSGERR_ATTR_OFFS,
+	NLMSGERR_ATTR_CODE,
+	NLMSGERR_ATTR_ATTR,
 };
 
 #define NETLINK_ADD_MEMBERSHIP		1
@@ -115,6 +140,7 @@ struct nlmsgerr {
 #define NETLINK_LISTEN_ALL_NSID		8
 #define NETLINK_LIST_MEMBERSHIPS	9
 #define NETLINK_CAP_ACK			10
+#define NETLINK_EXT_ACK			11
 
 struct nl_pktinfo {
 	__u32	group;
diff --git a/kernel/audit.c b/kernel/audit.c
index e794544f5e63..d5f0e1d92ea2 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1264,7 +1264,7 @@ static void audit_receive_skb(struct sk_buff *skb)
 		err = audit_receive_msg(skb, nlh);
 		/* if err or if this message says it wants a response */
 		if (err || (nlh->nlmsg_flags & NLM_F_ACK))
-			netlink_ack(skb, nlh, err);
+			netlink_ack(skb, nlh, err, NULL);
 
 		nlh = nlmsg_next(nlh, &len);
 	}
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index c4e84c558240..e3e693831e43 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4046,7 +4046,8 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 /* Process one rtnetlink message. */
 
-static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			     struct netlink_ext_err *exterr)
 {
 	struct net *net = sock_net(skb->sk);
 	rtnl_doit_func doit;
diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index 6b10573cc9fa..3348a7b976ce 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -246,7 +246,8 @@ static int __sock_diag_cmd(struct sk_buff *skb, struct nlmsghdr *nlh)
 	return err;
 }
 
-static int sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			     struct netlink_ext_err *exterr)
 {
 	int ret;
 
diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
index 1ab30e7d3f99..81dac16933fc 100644
--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -350,7 +350,7 @@ static int hsr_get_node_status(struct sk_buff *skb_in, struct genl_info *info)
 	return 0;
 
 invalid:
-	netlink_ack(skb_in, nlmsg_hdr(skb_in), -EINVAL);
+	netlink_ack(skb_in, nlmsg_hdr(skb_in), -EINVAL, NULL);
 	return 0;
 
 nla_put_failure:
@@ -432,7 +432,7 @@ static int hsr_get_node_list(struct sk_buff *skb_in, struct genl_info *info)
 	return 0;
 
 invalid:
-	netlink_ack(skb_in, nlmsg_hdr(skb_in), -EINVAL);
+	netlink_ack(skb_in, nlmsg_hdr(skb_in), -EINVAL, NULL);
 	return 0;
 
 nla_put_failure:
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index c296f9b606d4..26356bf8cebf 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1305,7 +1305,7 @@ ip_set_dump_start(struct sk_buff *skb, struct netlink_callback *cb)
 			 * manually :-(
 			 */
 			if (nlh->nlmsg_flags & NLM_F_ACK)
-				netlink_ack(cb->skb, nlh, ret);
+				netlink_ack(cb->skb, nlh, ret, NULL);
 			return ret;
 		}
 	}
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 68eda920160e..50f1ef27baab 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -148,7 +148,8 @@ int nfnetlink_unicast(struct sk_buff *skb, struct net *net, u32 portid,
 EXPORT_SYMBOL_GPL(nfnetlink_unicast);
 
 /* Process one complete nfnetlink message. */
-static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			     struct netlink_ext_err *exterr)
 {
 	struct net *net = sock_net(skb->sk);
 	const struct nfnl_callback *nc;
@@ -261,7 +262,7 @@ static void nfnl_err_deliver(struct list_head *err_list, struct sk_buff *skb)
 	struct nfnl_err *nfnl_err, *next;
 
 	list_for_each_entry_safe(nfnl_err, next, err_list, head) {
-		netlink_ack(skb, nfnl_err->nlh, nfnl_err->err);
+		netlink_ack(skb, nfnl_err->nlh, nfnl_err->err, NULL);
 		nfnl_err_del(nfnl_err);
 	}
 }
@@ -304,20 +305,20 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
 #endif
 		{
 			nfnl_unlock(subsys_id);
-			netlink_ack(oskb, nlh, -EOPNOTSUPP);
+			netlink_ack(oskb, nlh, -EOPNOTSUPP, NULL);
 			return kfree_skb(skb);
 		}
 	}
 
 	if (!ss->commit || !ss->abort) {
 		nfnl_unlock(subsys_id);
-		netlink_ack(oskb, nlh, -EOPNOTSUPP);
+		netlink_ack(oskb, nlh, -EOPNOTSUPP, NULL);
 		return kfree_skb(skb);
 	}
 
 	if (genid && ss->valid_genid && !ss->valid_genid(net, genid)) {
 		nfnl_unlock(subsys_id);
-		netlink_ack(oskb, nlh, -ERESTART);
+		netlink_ack(oskb, nlh, -ERESTART, NULL);
 		return kfree_skb(skb);
 	}
 
@@ -407,7 +408,8 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
 				 * pointing to the batch header.
 				 */
 				nfnl_err_reset(&err_list);
-				netlink_ack(oskb, nlmsg_hdr(oskb), -ENOMEM);
+				netlink_ack(oskb, nlmsg_hdr(oskb), -ENOMEM,
+					    NULL);
 				status |= NFNL_BATCH_FAILURE;
 				goto done;
 			}
@@ -467,7 +469,7 @@ static void nfnetlink_rcv_skb_batch(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 	err = nla_parse(cda, NFNL_BATCH_MAX, attr, attrlen, nfnl_batch_policy);
 	if (err < 0) {
-		netlink_ack(skb, nlh, err);
+		netlink_ack(skb, nlh, err, NULL);
 		return;
 	}
 	if (cda[NFNL_BATCH_GENID])
@@ -493,7 +495,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 		return;
 
 	if (!netlink_net_capable(skb, CAP_NET_ADMIN)) {
-		netlink_ack(skb, nlh, -EPERM);
+		netlink_ack(skb, nlh, -EPERM, NULL);
 		return;
 	}
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7b73c7c161a9..532d906af3be 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -85,6 +85,7 @@ struct listeners {
 #define NETLINK_F_RECV_NO_ENOBUFS	0x8
 #define NETLINK_F_LISTEN_ALL_NSID	0x10
 #define NETLINK_F_CAP_ACK		0x20
+#define NETLINK_F_EXT_ACK		0x40
 
 static inline int netlink_is_kernel(struct sock *sk)
 {
@@ -1619,6 +1620,13 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			nlk->flags &= ~NETLINK_F_CAP_ACK;
 		err = 0;
 		break;
+	case NETLINK_EXT_ACK:
+		if (val)
+			nlk->flags |= NETLINK_F_EXT_ACK;
+		else
+			nlk->flags &= ~NETLINK_F_EXT_ACK;
+		err = 0;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 	}
@@ -1703,6 +1711,16 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 			return -EFAULT;
 		err = 0;
 		break;
+	case NETLINK_EXT_ACK:
+		if (len < sizeof(int))
+			return -EINVAL;
+		len = sizeof(int);
+		val = nlk->flags & NETLINK_F_EXT_ACK ? 1 : 0;
+		if (put_user(len, optlen) ||
+		    put_user(val, optval))
+			return -EFAULT;
+		err = 0;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 	}
@@ -2234,7 +2252,8 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 }
 EXPORT_SYMBOL(__netlink_dump_start);
 
-void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
+void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
+		 const struct netlink_ext_err *exterr)
 {
 	struct sk_buff *skb;
 	struct nlmsghdr *rep;
@@ -2243,10 +2262,22 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 	struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
 
 	/* Error messages get the original request appened, unless the user
-	 * requests to cap the error message.
+	 * requests to cap the error message, and get extra error data if
+	 * requested.
 	 */
-	if (!(nlk->flags & NETLINK_F_CAP_ACK) && err)
-		payload += nlmsg_len(nlh);
+	if (err) {
+		if (!(nlk->flags & NETLINK_F_CAP_ACK))
+			payload += nlmsg_len(nlh);
+		if (nlk->flags & NETLINK_F_EXT_ACK) {
+			if (exterr && exterr->msg)
+				payload +=
+					nla_total_size(strlen(exterr->msg) + 1);
+			if (exterr && exterr->msg_offset)
+				payload += nla_total_size(sizeof(u32));
+			if (exterr && exterr->attr)
+				payload += nla_total_size(sizeof(u16));
+		}
+	}
 
 	skb = nlmsg_new(payload, GFP_KERNEL);
 	if (!skb) {
@@ -2268,13 +2299,28 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 	errmsg = nlmsg_data(rep);
 	errmsg->error = err;
 	memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh));
+
+	if (nlk->flags & NETLINK_F_EXT_ACK) {
+		if (exterr && exterr->msg)
+			WARN_ON(nla_put_string(skb, NLMSGERR_ATTR_MSG,
+					       exterr->msg));
+		if (exterr && exterr->msg_offset)
+			WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_OFFS,
+					    exterr->msg_offset));
+		if (exterr && exterr->attr)
+			WARN_ON(nla_put_u16(skb, NLMSGERR_ATTR_ATTR,
+					    exterr->attr));
+	}
+
 	netlink_unicast(in_skb->sk, skb, NETLINK_CB(in_skb).portid, MSG_DONTWAIT);
 }
 EXPORT_SYMBOL(netlink_ack);
 
 int netlink_rcv_skb(struct sk_buff *skb, int (*cb)(struct sk_buff *,
-						     struct nlmsghdr *))
+						   struct nlmsghdr *,
+						   struct netlink_ext_err *))
 {
+	struct netlink_ext_err exterr = {};
 	struct nlmsghdr *nlh;
 	int err;
 
@@ -2295,13 +2341,13 @@ int netlink_rcv_skb(struct sk_buff *skb, int (*cb)(struct sk_buff *,
 		if (nlh->nlmsg_type < NLMSG_MIN_TYPE)
 			goto ack;
 
-		err = cb(skb, nlh);
+		err = cb(skb, nlh, &exterr);
 		if (err == -EINTR)
 			goto skip;
 
 ack:
 		if (nlh->nlmsg_flags & NLM_F_ACK || err)
-			netlink_ack(skb, nlh, err);
+			netlink_ack(skb, nlh, err, &exterr);
 
 skip:
 		msglen = NLMSG_ALIGN(nlh->nlmsg_len);
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index fb6e10fdb217..48f21dc467a7 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -605,7 +605,8 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 	return err;
 }
 
-static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			struct netlink_ext_err *exterr)
 {
 	const struct genl_family *family;
 	int err;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 9705c279494b..de0c672c771b 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2441,7 +2441,8 @@ static const struct xfrm_link {
 	[XFRM_MSG_GETSPDINFO  - XFRM_MSG_BASE] = { .doit = xfrm_get_spdinfo   },
 };
 
-static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
+static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
+			     struct netlink_ext_err *exterr)
 {
 	struct net *net = sock_net(skb->sk);
 	struct nlattr *attrs[XFRMA_MAX+1];
-- 
2.11.0

Re: [RFC 1/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 20:26 +0200, Johannes Berg wrote:
> 
> +	if (nlk->flags & NETLINK_F_EXT_ACK) {
> +		if (exterr && exterr->msg)
> +			WARN_ON(nla_put_string(skb,
> NLMSGERR_ATTR_MSG,
> +					       exterr->msg));
> +		if (exterr && exterr->msg_offset)
> +			WARN_ON(nla_put_u32(skb, NLMSGERR_ATTR_OFFS,
> +					    exterr->msg_offset));
> +		if (exterr && exterr->attr)
> +			WARN_ON(nla_put_u16(skb, NLMSGERR_ATTR_ATTR,
> +					    exterr->attr));
> +	}

I forgot to check (err != 0) here, which can cause inconsistencies -
noticed that while just adding the error case Jamal wanted. I'll send
out that as a separate patch, and squash it later when I resubmit.

johannes

[RFC 2/3] genetlink: pass extended error report down

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/genetlink.h | 27 +++++++++++++++++++++++++++
 net/netlink/genetlink.c |  6 ++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index a34275be3600..67ad2326cfa6 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -84,6 +84,7 @@ struct nlattr **genl_family_attrbuf(const struct genl_family *family);
  * @attrs: netlink attributes
  * @_net: network namespace
  * @user_ptr: user pointers
+ * @exterr: extended error report struct
  */
 struct genl_info {
 	u32			snd_seq;
@@ -94,6 +95,7 @@ struct genl_info {
 	struct nlattr **	attrs;
 	possible_net_t		_net;
 	void *			user_ptr[2];
+	struct netlink_ext_err *exterr;
 };
 
 static inline struct net *genl_info_net(struct genl_info *info)
@@ -106,6 +108,31 @@ static inline void genl_info_net_set(struct genl_info *info, struct net *net)
 	write_pnet(&info->_net, net);
 }
 
+static inline int genl_err_str(struct genl_info *info, int err,
+			       const char *msg)
+{
+	info->exterr->msg = msg;
+
+	return err;
+}
+
+static inline int genl_err_attr(struct genl_info *info, int err,
+				u16 attr)
+{
+	info->exterr->attr = attr;
+
+	return err;
+}
+
+static inline int genl_err_str_attr(struct genl_info *info, int err,
+				    const char *msg, u16 attr)
+{
+	info->exterr->msg = msg;
+	info->exterr->attr = attr;
+
+	return err;
+}
+
 /**
  * struct genl_ops - generic netlink operations
  * @cmd: command identifier
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 48f21dc467a7..39182d6a990e 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -497,7 +497,8 @@ static int genl_lock_done(struct netlink_callback *cb)
 
 static int genl_family_rcv_msg(const struct genl_family *family,
 			       struct sk_buff *skb,
-			       struct nlmsghdr *nlh)
+			       struct nlmsghdr *nlh,
+			       struct netlink_ext_err *exterr)
 {
 	const struct genl_ops *ops;
 	struct net *net = sock_net(skb->sk);
@@ -584,6 +585,7 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 	info.genlhdr = nlmsg_data(nlh);
 	info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
 	info.attrs = attrbuf;
+	info.exterr = exterr;
 	genl_info_net_set(&info, net);
 	memset(&info.user_ptr, 0, sizeof(info.user_ptr));
 
@@ -618,7 +620,7 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (!family->parallel_ops)
 		genl_lock();
 
-	err = genl_family_rcv_msg(family, skb, nlh);
+	err = genl_family_rcv_msg(family, skb, nlh, exterr);
 
 	if (!family->parallel_ops)
 		genl_unlock();
-- 
2.11.0

Re: [RFC 2/3] genetlink: pass extended error report down

[Ben Greear]

On 04/07/2017 11:26 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/genetlink.h | 27 +++++++++++++++++++++++++++
>  net/netlink/genetlink.c |  6 ++++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/genetlink.h b/include/net/genetlink.h
> index a34275be3600..67ad2326cfa6 100644
> --- a/include/net/genetlink.h
> +++ b/include/net/genetlink.h
> @@ -84,6 +84,7 @@ struct nlattr **genl_family_attrbuf(const struct genl_family *family);
>   * @attrs: netlink attributes
>   * @_net: network namespace
>   * @user_ptr: user pointers
> + * @exterr: extended error report struct
>   */
>  struct genl_info {
>  	u32			snd_seq;
> @@ -94,6 +95,7 @@ struct genl_info {
>  	struct nlattr **	attrs;
>  	possible_net_t		_net;
>  	void *			user_ptr[2];
> +	struct netlink_ext_err *exterr;
>  };
>
>  static inline struct net *genl_info_net(struct genl_info *info)
> @@ -106,6 +108,31 @@ static inline void genl_info_net_set(struct genl_info *info, struct net *net)
>  	write_pnet(&info->_net, net);
>  }
>
> +static inline int genl_err_str(struct genl_info *info, int err,
> +			       const char *msg)
> +{
> +	info->exterr->msg = msg;
> +
> +	return err;
> +}

I guess the error string must be constant and always available in memory
in this implementation?

I think it would be nice to dynamically create strings (malloc, snprintf, etc)
and have the err_str logic free it when done?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/3] genetlink: pass extended error report down

[Johannes Berg]

On Fri, 2017-04-07 at 11:37 -0700, Ben Greear wrote:
> 
> I guess the error string must be constant and always available in
> memory in this implementation?

Yes.

> I think it would be nice to dynamically create strings (malloc,
> snprintf, etc) and have the err_str logic free it when done?

We can think about that later, but I don't actually think it makes a
lot of sense - if we point to the attribute and/or offset you really
ought to have enough info to figure out what's up.

johannes

Re: [RFC 2/3] genetlink: pass extended error report down

[Ben Greear]

On 04/07/2017 12:12 PM, Johannes Berg wrote:
> On Fri, 2017-04-07 at 11:37 -0700, Ben Greear wrote:
>>
>> I guess the error string must be constant and always available in
>> memory in this implementation?
>
> Yes.
>
>> I think it would be nice to dynamically create strings (malloc,
>> snprintf, etc) and have the err_str logic free it when done?
>
> We can think about that later, but I don't actually think it makes a
> lot of sense - if we point to the attribute and/or offset you really
> ought to have enough info to figure out what's up.

We can think about it later, but lots of things in the wifi stack
could use a descriptive message specific to the failure.  Often these
messages are much more useful if you explain why the failure conflicts
with regulatory, channel, virtual-dev combination, etc info, so that needs
to be dynamic.  The code that is failing knows, so I'd like to pass it
back to user-space.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

[RFC 3/3] nl80211: add a few extended error strings

[Johannes Berg]

From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 net/wireless/nl80211.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 9910aae08f1a..8808099db909 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -740,7 +740,8 @@ struct key_parse {
 	bool def_uni, def_multi;
 };
 
-static int nl80211_parse_key_new(struct nlattr *key, struct key_parse *k)
+static int nl80211_parse_key_new(struct genl_info *info, struct nlattr *key,
+				 struct key_parse *k)
 {
 	struct nlattr *tb[NL80211_KEY_MAX + 1];
 	int err = nla_parse_nested(tb, NL80211_KEY_MAX, key,
@@ -777,7 +778,7 @@ static int nl80211_parse_key_new(struct nlattr *key, struct key_parse *k)
 	if (tb[NL80211_KEY_TYPE]) {
 		k->type = nla_get_u32(tb[NL80211_KEY_TYPE]);
 		if (k->type < 0 || k->type >= NUM_NL80211_KEYTYPES)
-			return -EINVAL;
+			return genl_err_attr(info, -EINVAL, NL80211_KEY_TYPE);
 	}
 
 	if (tb[NL80211_KEY_DEFAULT_TYPES]) {
@@ -855,7 +856,7 @@ static int nl80211_parse_key(struct genl_info *info, struct key_parse *k)
 	k->type = -1;
 
 	if (info->attrs[NL80211_ATTR_KEY])
-		err = nl80211_parse_key_new(info->attrs[NL80211_ATTR_KEY], k);
+		err = nl80211_parse_key_new(info, info->attrs[NL80211_ATTR_KEY], k);
 	else
 		err = nl80211_parse_key_old(info, k);
 
@@ -863,23 +864,27 @@ static int nl80211_parse_key(struct genl_info *info, struct key_parse *k)
 		return err;
 
 	if (k->def && k->defmgmt)
-		return -EINVAL;
+		return genl_err_str(info, -EINVAL, "def && defmgmt is invalid");
 
 	if (k->defmgmt) {
 		if (k->def_uni || !k->def_multi)
-			return -EINVAL;
+			return genl_err_str(info, -EINVAL,
+					    "defmgmt must be mcast");
 	}
 
 	if (k->idx != -1) {
 		if (k->defmgmt) {
 			if (k->idx < 4 || k->idx > 5)
-				return -EINVAL;
+				return genl_err_str(info, -EINVAL,
+						    "defmgmt key idx not 4 or 5");
 		} else if (k->def) {
 			if (k->idx < 0 || k->idx > 3)
-				return -EINVAL;
+				return genl_err_str(info, -EINVAL,
+						    "def key idx not 0-3");
 		} else {
 			if (k->idx < 0 || k->idx > 5)
-				return -EINVAL;
+				return genl_err_str(info, -EINVAL,
+						    "key idx not 0-5");
 		}
 	}
 
@@ -888,8 +893,9 @@ static int nl80211_parse_key(struct genl_info *info, struct key_parse *k)
 
 static struct cfg80211_cached_keys *
 nl80211_parse_connkeys(struct cfg80211_registered_device *rdev,
-		       struct nlattr *keys, bool *no_ht)
+		       struct genl_info *info, bool *no_ht)
 {
+	struct nlattr *keys = info->attrs[NL80211_ATTR_KEYS];
 	struct key_parse parse;
 	struct nlattr *key;
 	struct cfg80211_cached_keys *result;
@@ -914,7 +920,7 @@ nl80211_parse_connkeys(struct cfg80211_registered_device *rdev,
 		memset(&parse, 0, sizeof(parse));
 		parse.idx = -1;
 
-		err = nl80211_parse_key_new(key, &parse);
+		err = nl80211_parse_key_new(info, key, &parse);
 		if (err)
 			goto error;
 		err = -EINVAL;
@@ -8460,9 +8466,7 @@ static int nl80211_join_ibss(struct sk_buff *skb, struct genl_info *info)
 	if (ibss.privacy && info->attrs[NL80211_ATTR_KEYS]) {
 		bool no_ht = false;
 
-		connkeys = nl80211_parse_connkeys(rdev,
-					  info->attrs[NL80211_ATTR_KEYS],
-					  &no_ht);
+		connkeys = nl80211_parse_connkeys(rdev, info, &no_ht);
 		if (IS_ERR(connkeys))
 			return PTR_ERR(connkeys);
 
@@ -8853,8 +8857,7 @@ static int nl80211_connect(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	if (connect.privacy && info->attrs[NL80211_ATTR_KEYS]) {
-		connkeys = nl80211_parse_connkeys(rdev,
-					  info->attrs[NL80211_ATTR_KEYS], NULL);
+		connkeys = nl80211_parse_connkeys(rdev, info, NULL);
 		if (IS_ERR(connkeys))
 			return PTR_ERR(connkeys);
 	}
-- 
2.11.0

Re: [RFC 0/3] netlink: extended error reporting

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Fri,  7 Apr 2017 20:26:17 +0200

> So this is my first draft of what we'd talked about at netconf.
> I'm not super happy with the way we have to pass the extended
> error struct, but I don't see a way to implement reporting any
> dynamic information (like error offsets) in any other way.
> 
> Alexander Shishkin had a nice way of reporting static extended
> error data, but that isn't really suitable for reporting the
> offset or even reporting the broken attribute from nla_parse().
> 
> Speaking of nla_parse(), that'll be somewhat complicated to do
> since we'll have to track the offsets of where we're parsing,
> but it might be possible since the nlattrs are just pointers
> into the message, so (optionally?) passing the skb as well can
> allow us to fill the offset information.

I like it, nice work.

I know people want dynamically generated strings and stuff, and we can
get there, but I prefer that the first thing we commit is super simple.

Someone gave me a hard time about the fact that we've been talking
about this idea for years but nothing ever happens.

I'm tempted to apply this as-is just to show that person that things
do in fact happen.... eventually :-)

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 11:53 -0700, David Miller wrote:

> > Alexander Shishkin had a nice way of reporting static extended
> > error data, but that isn't really suitable for reporting the
> > offset or even reporting the broken attribute from nla_parse().
> > 
> > Speaking of nla_parse(), that'll be somewhat complicated to do
> > since we'll have to track the offsets of where we're parsing,
> > but it might be possible since the nlattrs are just pointers
> > into the message, so (optionally?) passing the skb as well can
> > allow us to fill the offset information.
> 
> I like it, nice work.

I don't like it all that much ;-)

Alexander Shishkin's patch
https://patchwork.kernel.org/patch/7162421/

was nice in a way because you could get away without passing the error
structure down all the time, but like I said it doesn't deal with
dynamic errors (even the offset/attr from nla_parse) and if it's not
done *everywhere* it risks leaking those exterr numbers (-1024..-4095)
to userspace through syscalls if it's used in a non-netlink path for
some reason (e.g. a single driver call being called through both
netlink and ioctl, and trying to return an extended error)

> I know people want dynamically generated strings and stuff, and we
> can get there, but I prefer that the first thing we commit is super
> simple.
> 
> Someone gave me a hard time about the fact that we've been talking
> about this idea for years but nothing ever happens.
> 
> I'm tempted to apply this as-is just to show that person that things
> do in fact happen.... eventually :-)

Heh. I think I really want to solve - at least partially - nla_parse()
to see that it can be done this way. It'd be nice to even transform all
the callers (I generated half of these patches with spatch anyway) to
have at least that.

johannes

Re: [RFC 0/3] netlink: extended error reporting

[Pablo Neira Ayuso]

On Fri, Apr 07, 2017 at 08:59:12PM +0200, Johannes Berg wrote:
[...]
> Heh. I think I really want to solve - at least partially - nla_parse()
> to see that it can be done this way. It'd be nice to even transform all
> the callers (I generated half of these patches with spatch anyway) to
> have at least that.

We can just have a modified version of nla_parse that deals with this.
We can probably use struct nla_policy to place the extended error code
or the string (as we discussed I don't need string errors, but I'm
fine if other people find it useful).

Thanks!

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 21:06 +0200, Pablo Neira Ayuso wrote:
> On Fri, Apr 07, 2017 at 08:59:12PM +0200, Johannes Berg wrote:
> [...]
> > Heh. I think I really want to solve - at least partially -
> > nla_parse()
> > to see that it can be done this way. It'd be nice to even transform
> > all
> > the callers (I generated half of these patches with spatch anyway)
> > to
> > have at least that.
> 
> We can just have a modified version of nla_parse that deals with
> this.

Yes, but we need to figure out a good way to have the offset.

We also need to see if we want to *force* having the offset. In some
sense that'd be useful, in another it might be very complicated to fill
it in at all times, if for example errors come from lower layers like
drivers.

(It's ultimately always going to be optional since we'll continue
returning errors without *any* extended error information - likely
indefinitely - but if we have a wrong attribute, should we always have
an offset? Would be nice, but could be difficult in practice)

> We can probably use struct nla_policy to place the extended error
> code or the string (as we discussed I don't need string errors, but
> I'm fine if other people find it useful).

I don't think for the error strings really would be useful for
nla_parse() or a policy - we can return something generic like
"malformed attribute" there as a string, and hopefully point to the
attribute/offset from there generically. I just really want to see how
to actually do that.

johannes

Re: [RFC 0/3] netlink: extended error reporting

[Pablo Neira Ayuso]

On Fri, Apr 07, 2017 at 09:09:45PM +0200, Johannes Berg wrote:
> On Fri, 2017-04-07 at 21:06 +0200, Pablo Neira Ayuso wrote:
> > On Fri, Apr 07, 2017 at 08:59:12PM +0200, Johannes Berg wrote:
> > [...]
> > > Heh. I think I really want to solve - at least partially -
> > > nla_parse()
> > > to see that it can be done this way. It'd be nice to even transform
> > > all
> > > the callers (I generated half of these patches with spatch anyway)
> > > to
> > > have at least that.
> > 
> > We can just have a modified version of nla_parse that deals with
> > this.
> 
> Yes, but we need to figure out a good way to have the offset.
> 
> We also need to see if we want to *force* having the offset. In some
> sense that'd be useful, in another it might be very complicated to fill
> it in at all times, if for example errors come from lower layers like
> drivers.

For my usecases in netfilter, the attributes and an specific error
code should be enough to figure out what is wrong. Will not need
strings.

BTW, we may not have an offset, eg. EINVAL because of missing
attribute. Given we have different requirements, I would leave it to
each subsystem to decide what netlink error attributes are specified.

> (It's ultimately always going to be optional since we'll continue
> returning errors without *any* extended error information - likely
> indefinitely - but if we have a wrong attribute, should we always have
> an offset? Would be nice, but could be difficult in practice)
> 
> > We can probably use struct nla_policy to place the extended error
> > code or the string (as we discussed I don't need string errors, but
> > I'm fine if other people find it useful).
> 
> I don't think for the error strings really would be useful for
> nla_parse() or a policy - we can return something generic like
> "malformed attribute" there as a string, and hopefully point to the
> attribute/offset from there generically. I just really want to see how
> to actually do that.

I think the most flexible way is to pass the container error structure
to nla_parse() so it sets this for you. This would also save tons of
"malformed attribute" strings.

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 21:21 +0200, Pablo Neira Ayuso wrote:
> 
> For my usecases in netfilter, the attributes and an specific error
> code should be enough to figure out what is wrong. Will not need
> strings.

Fair enough.

> BTW, we may not have an offset, eg. EINVAL because of missing
> attribute. Given we have different requirements, I would leave it to
> each subsystem to decide what netlink error attributes are specified.
> 
> > (It's ultimately always going to be optional since we'll continue
> > returning errors without *any* extended error information - likely
> > indefinitely - but if we have a wrong attribute, should we always
> > have
> > an offset? Would be nice, but could be difficult in practice)
> > 
> > > We can probably use struct nla_policy to place the extended error
> > > code or the string (as we discussed I don't need string errors,
> > > but
> > > I'm fine if other people find it useful).
> > 
> > I don't think for the error strings really would be useful for
> > nla_parse() or a policy - we can return something generic like
> > "malformed attribute" there as a string, and hopefully point to the
> > attribute/offset from there generically. I just really want to see
> > how
> > to actually do that.
> 
> I think the most flexible way is to pass the container error
> structure to nla_parse() so it sets this for you. This would also
> save tons of "malformed attribute" strings.

Yes, for sure. The thing is we'll probalby have to pass down the
request skb *and* the error struct so that we can get the offset, and
this seems like the generic thing that we really should try to get the
most information generated.

johannes

Re: [RFC 0/3] netlink: extended error reporting

[Pablo Neira Ayuso]

On Fri, Apr 07, 2017 at 09:29:17PM +0200, Johannes Berg wrote:
> On Fri, 2017-04-07 at 21:21 +0200, Pablo Neira Ayuso wrote:
> > I think the most flexible way is to pass the container error
> > structure to nla_parse() so it sets this for you. This would also
> > save tons of "malformed attribute" strings.
> 
> Yes, for sure. The thing is we'll probalby have to pass down the
> request skb *and* the error struct so that we can get the offset, and
> this seems like the generic thing that we really should try to get the
> most information generated.

We only need to store the pointer to the attribute in the error
container structure. We can calculate the offset from nl_err() by
pasing the skbuff as parameter there, right?

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 21:45 +0200, Pablo Neira Ayuso wrote:
> 
> We only need to store the pointer to the attribute in the error
> container structure. We can calculate the offset from nl_err() by
> pasing the skbuff as parameter there, right?

Ah, that's a good point, we could store the nlattr pointer and
calculate the offset later. I'll need to try this, but yeah, good idea!

johannes

Re: [RFC 0/3] netlink: extended error reporting

[David Miller]

From: Pablo Neira Ayuso <pablo-Cap9r6Oaw4JrovVCs/uTlw@public.gmane.org>
Date: Fri, 7 Apr 2017 21:21:34 +0200

> For my usecases in netfilter, the attributes and an specific error
> code should be enough to figure out what is wrong. Will not need
> strings.
> 
> BTW, we may not have an offset, eg. EINVAL because of missing
> attribute. Given we have different requirements, I would leave it to
> each subsystem to decide what netlink error attributes are specified.

Yep, completely agreed.

The use cases for offset and missing attribute I see as follows:

1) Top-level attribute is missing. Here, offset is set to zero
   and the missing attribute number is given as well.

2) Nested attribute is missing.  Offset is set to the location of the
   beginning of nesting, inside of which the missing attribute was
   needed.

Re: [RFC 0/3] netlink: extended error reporting

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 07 Apr 2017 21:09:45 +0200

> On Fri, 2017-04-07 at 21:06 +0200, Pablo Neira Ayuso wrote:
>> On Fri, Apr 07, 2017 at 08:59:12PM +0200, Johannes Berg wrote:
>> [...]
>> > Heh. I think I really want to solve - at least partially -
>> > nla_parse()
>> > to see that it can be done this way. It'd be nice to even transform
>> > all
>> > the callers (I generated half of these patches with spatch anyway)
>> > to
>> > have at least that.
>> 
>> We can just have a modified version of nla_parse that deals with
>> this.
> 
> Yes, but we need to figure out a good way to have the offset.
> 
> We also need to see if we want to *force* having the offset. In some
> sense that'd be useful, in another it might be very complicated to fill
> it in at all times, if for example errors come from lower layers like
> drivers.

It has to be optional, some kinds of errors don't have an exact
context per-se.

Also another way to look at this is that we're providing a lot of
new power and expressability.  So even if only one aspect of the
new error reporting is used it's a positive step forward.

So allow offset "0" meaning "unspecified".

Re: [RFC 0/3] netlink: extended error reporting

[Pablo Neira Ayuso]

On Fri, Apr 07, 2017 at 12:22:23PM -0700, David Miller wrote:
> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
> Date: Fri, 07 Apr 2017 21:09:45 +0200
> 
> > On Fri, 2017-04-07 at 21:06 +0200, Pablo Neira Ayuso wrote:
> >> On Fri, Apr 07, 2017 at 08:59:12PM +0200, Johannes Berg wrote:
> >> [...]
> >> > Heh. I think I really want to solve - at least partially -
> >> > nla_parse()
> >> > to see that it can be done this way. It'd be nice to even transform
> >> > all
> >> > the callers (I generated half of these patches with spatch anyway)
> >> > to
> >> > have at least that.
> >> 
> >> We can just have a modified version of nla_parse that deals with
> >> this.
> > 
> > Yes, but we need to figure out a good way to have the offset.
> > 
> > We also need to see if we want to *force* having the offset. In some
> > sense that'd be useful, in another it might be very complicated to fill
> > it in at all times, if for example errors come from lower layers like
> > drivers.
> 
> It has to be optional, some kinds of errors don't have an exact
> context per-se.
> 
> Also another way to look at this is that we're providing a lot of
> new power and expressability.  So even if only one aspect of the
> new error reporting is used it's a positive step forward.
> 
> So allow offset "0" meaning "unspecified".

Instead, we can just not send the offset attribute to userspace if
it's not specified. So missing attribute means "unspecified".

I'm always a bit worried this "0 means something" semantics :)

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 21:27 +0200, Pablo Neira Ayuso wrote:
> 
> > Also another way to look at this is that we're providing a lot of
> > new power and expressability.  So even if only one aspect of the
> > new error reporting is used it's a positive step forward.

True.

> > So allow offset "0" meaning "unspecified".
> 
> Instead, we can just not send the offset attribute to userspace if
> it's not specified. So missing attribute means "unspecified".
> 
> I'm always a bit worried this "0 means something" semantics :)

Yeah, I have that. If it's 0 internally the attribute will be omitted.

johannes

Re: [RFC 0/3] netlink: extended error reporting

[David Miller]

From: Pablo Neira Ayuso <pablo-Cap9r6Oaw4JrovVCs/uTlw@public.gmane.org>
Date: Fri, 7 Apr 2017 21:27:14 +0200

> On Fri, Apr 07, 2017 at 12:22:23PM -0700, David Miller wrote:
>> From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
>> Date: Fri, 07 Apr 2017 21:09:45 +0200
>> 
>> > On Fri, 2017-04-07 at 21:06 +0200, Pablo Neira Ayuso wrote:
>> >> On Fri, Apr 07, 2017 at 08:59:12PM +0200, Johannes Berg wrote:
>> >> [...]
>> >> > Heh. I think I really want to solve - at least partially -
>> >> > nla_parse()
>> >> > to see that it can be done this way. It'd be nice to even transform
>> >> > all
>> >> > the callers (I generated half of these patches with spatch anyway)
>> >> > to
>> >> > have at least that.
>> >> 
>> >> We can just have a modified version of nla_parse that deals with
>> >> this.
>> > 
>> > Yes, but we need to figure out a good way to have the offset.
>> > 
>> > We also need to see if we want to *force* having the offset. In some
>> > sense that'd be useful, in another it might be very complicated to fill
>> > it in at all times, if for example errors come from lower layers like
>> > drivers.
>> 
>> It has to be optional, some kinds of errors don't have an exact
>> context per-se.
>> 
>> Also another way to look at this is that we're providing a lot of
>> new power and expressability.  So even if only one aspect of the
>> new error reporting is used it's a positive step forward.
>> 
>> So allow offset "0" meaning "unspecified".
> 
> Instead, we can just not send the offset attribute to userspace if
> it's not specified. So missing attribute means "unspecified".

Agreed, not providing the attribute to indicate this is fine.

Re: [RFC 0/3] netlink: extended error reporting

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 07 Apr 2017 20:59:12 +0200

> Alexander Shishkin's patch
> https://patchwork.kernel.org/patch/7162421/
> 
> was nice in a way because you could get away without passing the error
> structure down all the time, but like I said it doesn't deal with
> dynamic errors (even the offset/attr from nla_parse) and if it's not
> done *everywhere* it risks leaking those exterr numbers (-1024..-4095)
> to userspace through syscalls if it's used in a non-netlink path for
> some reason (e.g. a single driver call being called through both
> netlink and ioctl, and trying to return an extended error)

Alexander's patch is cute in that it annotates errors using the special
section blobs.

But what it lacks fundamentally is context.  Therefore it can't be
used to provide the offset or the bad attribute number.  So it can't
meet our requirements.

We've passed down new arguments into core method call chains for much
less useful facilitites than this.  So that doesn't bother me.  And
as that is a kernel internal matter, we can refine it later.

Wrt. nla_parse(), we can solve that problem as follow-on patches too.

So I consider this change ready as far as the implementation is
concerned.

Ok, Jiri, start reading.  I will try to make you happy here.

Let's just discuss the UAPI, since people complain we don't talk
about that enough :-)  For those playing at home it is three new
attributes returned in a netlink ACK when the application asks
for the extended response:

	NLMSGERR_ATTR_MSG	string	Extended error string
	NLMSGERR_ATTR_OFFS	u32	Byte offset to netlink element causing error
	NLMSGERR_ATTR_CODE	u32	Subsystem specific error code
	NLMSGERR_ATTR_ATTR	u16	Netlink attribute triggering error or missing

So please consider this interface carefully.

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 12:20 -0700, David Miller wrote:

> But what it lacks fundamentally is context.  Therefore it can't be
> used to provide the offset or the bad attribute number.  So it can't
> meet our requirements.

Yes, it doesn't address the requirements here, and in a sense I suspect
this will be true anywhere else too - perhaps that's the reason why it
was never applied even over in perf. I've not gotten a good answer for
that question yet :)

> We've passed down new arguments into core method call chains for much
> less useful facilitites than this.  So that doesn't bother me.  And
> as that is a kernel internal matter, we can refine it later.

Yes, both are true.

> Wrt. nla_parse(), we can solve that problem as follow-on patches too.

Yes. I just want to be sure we can solve it, but OTOH we're not locking
down anything but the UAPI so whatever way we find to solve it, even if
it requires a complete rearchitecture internally, it still doesn't
matter.

> So I consider this change ready as far as the implementation is
> concerned.

Fair enough. I still do need to test it though :)

> Ok, Jiri, start reading.  I will try to make you happy here.
> 
> Let's just discuss the UAPI, since people complain we don't talk
> about that enough :-)  For those playing at home it is three new
> attributes returned in a netlink ACK when the application asks
> for the extended response:
> 
>         NLMSGERR_ATTR_MSG       string  Extended error string
>         NLMSGERR_ATTR_OFFS      u32     Byte offset to netlink
> element causing error
>         NLMSGERR_ATTR_CODE      u32     Subsystem specific error code
>         NLMSGERR_ATTR_ATTR      u16     Netlink attribute triggering
> error or missing

That's four ;-)

Since you're carefully listing it here, you should also say that

 * the new behaviour is entirely optional, enabled by a new netlink
   socket option
 * the ACK message format ends up being
   [nlmsg header]
   [ack header including errno]
   [request message nlmsg hdr]
   [request message payload - optional, controlled by socket option,
    aligned to 4 byte length if present]
   [new TLVs - optional, controlled by socket option]

johannes

Re: [RFC 0/3] netlink: extended error reporting

[Pablo Neira Ayuso]

On Fri, Apr 07, 2017 at 12:20:53PM -0700, David Miller wrote:
[...]
> Let's just discuss the UAPI, since people complain we don't talk
> about that enough :-)  For those playing at home it is three new
> attributes returned in a netlink ACK when the application asks
> for the extended response:
> 
> 	NLMSGERR_ATTR_MSG	string	Extended error string
> 	NLMSGERR_ATTR_OFFS	u32	Byte offset to netlink element causing error
> 	NLMSGERR_ATTR_CODE	u32	Subsystem specific error code
> 	NLMSGERR_ATTR_ATTR	u16	Netlink attribute triggering error or missing

I think it would be good if we get a definition to cap the maximum
string length to something reasonable? This can be added in a follow
up patch BTW. Thus, we get people coming back to us and request larger
strings with a reason why they need more room for this.

In general, my main concern with strings is that they can be used in a
more freely way than these u32 offsets and error codes, and
specifically how inconsistent these string will look like across
different netlink subsystems.

Anyway, as long as this is optional (not every subsystem if forced to
use strings) I'm fine with it :).

Re: [RFC 0/3] netlink: extended error reporting

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 7 Apr 2017 21:35:50 +0200

> On Fri, Apr 07, 2017 at 12:20:53PM -0700, David Miller wrote:
> [...]
>> Let's just discuss the UAPI, since people complain we don't talk
>> about that enough :-)  For those playing at home it is three new
>> attributes returned in a netlink ACK when the application asks
>> for the extended response:
>> 
>> 	NLMSGERR_ATTR_MSG	string	Extended error string
>> 	NLMSGERR_ATTR_OFFS	u32	Byte offset to netlink element causing error
>> 	NLMSGERR_ATTR_CODE	u32	Subsystem specific error code
>> 	NLMSGERR_ATTR_ATTR	u16	Netlink attribute triggering error or missing
> 
> I think it would be good if we get a definition to cap the maximum
> string length to something reasonable? This can be added in a follow
> up patch BTW. Thus, we get people coming back to us and request larger
> strings with a reason why they need more room for this.
> 
> In general, my main concern with strings is that they can be used in a
> more freely way than these u32 offsets and error codes, and
> specifically how inconsistent these string will look like across
> different netlink subsystems.
> 
> Anyway, as long as this is optional (not every subsystem if forced to
> use strings) I'm fine with it :).

I have no strong opinions about string length.

In my opinion, I would like to believe that if someone tried to get a
networking patch applied that emitted a rediculously long string then
we would give them feedback about how that is not acceptable.  Right?

I suspect that someone will eventually give us a hard time about the
strings wrt. internationalization. :-) It's solvable at least
partially in userspace I suppose.

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 12:43 -0700, David Miller wrote:
> 
> I have no strong opinions about string length.
> 
> In my opinion, I would like to believe that if someone tried to get a
> networking patch applied that emitted a rediculously long string then
> we would give them feedback about how that is not acceptable.  Right?

Agree with this, I don't really see much point in adding an extra
warning. There's an implicit (multi-KB though) limit as well in the
message size ;-)

> I suspect that someone will eventually give us a hard time about the
> strings wrt. internationalization. :-) It's solvable at least
> partially in userspace I suppose.

I tend to think of the strings more of a debugging aid, but that's a
good point.

Perhaps we should have a macro that has the strings - similar to the
inline function I put into netlink - but if we make that a macro we can
put the strings into a separate section to be able to find them more
easily for later translation, and optionally even omit them entirely
(to satisfy the kernel tinification folks)?

johannes

Re: [RFC 0/3] netlink: extended error reporting

[David Miller]

From: Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
Date: Fri, 07 Apr 2017 21:46:46 +0200

> On Fri, 2017-04-07 at 12:43 -0700, David Miller wrote:
>> I suspect that someone will eventually give us a hard time about the
>> strings wrt. internationalization. :-) It's solvable at least
>> partially in userspace I suppose.
> 
> I tend to think of the strings more of a debugging aid, but that's a
> good point.
> 
> Perhaps we should have a macro that has the strings - similar to the
> inline function I put into netlink - but if we make that a macro we can
> put the strings into a separate section to be able to find them more
> easily for later translation, and optionally even omit them entirely
> (to satisfy the kernel tinification folks)?

One idea.  We use the macro thing to generate a "netlink.pot" file and
then some userland tree can contain the latest netlink.pot and the
translations.

I'm suggesting it this way because whilst putting the netlink.pot file
in the kernel tree is probably OK, putting all the translation there
might not be.

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 12:55 -0700, David Miller wrote:
> 
> One idea.  We use the macro thing to generate a "netlink.pot" file
> and then some userland tree can contain the latest netlink.pot and
> the translations.

Right. For the record - since we just talked about it - I was thinking
of putting it into a special section so that we could easily write a
script to generate the pot file from the kernel binary after doing
"allyesconfig" or collecting it from modules or so.

Thinking of modules though - do we need to adjust the module loading
code to load a new section? I'll have to look into that.

johannes

Re: [RFC 0/3] netlink: extended error reporting

[Pablo Neira Ayuso]

On Fri, Apr 07, 2017 at 11:53:15AM -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri,  7 Apr 2017 20:26:17 +0200
> 
> > So this is my first draft of what we'd talked about at netconf.
> > I'm not super happy with the way we have to pass the extended
> > error struct, but I don't see a way to implement reporting any
> > dynamic information (like error offsets) in any other way.
> > 
> > Alexander Shishkin had a nice way of reporting static extended
> > error data, but that isn't really suitable for reporting the
> > offset or even reporting the broken attribute from nla_parse().
> > 
> > Speaking of nla_parse(), that'll be somewhat complicated to do
> > since we'll have to track the offsets of where we're parsing,
> > but it might be possible since the nlattrs are just pointers
> > into the message, so (optionally?) passing the skb as well can
> > allow us to fill the offset information.
> 
> I like it, nice work.
> 
> I know people want dynamically generated strings and stuff, and we can
> get there, but I prefer that the first thing we commit is super simple.
> 
> Someone gave me a hard time about the fact that we've been talking
> about this idea for years but nothing ever happens.
> 
> I'm tempted to apply this as-is just to show that person that things
> do in fact happen.... eventually :-)

We can just send follow up patches to refine, I think it's a good
start, Johannes?

BTW, for this co-authored effort in designing this:

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks!

Re: [RFC 0/3] netlink: extended error reporting

[Johannes Berg]

On Fri, 2017-04-07 at 21:02 +0200, Pablo Neira Ayuso wrote:
> 
> > I'm tempted to apply this as-is just to show that person that
> > things do in fact happen.... eventually :-)
> 
> We can just send follow up patches to refine, I think it's a good
> start, Johannes?

I guess we can. Like I just said though - I do have a plan to make
nla_parse() work but the week's been exhausting enough that I don't
feel entirely comfortable saying that will definitely work.

OTOH, arguably this is something that seems to work - as evidenced by
the nl80211 changes - for at least some error reporting, so I guess we
can continue to refine it, and the only thing we're really locking in
to is the ABI, which is TLVs now, so that should be safe.

That said, the nl80211 patch really should be bigger, and I've not even
tested this at all yet, so I really need to do that first.

Also, I'd like to point to the genl_info change, does that seem like a
reasonable way to pass it around in genetlink? I'm much less familiar
with the "regular" netlink to say how to pass it around there beyond
what I already did.

johannes