Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa_action values

[Paolo Abeni <pabeni@redhat.com>]

On Mon, 2018-07-30 at 15:31 -0400, Jamal Hadi Salim wrote:
> On 30/07/18 12:41 PM, Paolo Abeni wrote:
> > On Mon, 2018-07-30 at 10:03 -0400, Jamal Hadi Salim wrote:
> > > On 30/07/18 08:30 AM, Paolo Abeni wrote:
> > > >    	}
> > > >    
> > > > +	if (!tcf_action_valid(a->tcfa_action)) {
> > > > +		NL_SET_ERR_MSG(extack, "invalid action value, using TC_ACT_UNSPEC instead");
> > > > +		a->tcfa_action = TC_ACT_UNSPEC;
> > > > +	}
> > > > +
> > > >    	return a;
> > > >    
> > > 
> > > 
> > > I think it would make a lot more sense to just reject the entry than
> > > changing it underneath the user to a default value. Least element of
> > > suprise.
> > 
> > I fear that would break existing (bad) users ?!? This way, such users
> > are notified they are doing something uncorrect, but still continue to
> > work.
> 
> 
> By "bad users" I think you mean someone setting a policy expecting
> one behavior but getting a different one? 

Generally speaking I thought about user-space pushing to the kernel
some uninitialized/random value for 'tcfa_action'.

> If yes, that policy was
> already wrong/buggy. As an example, if i configured:
> 
> match xxx action foo action goo action bar action gah
> 
> where action goo has a bad opcode
> If  you "fix it"  with TC_ACT_UNSPEC then basically the above
> policy is now equivalent to:
> 
> match xxx action foo action goo
> 
> Infact if there was a lower prio rule in the chain
> then lookup will continue there and produce even stranger
> results.

I see. 

Before this patch, the kernel exposed the same behaviour for negative
value of 'bar', while, for positive 'bar' values, the overall behaviour
was more complex (some classifier always stops with unknown positive
action value, others go to lower prio).

Overall, the kernel behavior should be more well-defined now, but yes,
there is a change of behavior under some circumstances.

What about instead mapping undefined/unknown actions value to TC_ACT_OK
(still at initialization time)? this is what is already done by
tcf_action_exec() for faulty opcodes/graphs and by tcf_ipt() and would
handle the above example more conistently.

Cheers,

Paolo