Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

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

On Wed, Jun 12, 2019 at 02:34:58PM -0700, Jakub Kicinski wrote:
> 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..

Thanks for your inputs. In summary, being able to do extra validations
for new actions is worth the inconsitency then.

  Marcelo