From: Vlad Buslov <vladbu@mellanox•com>
To: Davide Caratti <dcaratti@redhat•com>
Cc: Cong Wang <xiyou.wangcong@gmail•com>,
"David S. Miller" <davem@davemloft•net>,
Jamal Hadi Salim <jhs@mojatatu•com>,
Jiri Pirko <jiri@resnulli•us>, Paolo Abeni <pabeni@redhat•com>,
Linux Kernel Network Developers <netdev@vger•kernel.org>
Subject: Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
Date: Thu, 7 Mar 2019 14:51:07 +0000 [thread overview]
Message-ID: <vbfmum6lovf.fsf@mellanox.com> (raw)
In-Reply-To: <c972a67115b09f7eb49758bde226342622fc0923.camel@redhat.com>
On Thu 07 Mar 2019 at 15:56, Davide Caratti <dcaratti@redhat•com> wrote:
> On Fri, 2019-03-01 at 16:04 -0800, Cong Wang wrote:
>> > On Fri, Mar 1, 2019 at 10:02 AM Davide Caratti <dcaratti@redhat•com> wrote:
>> >
>> > if I well understand the question, you are worried about
>> > tcf_action_goto_chain_exec(), that can dereference 'oldchain' while we are
>> > overwriting the action. A call to tcf_chain_put_by_act(oldchain) would
>> > decrease refcounts and eventually call kfree(oldchain).
>> >
>> > But this would result in a use-after-free only in case the chain has only
>> > refcount held by 1 action (the one we are overwriting), and 0 filters: is
>> > this a condition where packets can go through this action's data plane?
>>
>> Hmm? Isn't goto chain can be arbitrary? Packets can be routed
>> from this action to any filter chain, so even if the target chain has 0
>> filter this action still has traffic as long as itself is not on the same
>> chain?
>
> hi,
>
> sorry for the delay: it took some time to verify experimentally if we
> really need this or not. So, we want to ensure the control path doesn't do
>
> tcf_csum_init(..., ovr=1, ...)
> tcf_chain_put_by_act(oldchain)
> tcf_chain_destroy(oldchain, ...)
> kfree(oldchain);
>
> while the traffic path of the action is doing
>
> res->goto_tp = rcu_dereference_bh(oldchain->filter_chain);
>
> I iterated this script many times on a KASAN kernel,
>
> # tc chain add dev crash0 chain 42 ingress protocol ip flower ip_proto udp action pass
> # tc filter add dev crash0 ingress protocol ip flower ip_proto udp action csum udp goto chain 42 index 66
> # tc chain del dev crash0 chain 42 ingress
> (start netperf traffic)
> # tc action replace action csum udp pass index 66
>
> and reproduced twice the following splat:
>
> ==================================================================
> BUG: KASAN: null-ptr-deref in tcf_action_exec+0x181/0x200
> Read of size 8 at addr 0000000000000000 by task netperf/5777
>
> CPU: 0 PID: 5777 Comm: netperf Not tainted 5.0.0-rc7.goto_chain_fix+ #551
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> Call Trace:
> <IRQ>
> dump_stack+0xc7/0x15b
> ? show_regs_print_info+0x5/0x5
> ? _raw_read_lock_irq+0x40/0x40
> ? tcf_action_exec+0x181/0x200
> ? tcf_action_exec+0x181/0x200
> kasan_report+0x176/0x192
> ? tcf_action_exec+0x181/0x200
> tcf_action_exec+0x181/0x200
> ? find_dump_kind+0x170/0x170
> ? rcu_irq_exit+0x153/0x210
> fl_classify+0x81a/0x830
>
> 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. 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.)
Am I missing anything?
Regards,
Vlad
next prev parent reply other threads:[~2019-03-07 14:51 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
2019-02-27 17:40 ` [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action Davide Caratti
2019-02-28 1:28 ` Cong Wang
2019-03-01 17:59 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 02/16] net/sched: act_bpf: validate the control action inside init() Davide Caratti
2019-02-27 17:40 ` [PATCH net 03/16] net/sched: act_csum: " Davide Caratti
2019-02-28 1:50 ` Cong Wang
2019-03-01 18:02 ` Davide Caratti
2019-03-02 0:04 ` Cong Wang
2019-03-07 13:56 ` Davide Caratti
2019-03-07 14:51 ` Vlad Buslov [this message]
2019-03-07 16:56 ` Davide Caratti
2019-03-08 17:47 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 04/16] net/sched: act_gact: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 05/16] net/sched: act_ife: " Davide Caratti
2019-03-01 16:33 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 06/16] net/sched: act_mirred: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 07/16] net/sched: act_connmark: " Davide Caratti
2019-03-01 16:35 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 08/16] net/sched: act_nat: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 09/16] net/sched: act_pedit: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 10/16] net/sched: act_police: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 11/16] net/sched: act_sample: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 12/16] net/sched: act_simple: " Davide Caratti
2019-03-01 16:39 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 13/16] net/sched: act_skbedit: " Davide Caratti
2019-03-01 16:43 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 14/16] net/sched: act_skbmod: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 15/16] net/sched: act_tunnel_key: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 16/16] net/sched: act_vlan: " Davide Caratti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vbfmum6lovf.fsf@mellanox.com \
--to=vladbu@mellanox$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dcaratti@redhat$(echo .)com \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox