On Mon, 2018-07-30 at 15:31 -0400, Jamal Hadi Salim wrote:

On 30/07/18 12:41 PM, Paolo Abeni wrote:

quoted

On Mon, 2018-07-30 at 10:03 -0400, Jamal Hadi Salim wrote:

quoted

On 30/07/18 08:30 AM, Paolo Abeni wrote:

quoted

   	}
   
+	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