Re: [PATCH 1/2] netlink: add NLA_REJECT policy type

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Thu, Sep 13, 2018 at 11:27:42PM +0200, Michal Kubecek wrote:
> On Thu, Sep 13, 2018 at 04:30:04PM -0300, Marcelo Ricardo Leitner wrote:
> > On Thu, Sep 13, 2018 at 10:46:02AM +0200, Johannes Berg wrote:
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > In some situations some netlink attributes may be used for output
> > > only (kernel->userspace) or may be reserved for future use. It's
> > > then helpful to be able to prevent userspace from using them in
> > > messages sent to the kernel, since they'd otherwise be ignored and
> > > any future will become impossible if this happens.
> > 
> > I don't follow, what's the issue with simply ignoring such attributes?
> 
> A bit artificial example but I can't come with somethin better at the
> moment: let's say newer kernel and iproute2 allow something like
> 
>   ip link del <condition1> <condition2>
> 
> and you run new ip with older kernel which only supports <condition1>.
> You don't really want kernel to silently ignore the second condition.
> Or e.g. when adding a netfilter rule, you wouldn't want kernel to ignore
> parts of the rule it does not understand.

Oh, I thought these are different issues. How would NLA_REJECT protect
against this, by reserving <condition2> in advance?

This example looks more like a case for NLM_F_STRICT:
https://lwn.net/Articles/661266/
On which the user space would explicitly say "please reject this if
you don't get it all", but it was rejected back then.

> 
> I must admit I'm not sure if there is really need for having reserved
> attributes with netlink. Maybe e.g. when we want to share part of
> (numeric) attribute types between different message types. Anyway, we
> have the same problem with attributes higher than maximum; NLA_REJECT
> wouldn't help with this but the discussion also touched the topic.

Yes, agree.

> 
> > > Add NLA_REJECT to the policy which does nothing but reject (with
> > > EINVAL) validation of any messages containing this attribute.
> > > Allow for returning a specific extended ACK error message in the
> > > validation_data pointer.
> > 
> > This has potential to create confusion because we can't use it on
> > {output,reserved} attributes that are already there (as we must always
> > ignore them now) and we will end up with a mix of it.
> 
> We can return -EINVAL even now, we just need to add a check after
> nla_parse() wrapper returns, e.g. here: (lines 314-320)
> 
>   https://github.com/mkubecek/ethnl/blob/master/kernel/0019-ethtool-implement-SET_PARAMS-message.patch#L314

That's new code, it's okay. Won't break anyone's setup.

> 
> It would be easier and IMHO cleaner if I could simply list these "read
> only attributes" with NLA_REJECT policy for "set" request.

Not that I'm against this. Point was fields that are considered output
only today are probably being silently ignored, and we can't change
them to be NLA_REJECT as it would break user applications. Then we
will have fields that are rejected, and those old that are not. In the
long run, nearly all output fields would be marked as NLA_REJECT,
okay.

Then I ask my first question again: why reject these? They are not
hurting anything, are they?  It's different from your example I think.
In there, the extra information which was ignored leads to a
different behavior.

Maybe it would be better to have NLA_IGNORE instead? </idea>

  Marcelo