[RFC 1/5] netlink: remove NLA_NESTED_COMPAT

[RFC 1/5] netlink: remove NLA_NESTED_COMPAT

[Johannes Berg]

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

This isn't used anywhere, so we might as well get rid of it.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h |  2 --
 lib/nlattr.c          | 11 -----------
 2 files changed, 13 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 318b1ded3833..b680fe365e91 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,7 +172,6 @@ enum {
 	NLA_FLAG,
 	NLA_MSECS,
 	NLA_NESTED,
-	NLA_NESTED_COMPAT,
 	NLA_NUL_STRING,
 	NLA_BINARY,
 	NLA_S8,
@@ -203,7 +202,6 @@ enum {
  *    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,
diff --git a/lib/nlattr.c b/lib/nlattr.c
index bb6fe5ed4ecf..120ad569e13d 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -140,17 +140,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			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.
-- 
2.14.4

[RFC 2/5] netlink: set extack error message in nla_validate()

[Johannes Berg]

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

In nla_parse() we already set this, but it makes sense to
also do it in nla_validate() which already also sets the
bad attribute pointer.

CC: David Ahern <dsahern@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/nlattr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 120ad569e13d..efbd6c1aff29 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
 	int rem;
 
 	nla_for_each_attr(nla, head, len, rem) {
-		int err = validate_nla(nla, maxtype, policy, NULL);
+		static const char _msg[] = "Attribute failed policy validation";
+		const char *msg = _msg;
+		int err = validate_nla(nla, maxtype, policy, &msg);
 
 		if (err < 0) {
+			if (extack)
+				extack->_msg = msg;
 			NL_SET_BAD_ATTR(extack, nla);
 			return err;
 		}
-- 
2.14.4

[RFC 3/5] netlink: combine validate/parse functions

[Johannes Berg]

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

The parse function of course contains validate, but it's
implemented a second time, sharing just the validation
of a single attribute.

Introduce nla_validate_parse() that can be used for both
parsing/validation and only validation, to share code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/nlattr.c | 76 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index efbd6c1aff29..46a6d79cf2d1 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -159,6 +159,37 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 	return 0;
 }
 
+static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
+			      const struct nla_policy *policy,
+			      struct netlink_ext_ack *extack,
+			      struct nlattr **tb)
+{
+	const struct nlattr *nla;
+	int rem;
+
+	nla_for_each_attr(nla, head, len, rem) {
+		static const char _msg[] = "Attribute failed policy validation";
+		const char *msg = _msg;
+		u16 type = nla_type(nla);
+
+		if (policy) {
+			int err = validate_nla(nla, maxtype, policy, &msg);
+
+			if (err < 0) {
+				if (extack)
+					extack->_msg = msg;
+				NL_SET_BAD_ATTR(extack, nla);
+				return err;
+			}
+		}
+
+		if (tb && type > 0 && type <= maxtype)
+			tb[type] = (struct nlattr *)nla;
+	}
+
+	return rem;
+}
+
 /**
  * nla_validate - Validate a stream of attributes
  * @head: head of attribute stream
@@ -177,21 +208,12 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
 		 const struct nla_policy *policy,
 		 struct netlink_ext_ack *extack)
 {
-	const struct nlattr *nla;
 	int rem;
 
-	nla_for_each_attr(nla, head, len, rem) {
-		static const char _msg[] = "Attribute failed policy validation";
-		const char *msg = _msg;
-		int err = validate_nla(nla, maxtype, policy, &msg);
+	rem = nla_validate_parse(head, len, maxtype, policy, extack, NULL);
 
-		if (err < 0) {
-			if (extack)
-				extack->_msg = msg;
-			NL_SET_BAD_ATTR(extack, nla);
-			return err;
-		}
-	}
+	if (rem < 0)
+		return rem;
 
 	return 0;
 }
@@ -245,39 +267,19 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 	      int len, const struct nla_policy *policy,
 	      struct netlink_ext_ack *extack)
 {
-	const struct nlattr *nla;
-	int rem, err;
+	int rem;
 
 	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) {
-			static const char _msg[] = "Attribute failed policy validation";
-			const char *msg = _msg;
-
-			if (policy) {
-				err = validate_nla(nla, maxtype, policy, &msg);
-				if (err < 0) {
-					NL_SET_BAD_ATTR(extack, nla);
-					if (extack)
-						extack->_msg = msg;
-					goto errout;
-				}
-			}
-
-			tb[type] = (struct nlattr *)nla;
-		}
-	}
+	rem = nla_validate_parse(head, len, maxtype, policy, extack, tb);
+	if (rem < 0)
+		return rem;
 
 	if (unlikely(rem > 0))
 		pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n",
 				    rem, current->comm);
 
-	err = 0;
-errout:
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(nla_parse);
 
-- 
2.14.4

[RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

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

In one of my previous patches in this area I introduced code
to pass out just the error message to store in the extack, for
use in NLA_REJECT.

Change this code now to set both the error message and the bad
attribute pointer, and carry around a boolean indicating that
the values have been set.

This will be used in the next patch to allow recursive validation
of nested policies, while preserving the innermost error message
rather than overwriting it with a generic out-level message.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/nlattr.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 46a6d79cf2d1..fecc7b834706 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -70,7 +70,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 
 static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy,
-			const char **error_msg)
+			struct netlink_ext_ack *extack, bool *extack_set)
 {
 	const struct nla_policy *pt;
 	int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
@@ -95,8 +95,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		break;
 
 	case NLA_REJECT:
-		if (pt->validation_data && error_msg)
-			*error_msg = pt->validation_data;
+		if (pt->validation_data && extack && !*extack_set) {
+			*extack_set = true;
+			extack->_msg = pt->validation_data;
+			NL_SET_BAD_ATTR(extack, nla);
+		}
 		return -EINVAL;
 
 	case NLA_FLAG:
@@ -161,24 +164,25 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 
 static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
 			      const struct nla_policy *policy,
-			      struct netlink_ext_ack *extack,
+			      struct netlink_ext_ack *extack, bool *extack_set,
 			      struct nlattr **tb)
 {
 	const struct nlattr *nla;
 	int rem;
 
 	nla_for_each_attr(nla, head, len, rem) {
-		static const char _msg[] = "Attribute failed policy validation";
-		const char *msg = _msg;
 		u16 type = nla_type(nla);
 
 		if (policy) {
-			int err = validate_nla(nla, maxtype, policy, &msg);
+			int err = validate_nla(nla, maxtype, policy,
+					       extack, extack_set);
 
 			if (err < 0) {
-				if (extack)
-					extack->_msg = msg;
-				NL_SET_BAD_ATTR(extack, nla);
+				if (!*extack_set) {
+					*extack_set = true;
+					NL_SET_ERR_MSG_ATTR(extack, nla,
+							    "Attribute failed policy validation");
+				}
 				return err;
 			}
 		}
@@ -208,9 +212,11 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
 		 const struct nla_policy *policy,
 		 struct netlink_ext_ack *extack)
 {
+	bool extack_set = false;
 	int rem;
 
-	rem = nla_validate_parse(head, len, maxtype, policy, extack, NULL);
+	rem = nla_validate_parse(head, len, maxtype, policy,
+				 extack, &extack_set, NULL);
 
 	if (rem < 0)
 		return rem;
@@ -267,11 +273,13 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 	      int len, const struct nla_policy *policy,
 	      struct netlink_ext_ack *extack)
 {
+	bool extack_set = false;
 	int rem;
 
 	memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 
-	rem = nla_validate_parse(head, len, maxtype, policy, extack, tb);
+	rem = nla_validate_parse(head, len, maxtype, policy,
+				 extack, &extack_set, tb);
 	if (rem < 0)
 		return rem;
 
-- 
2.14.4

[RFC 5/5] netlink: allow NLA_NESTED to specify nested policy to validate

[Johannes Berg]

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

Now that we have a validation_data pointer, and the len field in
the policy is unused for NLA_NESTED, we can allow using them both
to have nested validation. This can be nice in code, although we
still have to use nla_parse_nested() or similar which would also
take a policy; however, it also serves as documentation in the
policy without requiring a look at the code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h | 10 ++++++++--
 lib/nlattr.c          | 17 +++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b680fe365e91..6efa25a004f5 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -200,8 +200,10 @@ enum {
  *    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           Length verification is done by checking len of
+ *                         nested header (or empty); len field is used if
+ *                         validation_data is also used, for the max attr
+ *                         number in the nested policy.
  *    NLA_U8, NLA_U16,
  *    NLA_U32, NLA_U64,
  *    NLA_S8, NLA_S16,
@@ -224,6 +226,10 @@ enum {
  *    NLA_REJECT           This attribute is always rejected and validation data
  *                         may point to a string to report as the error instead
  *                         of the generic one in extended ACK.
+ *    NLA_NESTED           Points to a nested policy to validate, must also set
+ *                         `len' to the max attribute number.
+ *                         Note that nla_parse() will validate, but of course not
+ *                         parse, the nested sub-policies.
  *    All other            Unused
  *
  * Example:
diff --git a/lib/nlattr.c b/lib/nlattr.c
index fecc7b834706..4c8c4fffb20d 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -68,6 +68,11 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 	return 0;
 }
 
+static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
+			      const struct nla_policy *policy,
+			      struct netlink_ext_ack *extack, bool *extack_set,
+			      struct nlattr **tb);
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy,
 			struct netlink_ext_ack *extack, bool *extack_set)
@@ -149,6 +154,18 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		 */
 		if (attrlen == 0)
 			break;
+		if (attrlen < NLA_HDRLEN)
+			return -ERANGE;
+		if (pt->validation_data) {
+			int err;
+
+			err = nla_validate_parse(nla_data(nla), nla_len(nla),
+						 pt->len, pt->validation_data,
+						 extack, extack_set, NULL);
+			if (err < 0)
+				return err;
+		}
+		break;
 	default:
 		if (pt->len)
 			minlen = pt->len;
-- 
2.14.4

Re: [RFC 2/5] netlink: set extack error message in nla_validate()

[David Ahern]

On 9/18/18 6:12 AM, Johannes Berg wrote:
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 120ad569e13d..efbd6c1aff29 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
>  	int rem;
>  
>  	nla_for_each_attr(nla, head, len, rem) {
> -		int err = validate_nla(nla, maxtype, policy, NULL);
> +		static const char _msg[] = "Attribute failed policy validation";
> +		const char *msg = _msg;
> +		int err = validate_nla(nla, maxtype, policy, &msg);
>  
>  		if (err < 0) {
> +			if (extack)
> +				extack->_msg = msg;
>  			NL_SET_BAD_ATTR(extack, nla);
>  			return err;
>  		}
> 

I take it this set is on top of another set - the NLA_REJECT?

Re: [RFC 1/5] netlink: remove NLA_NESTED_COMPAT

[David Ahern]

On 9/18/18 6:12 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This isn't used anywhere, so we might as well get rid of it.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/netlink.h |  2 --
>  lib/nlattr.c          | 11 -----------
>  2 files changed, 13 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [RFC 2/5] netlink: set extack error message in nla_validate()

[Johannes Berg]

On Tue, 2018-09-18 at 10:18 -0700, David Ahern wrote:
> On 9/18/18 6:12 AM, Johannes Berg wrote:
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index 120ad569e13d..efbd6c1aff29 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
> >  	int rem;
> >  
> >  	nla_for_each_attr(nla, head, len, rem) {
> > -		int err = validate_nla(nla, maxtype, policy, NULL);
> > +		static const char _msg[] = "Attribute failed policy validation";
> > +		const char *msg = _msg;
> > +		int err = validate_nla(nla, maxtype, policy, &msg);
> >  
> >  		if (err < 0) {
> > +			if (extack)
> > +				extack->_msg = msg;
> >  			NL_SET_BAD_ATTR(extack, nla);
> >  			return err;
> >  		}
> > 
> 
> I take it this set is on top of another set - the NLA_REJECT?

Yeah, these two:

https://patchwork.ozlabs.org/project/netdev/list/?series=65982

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Marcelo Ricardo Leitner]

On Tue, Sep 18, 2018 at 03:12:11PM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> In one of my previous patches in this area I introduced code
> to pass out just the error message to store in the extack, for
> use in NLA_REJECT.
> 
> Change this code now to set both the error message and the bad
> attribute pointer, and carry around a boolean indicating that
> the values have been set.
> 
> This will be used in the next patch to allow recursive validation
> of nested policies, while preserving the innermost error message
> rather than overwriting it with a generic out-level message.

Did you consider indicating the message level, and only overwrite the
message that is already in there if the new message level is higher
than the current one?

This way the first to set an Error message will have it, and Warning
messages would be overwritten by such if needed. But it also would
cause the first warning to be held, and not the last one, as it does
today. We want the first error, but the last warning otherwise.

It would not be possible to overwrite if new_msglvl >= cur_msglvl
because then it would trigger the initial issue again, so some extra
logic would be needed to solve this.

  Marcelo

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Jiri Benc]

On Tue, 18 Sep 2018 15:12:11 +0200, Johannes Berg wrote:
>  static int validate_nla(const struct nlattr *nla, int maxtype,
>  			const struct nla_policy *policy,
> -			const char **error_msg)
> +			struct netlink_ext_ack *extack, bool *extack_set)

Can't the extack_set be included in the struct netlink_ext_ack? One less
parameter to pass around and the NL_SET_* macros could check this on
their own.

This way, it can be also easily turned to a more complex mechanism,
such as the one Marcelo proposed.

Thanks,

 Jiri

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 11:10 +0200, Jiri Benc wrote:
> On Tue, 18 Sep 2018 15:12:11 +0200, Johannes Berg wrote:
> >  static int validate_nla(const struct nlattr *nla, int maxtype,
> >  			const struct nla_policy *policy,
> > -			const char **error_msg)
> > +			struct netlink_ext_ack *extack, bool *extack_set)
> 
> Can't the extack_set be included in the struct netlink_ext_ack? One less
> parameter to pass around and the NL_SET_* macros could check this on
> their own.

No, I don't think it can or should.

For one, having the NL_SET_* macros check it on their own will already
not work - as we discussed over in the NLA_REJECT thread, we do need to
be able to override the data, e.g. if somebody does

NL_SET_ERR_MSG(extack, "warning: deprecated command");
err = nla_parse(..., extack);

and nla_parse() sets a new message. Thus, hiding all the logic in there
already will not work, and is also IMHO rather unexpected. Normally,
*later* messages should win, not *earlier* ones - and that doesn't
require any tracking, just setting them unconditionally.

It's just a side effect of how we do the recursive validation here that
we want *earlier* messages to win (but only within this code piece - if
a previous message was set, we want it to be overwritten by nla_parse!).

It might be possible to do this differently, in theory, but all the ways
I've tried to come up with so far made the code vastly more complex.

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 00:37 -0300, Marcelo Ricardo Leitner wrote:

> Did you consider indicating the message level, and only overwrite the
> message that is already in there if the new message level is higher
> than the current one?

Hmm, no, I guess I didn't - I'm not even sure I understand what you're
saying.

This code in itself generates no "warning" messages; that was just a
construct we discussed in the NLA_REJECT thread, e.g. if you say (like I
just also wrote in my reply to Jiri):

	NL_SET_ERR_MSG(extack, "warning: deprecated command");
	err = nla_parse(..., extack);
	if (err)
		return err;
	/* do something */
	return 0;

Here you could consider the message there a warning that's transported
out even if we return 0, but if we return with a failure from
nla_parse() (or nla_validate instead if you wish), then that failure
message "wins".

> This way the first to set an Error message will have it, and Warning
> messages would be overwritten by such if needed. But it also would
> cause the first warning to be held, and not the last one, as it does
> today. We want the first error, but the last warning otherwise.
> 
> It would not be possible to overwrite if new_msglvl >= cur_msglvl
> because then it would trigger the initial issue again, so some extra
> logic would be needed to solve this.

That sounds way more complex than what I'm doing now?

Note, like I said above, this isn't *generic* in any way. This code here
will only ever set error messages that should "win".

I suppose we could - technically - make that generic, in that we could
have both

  NLA_SET_WARN_MSG(extack, "...");
  NLA_SET_ERR_MSG(extack, "...");

and keep track of warning vs. error; however, just like my first version
of the NLA_REJECT patch, that would break existing code.

I also don't think that we actually *need* this complexity in general.
It should almost always be possible (and realistically, pretty easy) to
structure your code in a way that warning messages only go out if no
error message overwrites them. The only reason we were ever even
discussing this was that in NLA_REJECT I missed the fact that somebody
could've set a message before and thus would keep it rather than
overwrite it, which was a change in behaviour.

Now, with this patch, all I'm doing is changing the internal behaviour
of nla_parse/nla_validate - externally, it still overwrites any existing
message if an error occurs, but internally it keeps the inner-most
error.

Why is this? Consider this:

static const struct nla_policy inner_policy[] = {
	[INNER_FLAG] = { .type = NLA_REJECT,
                         .validation_data = "must not set this flag" }
};

static const struct nla_policy outer_policy[] = {
	[OUTER_NESTING] = { .type = NLA_NESTED, .len = INNER_MAX,
                            .validation_data = inner_policy,
};

Now if you invoke nla_parse/nla_validate with a message like this

[ OUTER_NESTING => [ INNER_FLAG, ... ], ... ]

you'd get "must not set this flag" with the error offset pointing to
that; if I didn't do this construction here with inner messages winning,
you'd get "Attribute failed policy validation" with the error offset
pointing to the "OUTER_NESTING" attribute, that's pretty useless.

>From an external API POV though, nla_validate/nla_parse will continue to
unconditionally overwrite any existing "warning" messages with errors,
if such occurred. They just won't overwrite their own messages when
returning from a nested policy validation.

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Jiri Benc]

On Wed, 19 Sep 2018 11:15:25 +0200, Johannes Berg wrote:
> For one, having the NL_SET_* macros check it on their own will already
> not work - as we discussed over in the NLA_REJECT thread, we do need to
> be able to override the data, e.g. if somebody does
> 
> NL_SET_ERR_MSG(extack, "warning: deprecated command");
> err = nla_parse(..., extack);
> 
> and nla_parse() sets a new message. Thus, hiding all the logic in there
> already will not work, and is also IMHO rather unexpected. Normally,
> *later* messages should win, not *earlier* ones - and that doesn't
> require any tracking, just setting them unconditionally.
> 
> It's just a side effect of how we do the recursive validation here that
> we want *earlier* messages to win (but only within this code piece - if
> a previous message was set, we want it to be overwritten by nla_parse!).

Fair enough.

> It might be possible to do this differently, in theory, but all the ways
> I've tried to come up with so far made the code vastly more complex.

Wouldn't still make sense to store the flag in the struct
netlink_ext_ack, though? The way the parameters are passed around in
this patch looks ugly. And adding more users of the flag later (there
may be other cases when we want the earlier messages to be preserved)
would mean adding parameters all around, while the flag in the struct
would be readily available.

I don't have a strong opinion on this, just the patch seems to be
inelegant.

 Jiri

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Jiri Benc]

On Wed, 19 Sep 2018 11:25:17 +0200, Johannes Berg wrote:
> Now, with this patch, all I'm doing is changing the internal behaviour
> of nla_parse/nla_validate - externally, it still overwrites any existing
> message if an error occurs, but internally it keeps the inner-most
> error.

Ah, okay, that answers my question about putting the flag into the
ext_ack struct, too.

It may still be worthwhile to have a mechanism for prioritizing certain
extack messages over another ones but it's clearly out of scope of this
patchset.

The patchset looks good to me. Just include the description you just
wrote in the commit message :-)

Thanks!

 Jiri

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 11:28 +0200, Jiri Benc wrote:

> > It might be possible to do this differently, in theory, but all the ways
> > I've tried to come up with so far made the code vastly more complex.
> 
> Wouldn't still make sense to store the flag in the struct
> netlink_ext_ack, though?

Does it make sense to store a flag there that only has a single,
localized, user? I'd say no.

> The way the parameters are passed around in
> this patch looks ugly. 

Yeah, it's not the best in some ways.

I considered having a cleared new extack on the stack in the outer-most
call (nla_parse/nla_validate), and then we can "set if not already set",
and at the end unconditionally copy/set to the real one... But then I
either need to duplicate that code in both, or pass another argument to
nla_validate_parse() anyway to indicate whether to do this or not (since
the inner calls to it shouldn't, since that would defeat the purpose),
or add another level of function call indirection?

I'm not really sure any of these are better ... and more complex, in
some ways, since we have to copy all the data around.

> And adding more users of the flag later (there
> may be other cases when we want the earlier messages to be preserved)
> would mean adding parameters all around, while the flag in the struct
> would be readily available.

I can't really think of any other users of such a thing?

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Marcelo Ricardo Leitner]

On Wed, Sep 19, 2018 at 11:25:17AM +0200, Johannes Berg wrote:
> On Wed, 2018-09-19 at 00:37 -0300, Marcelo Ricardo Leitner wrote:
> 
> > Did you consider indicating the message level, and only overwrite the
> > message that is already in there if the new message level is higher
> > than the current one?
> 
> Hmm, no, I guess I didn't - I'm not even sure I understand what you're
> saying.
> 
> This code in itself generates no "warning" messages; that was just a
> construct we discussed in the NLA_REJECT thread, e.g. if you say (like I
> just also wrote in my reply to Jiri):
> 
> 	NL_SET_ERR_MSG(extack, "warning: deprecated command");
> 	err = nla_parse(..., extack);
> 	if (err)
> 		return err;
> 	/* do something */
> 	return 0;
> 
> Here you could consider the message there a warning that's transported
> out even if we return 0, but if we return with a failure from
> nla_parse() (or nla_validate instead if you wish), then that failure
> message "wins".

Agree. This is the core issue here, IMHO. Once out of the context that
set the message, we have no way of knowing if we can nor should
overwrite the message that is already in there.

> 
> > This way the first to set an Error message will have it, and Warning
> > messages would be overwritten by such if needed. But it also would
> > cause the first warning to be held, and not the last one, as it does
> > today. We want the first error, but the last warning otherwise.
> > 
> > It would not be possible to overwrite if new_msglvl >= cur_msglvl
> > because then it would trigger the initial issue again, so some extra
> > logic would be needed to solve this.
> 
> That sounds way more complex than what I'm doing now?

Yes but hopefully it would a clearer way of how it is/should be handled.

> 
> Note, like I said above, this isn't *generic* in any way. This code here
> will only ever set error messages that should "win".
> 
> I suppose we could - technically - make that generic, in that we could
> have both
> 
>   NLA_SET_WARN_MSG(extack, "...");
>   NLA_SET_ERR_MSG(extack, "...");

I like this.

> 
> and keep track of warning vs. error; however, just like my first version
> of the NLA_REJECT patch, that would break existing code.

Hm, I may have missed something but I think the discussion in there
was for a different context. For an extack msg to be set by
when validate_nla() call returns on nla_parse(), the previous message
had to be a "warning" because otherwise the parsing wouldn't be even
attempted. So in that case, we are safe to simply overwrite it.

While for the situation you are describing here, it will set a generic
error message in case the inner code didn't do it.

Using the semantics of NLA_SET_WARN_MSG and ERR, then WARN would
replace other WARNs but not ERRs, and ERR would replace other WARNs
too but not other ERRs. All we need to do handle this is a bit in
extack saying if the message is considered a warning or not, or an
error/fatal message or not.

> 
> I also don't think that we actually *need* this complexity in general.
> It should almost always be possible (and realistically, pretty easy) to
> structure your code in a way that warning messages only go out if no
> error message overwrites them. The only reason we were ever even
> discussing this was that in NLA_REJECT I missed the fact that somebody
> could've set a message before and thus would keep it rather than
> overwrite it, which was a change in behaviour.

Okay but we have split parsing of netlink messages and this could be
useful in there too:
In cls_api.c, tc_new_tfilter() calls nmmsg_parse() and do some
processing, and then handle it to a classifier. cls_flower, for
example, will then do another parsing. If, for whatever reason, flower
failed and did not set an extack msg, tc_new_tfilter() could set a
default error message, but at this moment we can't tell if the msg
already set was just a warning from the first parsing (or any other
code before engaging flower) (which we can overwrite) or if it a
message that flower set (which we should not overwrite).

Hopefully my description clear.. 8-)

I think this is the same situation as with the nested parsing you're
proposing.

Currently it (tc_new_tfilter) doesn't set any default error message,
so this issue is/was not noticed.

> 
> Now, with this patch, all I'm doing is changing the internal behaviour
> of nla_parse/nla_validate - externally, it still overwrites any existing
> message if an error occurs, but internally it keeps the inner-most
> error.
> 
> Why is this? Consider this:
> 
> static const struct nla_policy inner_policy[] = {
> 	[INNER_FLAG] = { .type = NLA_REJECT,
>                          .validation_data = "must not set this flag" }
> };
> 
> static const struct nla_policy outer_policy[] = {
> 	[OUTER_NESTING] = { .type = NLA_NESTED, .len = INNER_MAX,
>                             .validation_data = inner_policy,
> };
> 
> Now if you invoke nla_parse/nla_validate with a message like this
> 
> [ OUTER_NESTING => [ INNER_FLAG, ... ], ... ]
> 
> you'd get "must not set this flag" with the error offset pointing to
> that; if I didn't do this construction here with inner messages winning,
> you'd get "Attribute failed policy validation" with the error offset
> pointing to the "OUTER_NESTING" attribute, that's pretty useless.

Yes, agree.

> 
> From an external API POV though, nla_validate/nla_parse will continue to
> unconditionally overwrite any existing "warning" messages with errors,
> if such occurred. They just won't overwrite their own messages when
> returning from a nested policy validation.

Yes, that's fine. I'm not objecting to the behavior. It just feels
that extack handling is getting tricky by the day and we could seize
the moment to improve it while we can.

  Marcelo

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 15:46 -0300, Marcelo Ricardo Leitner wrote:

> > 	NL_SET_ERR_MSG(extack, "warning: deprecated command");
> > 	err = nla_parse(..., extack);
> > 	if (err)
> > 		return err;
> > 	/* do something */
> > 	return 0;
> > 
> > Here you could consider the message there a warning that's transported
> > out even if we return 0, but if we return with a failure from
> > nla_parse() (or nla_validate instead if you wish), then that failure
> > message "wins".
> 
> Agree. This is the core issue here, IMHO. Once out of the context that
> set the message, we have no way of knowing if we can nor should
> overwrite the message that is already in there.

True.

I'm not really sure I'd go as far as calling it an issue though.

IMHO, we really get into this situation if the code is badly structured
to start with. Taking the above code, if you write it as

	err = nla_parse(...);
	if (err)
		return err;
	/* do something */
	NLA_SET_ERR_MSG(extack, "warning: deprecated command");
	return 0;

instead, then you have no such problems.

Well, perhaps you do, if "/* do something */" might *also* set the
message, but basically you have to statically decide which one wins by
ordering the code correctly.

I'm still not convinced, btw, that we actually have any instance in the
kernel today of the issue we've been discussing - namely that some code
does something like the original quoted code at the top of the email.

> > I suppose we could - technically - make that generic, in that we could
> > have both
> > 
> >   NLA_SET_WARN_MSG(extack, "...");
> >   NLA_SET_ERR_MSG(extack, "...");
> 
> I like this.

I'm not really sure what for though :-)

FWIW, if you do think that there's a need for distinguishing this, then
I'd argue that perhaps the right way to address this would be to extend
this all the way to userspace and have two separate attributes for
errors and warnings in the extended ACK message?

> > and keep track of warning vs. error; however, just like my first version
> > of the NLA_REJECT patch, that would break existing code.
> 
> Hm, I may have missed something but I think the discussion in there
> was for a different context. For an extack msg to be set by
> when validate_nla() call returns on nla_parse(), the previous message
> had to be a "warning" because otherwise the parsing wouldn't be even
> attempted. So in that case, we are safe to simply overwrite it.

Yes, arguably, if you really had an "error" then you'd probably have
never gotten to the parsing. It's possible - although sort of stupid -
to write this code too though:

NL_SET_ERR_MSG(extack, "error: unsupported command");
err = nla_parse(...);
return err ? : -EOPNOTSUPP;

Which one should win in that case?

Again, in my opinion we've got enough flexibility if we let
nla_parse/nla_validate behave as today (mostly, nla_validate doesn't set
a message and I'd like to change that, it does set the error attribute
pointer/offset) and let the calling code sort it out.

In many cases nla_parse() will be called by generic code (e.g.
genetlink) and you'd never get into this situation. Once you're past
that, you  can do whatever you like.

> While for the situation you are describing here, it will set a generic
> error message in case the inner code didn't do it.

Yes, but that's not a change in semantics as far as the caller of
nla_parse/nla_validate is concerned - it'll continue to unconditionally
set a message if an error occurred, only the internal behaviour as to
which message seems more relevant is at issue, and the whole recursion
thing and avoiding an outer one overwriting an inner one is IMHO more
useful because that's the more specific error.

> Using the semantics of NLA_SET_WARN_MSG and ERR, then WARN would
> replace other WARNs but not ERRs, and ERR would replace other WARNs
> too but not other ERRs. All we need to do handle this is a bit in
> extack saying if the message is considered a warning or not, or an
> error/fatal message or not.

I'm still not really sure what the use case for a warning is, so not
sure I can really comment on this.

> Okay but we have split parsing of netlink messages and this could be
> useful in there too:
> In cls_api.c, tc_new_tfilter() calls nmmsg_parse() and do some
> processing, and then handle it to a classifier. cls_flower, for
> example, will then do another parsing. If, for whatever reason, flower
> failed and did not set an extack msg, tc_new_tfilter() could set a
> default error message, but at this moment we can't tell if the msg
> already set was just a warning from the first parsing (or any other
> code before engaging flower) (which we can overwrite) or if it a
> message that flower set (which we should not overwrite).
> 
> Hopefully my description clear.. 8-)
> 
> I think this is the same situation as with the nested parsing you're
> proposing.

Yes, I admit that's the same, just not part of pure policy checking, but
open-coded (in a way).

> Currently it (tc_new_tfilter) doesn't set any default error message,
> so this issue is/was not noticed.

True.

Except that I'd still say that tc_new_tfilter() can very well assume
that nothing has set a message before it ran, it's invoked directly by
the core netlink code after all.

So IMHO we don't have an issue here because there aren't arbitrary
callers of this and it can't know what the state is; it does in fact
know very well what the state is when it's called.

With nla_validate/nla_parse that have far more callers we can't be as
certain, although again - I doubt we actually have places today that
would run into this situation (given how all this stuff is still fairly
new).

> > From an external API POV though, nla_validate/nla_parse will continue to
> > unconditionally overwrite any existing "warning" messages with errors,
> > if such occurred. They just won't overwrite their own messages when
> > returning from a nested policy validation.
> 
> Yes, that's fine. I'm not objecting to the behavior. It just feels
> that extack handling is getting tricky by the day and we could seize
> the moment to improve it while we can.

Ok, I guess that's fair. I don't think this actually changes the
"trickiness" of extack handling though; it's purely an internal detail
of nla_validate/nla_parse due to me wanting to add recursion; from an
external POV nothing changed.

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Marcelo Ricardo Leitner]

On Wed, Sep 19, 2018 at 09:19:31PM +0200, Johannes Berg wrote:
> On Wed, 2018-09-19 at 15:46 -0300, Marcelo Ricardo Leitner wrote:
> 
> > > 	NL_SET_ERR_MSG(extack, "warning: deprecated command");
> > > 	err = nla_parse(..., extack);
> > > 	if (err)
> > > 		return err;
> > > 	/* do something */
> > > 	return 0;
> > > 
> > > Here you could consider the message there a warning that's transported
> > > out even if we return 0, but if we return with a failure from
> > > nla_parse() (or nla_validate instead if you wish), then that failure
> > > message "wins".
> > 
> > Agree. This is the core issue here, IMHO. Once out of the context that
> > set the message, we have no way of knowing if we can nor should
> > overwrite the message that is already in there.
> 
> True.
> 
> I'm not really sure I'd go as far as calling it an issue though.

Ok, s/issue/matter in question/.

...
> > > I suppose we could - technically - make that generic, in that we could
> > > have both
> > > 
> > >   NLA_SET_WARN_MSG(extack, "...");
> > >   NLA_SET_ERR_MSG(extack, "...");
> > 
> > I like this.
> 
> I'm not really sure what for though :-)

Hehe :-)

> 
> FWIW, if you do think that there's a need for distinguishing this, then
> I'd argue that perhaps the right way to address this would be to extend
> this all the way to userspace and have two separate attributes for
> errors and warnings in the extended ACK message?

Likely, yes. And perhaps even support multiple messages. That way we
could, for example, parse all attributes and return a list of the all
the offending ones, instead of returning one at a time. Net benefit?
Not clear.. over engineering? Possibly.

I agree with you that in general we should not need this.

> > While for the situation you are describing here, it will set a generic
> > error message in case the inner code didn't do it.
> 
> Yes, but that's not a change in semantics as far as the caller of
> nla_parse/nla_validate is concerned - it'll continue to unconditionally
> set a message if an error occurred, only the internal behaviour as to
> which message seems more relevant is at issue, and the whole recursion
> thing and avoiding an outer one overwriting an inner one is IMHO more
> useful because that's the more specific error.

That's fine. My point here was only to clarify the use case, which
would be used in other places too (like in the example below).

> 
> > Using the semantics of NLA_SET_WARN_MSG and ERR, then WARN would
> > replace other WARNs but not ERRs, and ERR would replace other WARNs
> > too but not other ERRs. All we need to do handle this is a bit in
> > extack saying if the message is considered a warning or not, or an
> > error/fatal message or not.
> 
> I'm still not really sure what the use case for a warning is, so not
> sure I can really comment on this.

Good point. From iproute POV, a warning is a non-fatal message. From
an user perspective, that probably translates are nothing because in
the end the command actually worked. :-)

Seriously, I do think it's more of a hint for developers than anyone
else.

> 
> > Okay but we have split parsing of netlink messages and this could be
> > useful in there too:
> > In cls_api.c, tc_new_tfilter() calls nmmsg_parse() and do some
> > processing, and then handle it to a classifier. cls_flower, for
> > example, will then do another parsing. If, for whatever reason, flower
> > failed and did not set an extack msg, tc_new_tfilter() could set a
> > default error message, but at this moment we can't tell if the msg
> > already set was just a warning from the first parsing (or any other
> > code before engaging flower) (which we can overwrite) or if it a
> > message that flower set (which we should not overwrite).
> > 
> > Hopefully my description clear.. 8-)
> > 
> > I think this is the same situation as with the nested parsing you're
> > proposing.
> 
> Yes, I admit that's the same, just not part of pure policy checking, but
> open-coded (in a way).
> 
> > Currently it (tc_new_tfilter) doesn't set any default error message,
> > so this issue is/was not noticed.
> 
> True.
> 
> Except that I'd still say that tc_new_tfilter() can very well assume
> that nothing has set a message before it ran, it's invoked directly by
> the core netlink code after all.
> 
> So IMHO we don't have an issue here because there aren't arbitrary
> callers of this and it can't know what the state is; it does in fact
> know very well what the state is when it's called.

Good point.

> 
> With nla_validate/nla_parse that have far more callers we can't be as
> certain, although again - I doubt we actually have places today that
> would run into this situation (given how all this stuff is still fairly
> new).
> 
> > > From an external API POV though, nla_validate/nla_parse will continue to
> > > unconditionally overwrite any existing "warning" messages with errors,
> > > if such occurred. They just won't overwrite their own messages when
> > > returning from a nested policy validation.
> > 
> > Yes, that's fine. I'm not objecting to the behavior. It just feels
> > that extack handling is getting tricky by the day and we could seize
> > the moment to improve it while we can.
> 
> Ok, I guess that's fair. I don't think this actually changes the
> "trickiness" of extack handling though; it's purely an internal detail
> of nla_validate/nla_parse due to me wanting to add recursion; from an
> external POV nothing changed.
> 
> johannes
> 

Thanks,
  Marcelo

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 18:10 -0300, Marcelo Ricardo Leitner wrote:

> > FWIW, if you do think that there's a need for distinguishing this, then
> > I'd argue that perhaps the right way to address this would be to extend
> > this all the way to userspace and have two separate attributes for
> > errors and warnings in the extended ACK message?
> 
> Likely, yes. And perhaps even support multiple messages. That way we
> could, for example, parse all attributes and return a list of the all
> the offending ones, instead of returning one at a time. Net benefit?
> Not clear.. over engineering? Possibly.

Not sure I'd want to continue parsing after hitting something that's
considered garbage? It might be that the attribute length field is
corrupted and the data is actually fine, or something, and then
continuing to parse would just lead to more errors over and over again.

Also, I'd worry about being able to "blow up" the message, if we get a
text & bad attribute for everything that's wrong, it's pretty easy to
create a sort of "message amplification" and we'd probably have to
defend against that in terms of limiting memory allocation for all the
possible errors etc. Not sure I'd want to go there.

> I agree with you that in general we should not need this.

:-)

> > I'm still not really sure what the use case for a warning is, so not
> > sure I can really comment on this.
> 
> Good point. From iproute POV, a warning is a non-fatal message. From
> an user perspective, that probably translates are nothing because in
> the end the command actually worked. :-)
> 
> Seriously, I do think it's more of a hint for developers than anyone
> else.

I guess we also have the (ratelimited) messages from the kernel, like
the one saying you have extra bytes after your attributes at the end of
the message. Not sure which makes more sense, depends on the specific
case you'd use this in I guess.


Anyway - we got into this discussion because of all the extra recursion
stuff I was adding. With the change suggested by David we don't need
that now at all, so I guess it'd be better to propose a patch if you (or
perhaps I will see a need later) need such a facility for multiple
messages or multiple message levels?

johannes

Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

[Marcelo Ricardo Leitner]

On Thu, Sep 20, 2018 at 10:14:10AM +0200, Johannes Berg wrote:
> Anyway - we got into this discussion because of all the extra recursion
> stuff I was adding. With the change suggested by David we don't need
> that now at all, so I guess it'd be better to propose a patch if you (or
> perhaps I will see a need later) need such a facility for multiple
> messages or multiple message levels?

Yep! 

Thanks,
  Marcelo