diff mbox series

[2/2] netlink: export policy in extended ACK

Message ID 20201006142714.3c8b8db03517.I6dae2c514a6abc924ee8b3e2befb0d51b086cf70@changeid
State New
Headers show
Series netlink: export policy on validation failures | expand

Commit Message

Johannes Berg Oct. 6, 2020, 12:32 p.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

Add a new attribute NLMSGERR_ATTR_POLICY to the extended ACK
to advertise the policy, e.g. if an attribute was out of range,
you'll know the range that's permissible.

Add new NL_SET_ERR_MSG_ATTR_POL() and NL_SET_ERR_MSG_ATTR_POL()
macros to set this, since realistically it's only useful to do
this when the bad attribute (offset) is also returned.

Use it in lib/nlattr.c which practically does all the policy
validation.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/linux/netlink.h      | 30 ++++++++++++++++++++----------
 include/net/netlink.h        |  3 +++
 include/uapi/linux/netlink.h |  2 ++
 lib/nlattr.c                 | 35 ++++++++++++++++++-----------------
 net/netlink/af_netlink.c     |  6 ++++++
 net/netlink/policy.c         | 17 +++++++++++++++++
 6 files changed, 66 insertions(+), 27 deletions(-)

Comments

Johannes Berg Oct. 6, 2020, 3:10 p.m. UTC | #1 
Sorry, hat to run out earlier and forgot to comment here.

On Tue, 2020-10-06 at 14:32 +0200, Johannes Berg wrote:
> 
> +	/* the max policy content is currently ~44 bytes for range min/max */
> +	if (err && nlk_has_extack && extack && extack->policy)
> +		tlvlen += 64;

So I'm not really happy with this. I counted 44 bytes content (so 48
bytes for the nested attribute) for the biggest that we have now, but if
we ever implement e.g. dumping out the reject string for NLA_REJECT
(though not sure anyone even uses that?) then it'd be more variable.

I couldn't really come up with any better idea, but I believe we do need
to size the skb fairly well to return the original one ...

The only solution I _could_ think of was to allocate another skb, put
the attribute into it, check the length, and then later append it to the
message ... but that seemed kinda ugly.

Any preferences?

johannes
Jakub Kicinski Oct. 6, 2020, 4:16 p.m. UTC | #2 
On Tue, 06 Oct 2020 17:10:44 +0200 Johannes Berg wrote:
> Sorry, hat to run out earlier and forgot to comment here.
> 
> On Tue, 2020-10-06 at 14:32 +0200, Johannes Berg wrote:
> > 
> > +	/* the max policy content is currently ~44 bytes for range min/max */
> > +	if (err && nlk_has_extack && extack && extack->policy)
> > +		tlvlen += 64;  
> 
> So I'm not really happy with this. I counted 44 bytes content (so 48
> bytes for the nested attribute) for the biggest that we have now, but if
> we ever implement e.g. dumping out the reject string for NLA_REJECT
> (though not sure anyone even uses that?) then it'd be more variable.

I wonder if we should in fact dump the reject string, in this case it
feels like an omission not to have it... although as you say, grep for
reject_message reveals it's completely unused today.

> I couldn't really come up with any better idea, but I believe we do need
> to size the skb fairly well to return the original one ...
> 
> The only solution I _could_ think of was to allocate another skb, put
> the attribute into it, check the length, and then later append it to the
> message ... but that seemed kinda ugly.
> 
> Any preferences?

It'd feel pretty idiomatic for (rt)netlink to have

	netlink_policy_dump_attr_size()

which would calculate the size. That'd cost us probably ~100 LoC?

If that's too much we could at least add a define for this constant,
and WARN_ON_ONCE() in __netlink_policy_dump_write_attr() if the dump
ends up larger?
Johannes Berg Oct. 6, 2020, 5:31 p.m. UTC | #3 
On Tue, 2020-10-06 at 09:16 -0700, Jakub Kicinski wrote:
> On Tue, 06 Oct 2020 17:10:44 +0200 Johannes Berg wrote:

> > Sorry, hat to run out earlier and forgot to comment here.

> > 

> > On Tue, 2020-10-06 at 14:32 +0200, Johannes Berg wrote:

> > > +	/* the max policy content is currently ~44 bytes for range min/max */

> > > +	if (err && nlk_has_extack && extack && extack->policy)

> > > +		tlvlen += 64;  

> > 

> > So I'm not really happy with this. I counted 44 bytes content (so 48

> > bytes for the nested attribute) for the biggest that we have now, but if

> > we ever implement e.g. dumping out the reject string for NLA_REJECT

> > (though not sure anyone even uses that?) then it'd be more variable.

> 

> I wonder if we should in fact dump the reject string, in this case it

> feels like an omission not to have it... although as you say, grep for

> reject_message reveals it's completely unused today.


Yeah. I'm not even sure why I allowed for it. I mean, I added NLA_REJECT
so you could explicitly reject old stuff when you don't have strict
policy enforcement yet (where NLA_UNSPEC is basically the same), but
then never used the string ... *shrug*

Perhaps if somebody wants it they can add it?

> > I couldn't really come up with any better idea, but I believe we do need

> > to size the skb fairly well to return the original one ...

> > 

> > The only solution I _could_ think of was to allocate another skb, put

> > the attribute into it, check the length, and then later append it to the

> > message ... but that seemed kinda ugly.

> > 

> > Any preferences?

> 

> It'd feel pretty idiomatic for (rt)netlink to have

> 

> 	netlink_policy_dump_attr_size()

> 

> which would calculate the size. That'd cost us probably ~100 LoC?


From an API POV I'd agree, but it's ~100 LOC that's effectively
duplicated, and we'd have to maintain both. And if we add something
(like you added the mask) we'd have to add it again there in the size
calculation, and somehow maintain that the two are always in sync.

Feels like a lot of pain for no practical gain?

> If that's too much we could at least add a define for this constant,

> and WARN_ON_ONCE() in __netlink_policy_dump_write_attr() if the dump

> ends up larger?


I didn't feel very comfortable putting a WARN_ON there that effectively
ends up being user-triggerable at will when we made a mistake somewhere,
but will basically never happen on our (developers') machines ...

But hmm, yeah, if we put it into __netlink_policy_dump_write_attr()
rather than netlink_ack() which I had considered, at least there's a
chance we'd exercise the code path in "good" cases an hit the WARN_ON,
and, more importantly, developers will hopefully at least once test
their code and hit it.

So yeah, I think I'll do that.

The other option would be to implement *both* (in a way), and check that
the size returned by netlink_policy_dump_attr_size() matched the actual
size (or at least was bigger), but I still don't really know if I want
to have the duplication?

johannes
diff mbox series

Patch

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index e3e49f0e5c13..666cd0390699 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -68,12 +68,14 @@  netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
  * @_msg: message string to report - don't access directly, use
  *	%NL_SET_ERR_MSG
  * @bad_attr: attribute with error
+ * @policy: policy for a bad attribute
  * @cookie: cookie data to return to userspace (for success)
  * @cookie_len: actual cookie data length
  */
 struct netlink_ext_ack {
 	const char *_msg;
 	const struct nlattr *bad_attr;
+	const struct nla_policy *policy;
 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
 	u8 cookie_len;
 };
@@ -95,21 +97,29 @@  struct netlink_ext_ack {
 #define NL_SET_ERR_MSG_MOD(extack, msg)			\
 	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
 
-#define NL_SET_BAD_ATTR(extack, attr) do {		\
-	if ((extack))					\
+#define NL_SET_BAD_ATTR_POLICY(extack, attr, pol) do {	\
+	if ((extack)) {					\
 		(extack)->bad_attr = (attr);		\
+		(extack)->policy = (pol);		\
+	}						\
 } while (0)
 
-#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do {	\
-	static const char __msg[] = msg;		\
-	struct netlink_ext_ack *__extack = (extack);	\
-							\
-	if (__extack) {					\
-		__extack->_msg = __msg;			\
-		__extack->bad_attr = (attr);		\
-	}						\
+#define NL_SET_BAD_ATTR(extack, attr) NL_SET_BAD_ATTR_POLICY(extack, attr, NULL)
+
+#define NL_SET_ERR_MSG_ATTR_POL(extack, attr, pol, msg) do {	\
+	static const char __msg[] = msg;			\
+	struct netlink_ext_ack *__extack = (extack);		\
+								\
+	if (__extack) {						\
+		__extack->_msg = __msg;				\
+		__extack->bad_attr = (attr);			\
+		__extack->policy = (pol);			\
+	}							\
 } while (0)
 
+#define NL_SET_ERR_MSG_ATTR(extack, attr, msg)		\
+	NL_SET_ERR_MSG_ATTR_POL(extack, attr, NULL, msg)
+
 static inline void nl_set_extack_cookie_u64(struct netlink_ext_ack *extack,
 					    u64 cookie)
 {
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 5a5ff97cc596..6c9b08c0f071 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1946,6 +1946,9 @@  int netlink_policy_dump_get_policy_idx(struct netlink_policy_dump_state *state,
 bool netlink_policy_dump_loop(struct netlink_policy_dump_state *state);
 int netlink_policy_dump_write(struct sk_buff *skb,
 			      struct netlink_policy_dump_state *state);
+int netlink_policy_dump_write_attr(struct sk_buff *skb,
+				   const struct nla_policy *pt,
+				   int nestattr);
 void netlink_policy_dump_free(struct netlink_policy_dump_state *state);
 
 #endif
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index eac8a6a648ea..b9c28c558698 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -129,6 +129,7 @@  struct nlmsgerr {
  * @NLMSGERR_ATTR_COOKIE: arbitrary subsystem specific cookie to
  *	be used - in the success case - to identify a created
  *	object or operation or similar (binary)
+ * @NLMSGERR_ATTR_POLICY: policy for a rejected attribute
  * @__NLMSGERR_ATTR_MAX: number of attributes
  * @NLMSGERR_ATTR_MAX: highest attribute number
  */
@@ -137,6 +138,7 @@  enum nlmsgerr_attrs {
 	NLMSGERR_ATTR_MSG,
 	NLMSGERR_ATTR_OFFS,
 	NLMSGERR_ATTR_COOKIE,
+	NLMSGERR_ATTR_POLICY,
 
 	__NLMSGERR_ATTR_MAX,
 	NLMSGERR_ATTR_MAX = __NLMSGERR_ATTR_MAX - 1
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 80ff9fe83696..cdd78c52c3c4 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -96,8 +96,8 @@  static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
 			continue;
 
 		if (nla_len(entry) < NLA_HDRLEN) {
-			NL_SET_ERR_MSG_ATTR(extack, entry,
-					    "Array element too short");
+			NL_SET_ERR_MSG_ATTR_POL(extack, entry, policy,
+						"Array element too short");
 			return -ERANGE;
 		}
 
@@ -195,8 +195,8 @@  static int nla_validate_range_unsigned(const struct nla_policy *pt,
 		pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
 				    current->comm, pt->type);
 		if (validate & NL_VALIDATE_STRICT_ATTRS) {
-			NL_SET_ERR_MSG_ATTR(extack, nla,
-					    "invalid attribute length");
+			NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+						"invalid attribute length");
 			return -EINVAL;
 		}
 
@@ -208,11 +208,11 @@  static int nla_validate_range_unsigned(const struct nla_policy *pt,
 		bool binary = pt->type == NLA_BINARY;
 
 		if (binary)
-			NL_SET_ERR_MSG_ATTR(extack, nla,
-					    "binary attribute size out of range");
+			NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+						"binary attribute size out of range");
 		else
-			NL_SET_ERR_MSG_ATTR(extack, nla,
-					    "integer out of range");
+			NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+						"integer out of range");
 
 		return -ERANGE;
 	}
@@ -291,8 +291,8 @@  static int nla_validate_int_range_signed(const struct nla_policy *pt,
 	nla_get_range_signed(pt, &range);
 
 	if (value < range.min || value > range.max) {
-		NL_SET_ERR_MSG_ATTR(extack, nla,
-				    "integer out of range");
+		NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+					"integer out of range");
 		return -ERANGE;
 	}
 
@@ -346,8 +346,8 @@  static int validate_nla(const struct nlattr *nla, int maxtype,
 		pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
 				    current->comm, type);
 		if (validate & NL_VALIDATE_STRICT_ATTRS) {
-			NL_SET_ERR_MSG_ATTR(extack, nla,
-					    "invalid attribute length");
+			NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+						"invalid attribute length");
 			return -EINVAL;
 		}
 	}
@@ -355,14 +355,14 @@  static int validate_nla(const struct nlattr *nla, int maxtype,
 	if (validate & NL_VALIDATE_NESTED) {
 		if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
 		    !(nla->nla_type & NLA_F_NESTED)) {
-			NL_SET_ERR_MSG_ATTR(extack, nla,
-					    "NLA_F_NESTED is missing");
+			NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+						"NLA_F_NESTED is missing");
 			return -EINVAL;
 		}
 		if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
 		    pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
-			NL_SET_ERR_MSG_ATTR(extack, nla,
-					    "NLA_F_NESTED not expected");
+			NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+						"NLA_F_NESTED not expected");
 			return -EINVAL;
 		}
 	}
@@ -514,7 +514,8 @@  static int validate_nla(const struct nlattr *nla, int maxtype,
 
 	return 0;
 out_err:
-	NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation");
+	NL_SET_ERR_MSG_ATTR_POL(extack, nla, pt,
+				"Attribute failed policy validation");
 	return err;
 }
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index df675a8e1918..8273d5e70594 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2420,6 +2420,9 @@  void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 		tlvlen += nla_total_size(sizeof(u32));
 	if (nlk_has_extack && extack && extack->cookie_len)
 		tlvlen += nla_total_size(extack->cookie_len);
+	/* the max policy content is currently ~44 bytes for range min/max */
+	if (err && nlk_has_extack && extack && extack->policy)
+		tlvlen += 64;
 
 	if (tlvlen)
 		flags |= NLM_F_ACK_TLVS;
@@ -2452,6 +2455,9 @@  void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 		if (extack->cookie_len)
 			WARN_ON(nla_put(skb, NLMSGERR_ATTR_COOKIE,
 					extack->cookie_len, extack->cookie));
+		if (extack->policy)
+			netlink_policy_dump_write_attr(skb, extack->policy,
+						       NLMSGERR_ATTR_POLICY);
 	}
 
 	nlmsg_end(skb, rep);
diff --git a/net/netlink/policy.c b/net/netlink/policy.c
index bb4e3ffb4924..b19f2a3f2533 100644
--- a/net/netlink/policy.c
+++ b/net/netlink/policy.c
@@ -332,6 +332,23 @@  __netlink_policy_dump_write_attr(struct netlink_policy_dump_state *state,
 	return -ENOBUFS;
 }
 
+/**
+ * netlink_policy_dump_write_attr - write a given attribute policy
+ * @skb: the message skb to write to
+ * @pt: the attribute's policy
+ * @nestattr: the nested attribute ID to use
+ *
+ * Returns: 0 on success, an error code otherwise; -%ENODATA is
+ *	    special, indicating that there's no policy data and
+ *	    the attribute is generally rejected.
+ */
+int netlink_policy_dump_write_attr(struct sk_buff *skb,
+				   const struct nla_policy *pt,
+				   int nestattr)
+{
+	return __netlink_policy_dump_write_attr(NULL, skb, pt, nestattr);
+}
+
 /**
  * netlink_policy_dump_write - write current policy dump attributes
  * @skb: the message skb to write to