Message-ID: <20180919184652.GL4590@localhost.localdomain>
Date:   Wed, 19 Sep 2018 15:46:52 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

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

Powered by blists - more mailing lists