Commit ee3bbfe8 authored by Davide Caratti's avatar Davide Caratti Committed by David S. Miller

net/sched: let actions use RCU to access 'goto_chain'

use RCU when accessing the action chain, to avoid use after free in the
traffic path when 'goto chain' is replaced on existing TC actions (see
script below). Since the control action is read in the traffic path
without holding the action spinlock, we need to explicitly ensure that
a->goto_chain is not NULL before dereferencing (i.e it's not sufficient
to rely on the value of TC_ACT_GOTO_CHAIN bits). Not doing so caused NULL
dereferences in tcf_action_goto_chain_exec() when the following script:

 # tc chain add dev dd0 chain 42 ingress protocol ip flower \
 > ip_proto udp action pass index 4
 # tc filter add dev dd0 ingress protocol ip flower \
 > ip_proto udp action csum udp goto chain 42 index 66
 # tc chain del dev dd0 chain 42 ingress
 (start UDP traffic towards dd0)
 # tc action replace action csum udp pass index 66

was run repeatedly for several hours.
Suggested-by: default avatarCong Wang <xiyou.wangcong@gmail.com>
Suggested-by: default avatarVlad Buslov <vladbu@mellanox.com>
Signed-off-by: default avatarDavide Caratti <dcaratti@redhat.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent fe384e2f
...@@ -39,7 +39,7 @@ struct tc_action { ...@@ -39,7 +39,7 @@ struct tc_action {
struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw; struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw;
struct gnet_stats_queue __percpu *cpu_qstats; struct gnet_stats_queue __percpu *cpu_qstats;
struct tc_cookie __rcu *act_cookie; struct tc_cookie __rcu *act_cookie;
struct tcf_chain *goto_chain; struct tcf_chain __rcu *goto_chain;
}; };
#define tcf_index common.tcfa_index #define tcf_index common.tcfa_index
#define tcf_refcnt common.tcfa_refcnt #define tcf_refcnt common.tcfa_refcnt
......
...@@ -378,6 +378,7 @@ struct tcf_chain { ...@@ -378,6 +378,7 @@ struct tcf_chain {
bool flushing; bool flushing;
const struct tcf_proto_ops *tmplt_ops; const struct tcf_proto_ops *tmplt_ops;
void *tmplt_priv; void *tmplt_priv;
struct rcu_head rcu;
}; };
struct tcf_block { struct tcf_block {
......
...@@ -31,7 +31,7 @@ ...@@ -31,7 +31,7 @@
static void tcf_action_goto_chain_exec(const struct tc_action *a, static void tcf_action_goto_chain_exec(const struct tc_action *a,
struct tcf_result *res) struct tcf_result *res)
{ {
const struct tcf_chain *chain = a->goto_chain; const struct tcf_chain *chain = rcu_dereference_bh(a->goto_chain);
res->goto_tp = rcu_dereference_bh(chain->filter_chain); res->goto_tp = rcu_dereference_bh(chain->filter_chain);
} }
...@@ -91,13 +91,11 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp, ...@@ -91,13 +91,11 @@ int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
EXPORT_SYMBOL(tcf_action_check_ctrlact); EXPORT_SYMBOL(tcf_action_check_ctrlact);
struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action, struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,
struct tcf_chain *newchain) struct tcf_chain *goto_chain)
{ {
struct tcf_chain *oldchain = a->goto_chain;
a->tcfa_action = action; a->tcfa_action = action;
a->goto_chain = newchain; rcu_swap_protected(a->goto_chain, goto_chain, 1);
return oldchain; return goto_chain;
} }
EXPORT_SYMBOL(tcf_action_set_ctrlact); EXPORT_SYMBOL(tcf_action_set_ctrlact);
...@@ -108,7 +106,7 @@ EXPORT_SYMBOL(tcf_action_set_ctrlact); ...@@ -108,7 +106,7 @@ EXPORT_SYMBOL(tcf_action_set_ctrlact);
*/ */
static void free_tcf(struct tc_action *p) static void free_tcf(struct tc_action *p)
{ {
struct tcf_chain *chain = p->goto_chain; struct tcf_chain *chain = rcu_dereference_protected(p->goto_chain, 1);
free_percpu(p->cpu_bstats); free_percpu(p->cpu_bstats);
free_percpu(p->cpu_bstats_hw); free_percpu(p->cpu_bstats_hw);
...@@ -686,6 +684,10 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions, ...@@ -686,6 +684,10 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
return TC_ACT_OK; return TC_ACT_OK;
} }
} else if (TC_ACT_EXT_CMP(ret, TC_ACT_GOTO_CHAIN)) { } else if (TC_ACT_EXT_CMP(ret, TC_ACT_GOTO_CHAIN)) {
if (unlikely(!rcu_access_pointer(a->goto_chain))) {
net_warn_ratelimited("can't go to NULL chain!\n");
return TC_ACT_SHOT;
}
tcf_action_goto_chain_exec(a, res); tcf_action_goto_chain_exec(a, res);
} }
...@@ -931,7 +933,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, ...@@ -931,7 +933,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
module_put(a_o->owner); module_put(a_o->owner);
if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) && if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
!a->goto_chain) { !rcu_access_pointer(a->goto_chain)) {
tcf_action_destroy_1(a, bind); tcf_action_destroy_1(a, bind);
NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain"); NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
return ERR_PTR(-EINVAL); return ERR_PTR(-EINVAL);
......
...@@ -367,7 +367,7 @@ static void tcf_chain_destroy(struct tcf_chain *chain, bool free_block) ...@@ -367,7 +367,7 @@ static void tcf_chain_destroy(struct tcf_chain *chain, bool free_block)
struct tcf_block *block = chain->block; struct tcf_block *block = chain->block;
mutex_destroy(&chain->filter_chain_lock); mutex_destroy(&chain->filter_chain_lock);
kfree(chain); kfree_rcu(chain, rcu);
if (free_block) if (free_block)
tcf_block_destroy(block); tcf_block_destroy(block);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment