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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180919112829.647896cf@redhat.com>
Date:   Wed, 19 Sep 2018 11:28:29 +0200
From:   Jiri Benc <jbenc@...hat.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, 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