netdev - Re: [PATCH net-next v6] net: sched: Introduce act

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: <20190612191859.GJ31797@unicorn.suse.cz>
Date:   Wed, 12 Jun 2019 21:18:59 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Kevin 'ldir' Darbyshire-Bryant <ldir@...byshire-bryant.me.uk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Paul Blakey <paulb@...lanox.com>,
        John Hurley <john.hurley@...ronome.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        "dcaratti@...hat.com" <dcaratti@...hat.com>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

On Wed, Jun 12, 2019 at 08:56:10PM +0200, Johannes Berg wrote:
> (switching to my personal email)
> 
> > > I can't add these actions with current net-next and iproute-next:
> > > # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> > > Error: NLA_F_NESTED is missing.
> > > We have an error talking to the kernel
> > > 
> > > This also happens with the current post of act_ct and should also
> > > happen with the act_mpls post (thus why Cc'ing John as well).
> > > 
> > > I'm not sure how we should fix this. In theory the kernel can't get
> > > stricter with userspace here, as that breaks user applications as
> > > above, so older actions can't use the more stricter parser. Should we
> > > have some actions behaving one way, and newer ones in a different way?
> > > That seems bad.
> 
> I think you could just fix all of the actions in userspace, since the
> older kernel would allow both with and without the flag, and then from a
> userspace POV it all behaves the same, just the kernel accepts some
> things without the flag for compatibility with older iproute2?
> 
> > > Or maybe all actions should just use nla_parse_nested_deprecated()?
> > > I'm thinking this last. Yet, then the _deprecated suffix may not make
> > > much sense here. WDYT?
> > 
> > Surely for new actions we can require strict validation, there is
> > no existing user space to speak of..  
> 
> That was the original idea.
> 
> > Perhaps act_ctinfo and act_ct
> > got slightly confused with the race you described, but in principle
> > there is nothing stopping new actions from implementing the user space
> > correctly, right?
> 
> There's one potential thing where you have a new command in netlink
> (which thus will use strict validation), but you use existing code in
> userspace to build the netlink message or parts thereof?
> 
> But then again you can just fix that while you test it, and the current
> and older kernel will accept the stricter version for the existing use
> of the existing code too, right?

Userspace can safely set NLA_F_NESTED on every nested attribute as there
are only few places in kernel where nla->type is accessed directly
rather than through nla_type() and those are rather specific (mostly
when attribute type is actually used as an array index). So the best
course of action would be letting userspace always set NLA_F_NESTED.
So kernel can only by strict on newly added attributes but userspace can
(and should) set NLA_F_NESTED always.

The opposite direction (kernel -> userspace) is more tricky as we can
never be sure there isn't some userspace client accessing the type directly
without masking out the flags. Thus kernel can only set NLA_F_NESTED on
new attributes where there cannot be any userspace program used to it
not being set.

Michal