netdev - Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7494c1b7c4412e8c56f33d216f79e2a84ada34f9.camel@redhat.com>
Date:   Fri, 08 Mar 2019 18:47:38 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>, Paolo Abeni <pabeni@...hat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net 03/16] net/sched: act_csum: validate the control
 action inside init()

On Thu, 2019-03-07 at 17:56 +0100, Davide Caratti wrote:
> On Thu, 2019-03-07 at 14:51 +0000, Vlad Buslov wrote:
> 
> [...]

hi Vlad,

> > On Thu 07 Mar 2019 at 15:56, Davide Caratti <dcaratti@...hat.com> wrote:
> > > so, I think that the answer to your question:
> > > 
> > > On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
> > > > > > > +       if (oldchain)
> > > > > > > +               tcf_chain_put_by_act(oldchain);
> > > > > > 
> > > > > > Do we need to respect RCU grace period here?
> > > 
> > > is a "yes, we do".
> > > Now I'm trying something similar to what's done in tcf_bpf_init(), to
> > > release the bpf program on 'replace' operations:
> > > 
> > > 365         if (res == ACT_P_CREATED) {
> > > 366                 tcf_idr_insert(tn, *act);
> > > 367         } else {
> > > 368                 /* make sure the program being replaced is no longer executing */
> > > 369                 synchronize_rcu();
> > > 370                 tcf_bpf_cfg_cleanup(&old);
> > > 371         }
> > > 
> > > do you think it's worth going in this direction?
> > > thank you in advance!
> > 
> > Hi Davide,
> > 
> > Using synchronize_rcu() will impact rule update rate performance and I
> > don't think we really need it. 

... and moreover it doesn't seem to fix anything with my KASAN kernel when
the action is replaced with netperf running. I still see the same splat in
tcf_action_goto_chain_exec().

Probably the reason is that the "bad pointer" is chain, not chain->filter.
I will experiment rcu-ifiying 'goto_chain', like you proposed below.

> > I don't see any reason why we can't just
> > update chain to be rcu-friendly. Data path is already rcu_read
> > protected, in fact it only needs chain to read rcu-pointer to tp list
> > when jumping to chain. So it should be enough to do the following:
> > 
> > 1) Update tcf_chain_destroy() to free chain after rcu grace period.
> > 
> > 2) Convert tc_action->goto_chain to be a proper rcu pointer. (mark it
> > with "__rcu", assign with rcu_assign_pointer(), read it with
> > rcu_dereference{_bh}(), etc.)
> 
> it seems feasible, with some attention points:
> 
> 1) replacing the 'goto chain' in the init() function will then become 
> 
>     rcu_swap_protected(p->tcf_goto_chain, newchain,
>                        lockdep_is_held(&p->tcf_lock));
> 
> with p->tcf_lock held, and we will have to do this unconditionally also on
> non-update paths (it should have the same cost in CPU cycles as the rcu
> init / assign code).
> Unlike the synchronize_rcu(), that would only happen only in the update
> path of goto_chain actions, this is a fee that we pay in every path
> 
> 2) in tcf_action_goto_chain_exec(), we would have two "cascaded"
> rcu_dereference(), action->chain and chain->filter. Is this design
> acceptable?

thanks!
-- 
davide