Message-ID: <20190612143458.0b6fe526@cakuba.netronome.com>
Date:   Wed, 12 Jun 2019 14:34:58 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     Johannes Berg <johannes@...solutions.net>,
        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, 12 Jun 2019 21:18:59 +0200, Michal Kubecek wrote:
> 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.

Agreed, so it's just the slight inconsistency in the dumps, which I'd
think is a fair price to pay here.  Old user space won't recognize the
new attributes, anyway, so doesn't matter what flags they have..

Powered by blists - more mailing lists