Re: [PATCH RFC 0/5] net/sched: validate the control action with all the other parameters

[Davide Caratti <dcaratti@redhat.com>]

On Mon, 2019-02-18 at 22:42 -0800, Cong Wang wrote:
> On Fri, Feb 15, 2019 at 3:06 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > currently, the kernel checks for bad values of the control action in
> > tcf_action_init_1(), after a successful call to the action's init()
> > function. This causes three bad behaviors:
> 
> Yeah, I have been complaining about this for a long time,
> although slightly differently. The problem here is the lack of
> "copy" in RCU mechanism, which makes it nearly impossible
> to rollback to the previous state of an action on failure path
> of an update, which also makes RCU readers reading a partially
> updated action too.

thanks for looking at this code.

by the way, I see that act_mirred has an error path after the
assignment of tcfm_eaction and tcfa_action, and this is again causing
a fail in the 'replace with bad action' tests ('half write', issue #1).
Since it's the same problem, I will fix this in the same patch (moving the
assignment after the 'if' test on the value of parm->ifindex.

> Before I fix the "copy" part, your fixes make sense to me. There
> might be some other way to expose the action-specific tcfa_action
> opcode, but it might not be better than yours.
> 
> BTW, please fold these bad behaviors into each appropriate
> patch, and keep the cover letter as an overview of the whole
> patchset rather than showing any details.
> 
> [...]

Ok, and I plan to add a selftest for each action - so that it's possible
to verify functionality (at least problem #1) before and after each
ommit.

> > all these three problems can be fixed if we validate the control action
> > in the init() function, in the same way as we are already doing for all
> > the other parameters.
> > 
> > - patch 1 is a temporary fix for problem 2), but it's reverted at the
> >   end of the series
> 
> Please drop patch 1, it is very unlikely only patch 1 will be backported,
> I think the whole patchset should be, therefore we have no reason
> to carry a temporary fix here.

sure, I will do that.
thanks!

-- 
davide