* [PATCH net-next 01/14] net: sched: act_bpf: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 02/14] net: sched: act_csum: " Vlad Buslov
` (12 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock to protect bpf action private data from concurrent
modification during dump and init. Remove rtnl lock assertion that is no
longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_bpf.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 6203eb075c9a..9e8a33f9fee3 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -143,11 +143,12 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act,
.index = prog->tcf_index,
.refcnt = refcount_read(&prog->tcf_refcnt) - ref,
.bindcnt = atomic_read(&prog->tcf_bindcnt) - bind,
- .action = prog->tcf_action,
};
struct tcf_t tm;
int ret;
+ spin_lock(&prog->tcf_lock);
+ opt.action = prog->tcf_action;
if (nla_put(skb, TCA_ACT_BPF_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -163,9 +164,11 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act,
TCA_ACT_BPF_PAD))
goto nla_put_failure;
+ spin_unlock(&prog->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock(&prog->tcf_lock);
nlmsg_trim(skb, tp);
return -1;
}
@@ -264,7 +267,7 @@ static void tcf_bpf_prog_fill_cfg(const struct tcf_bpf *prog,
{
cfg->is_ebpf = tcf_bpf_is_ebpf(prog);
/* updates to prog->filter are prevented, since it's called either
- * with rtnl lock or during final cleanup in rcu callback
+ * with tcf lock or during final cleanup in rcu callback
*/
cfg->filter = rcu_dereference_protected(prog->filter, 1);
@@ -336,8 +339,8 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
goto out;
prog = to_bpf(*act);
- ASSERT_RTNL();
+ spin_lock(&prog->tcf_lock);
if (res != ACT_P_CREATED)
tcf_bpf_prog_fill_cfg(prog, &old);
@@ -349,6 +352,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
prog->tcf_action = parm->action;
rcu_assign_pointer(prog->filter, cfg.filter);
+ spin_unlock(&prog->tcf_lock);
if (res == ACT_P_CREATED) {
tcf_idr_insert(tn, *act);
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 02/14] net: sched: act_csum: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 01/14] net: sched: act_bpf: remove dependency on rtnl lock Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 03/14] net: sched: act_gact: " Vlad Buslov
` (11 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf lock to protect csum action struct private data from concurrent
modification in init and dump. Use rcu swap operation to reassign params
pointer under protection of tcf lock. (old params value is not used by
init, so there is no need of standalone rcu dereference step)
Remove rtnl assertion that is no longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_csum.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 648a3a35b720..f01c59ba6d12 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -50,7 +50,7 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, csum_net_id);
- struct tcf_csum_params *params_old, *params_new;
+ struct tcf_csum_params *params_new;
struct nlattr *tb[TCA_CSUM_MAX + 1];
struct tc_csum *parm;
struct tcf_csum *p;
@@ -88,20 +88,22 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
}
p = to_tcf_csum(*a);
- ASSERT_RTNL();
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
tcf_idr_release(*a, bind);
return -ENOMEM;
}
- params_old = rtnl_dereference(p->params);
+ params_new->update_flags = parm->update_flags;
+ spin_lock(&p->tcf_lock);
p->tcf_action = parm->action;
- params_new->update_flags = parm->update_flags;
- rcu_assign_pointer(p->params, params_new);
- if (params_old)
- kfree_rcu(params_old, rcu);
+ rcu_swap_protected(p->params, params_new,
+ lockdep_is_held(&p->tcf_lock));
+ spin_unlock(&p->tcf_lock);
+
+ if (params_new)
+ kfree_rcu(params_new, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -599,11 +601,13 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
.index = p->tcf_index,
.refcnt = refcount_read(&p->tcf_refcnt) - ref,
.bindcnt = atomic_read(&p->tcf_bindcnt) - bind,
- .action = p->tcf_action,
};
struct tcf_t t;
- params = rtnl_dereference(p->params);
+ spin_lock(&p->tcf_lock);
+ params = rcu_dereference_protected(p->params,
+ lockdep_is_held(&p->tcf_lock));
+ opt.action = p->tcf_action;
opt.update_flags = params->update_flags;
if (nla_put(skb, TCA_CSUM_PARMS, sizeof(opt), &opt))
@@ -612,10 +616,12 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
tcf_tm_dump(&t, &p->tcf_tm);
if (nla_put_64bit(skb, TCA_CSUM_TM, sizeof(t), &t, TCA_CSUM_PAD))
goto nla_put_failure;
+ spin_unlock(&p->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock(&p->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 03/14] net: sched: act_gact: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 01/14] net: sched: act_bpf: remove dependency on rtnl lock Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 02/14] net: sched: act_csum: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 04/14] net: sched: act_ife: " Vlad Buslov
` (10 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock to protect gact action private state from concurrent
modification during dump and init. Remove rtnl assertion that is no longer
necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_gact.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 661b72b9147d..bfccd34a3968 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -113,7 +113,7 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
gact = to_gact(*a);
- ASSERT_RTNL();
+ spin_lock(&gact->tcf_lock);
gact->tcf_action = parm->action;
#ifdef CONFIG_GACT_PROB
if (p_parm) {
@@ -126,6 +126,8 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
gact->tcfg_ptype = p_parm->ptype;
}
#endif
+ spin_unlock(&gact->tcf_lock);
+
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
@@ -178,10 +180,11 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
.index = gact->tcf_index,
.refcnt = refcount_read(&gact->tcf_refcnt) - ref,
.bindcnt = atomic_read(&gact->tcf_bindcnt) - bind,
- .action = gact->tcf_action,
};
struct tcf_t t;
+ spin_lock(&gact->tcf_lock);
+ opt.action = gact->tcf_action;
if (nla_put(skb, TCA_GACT_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
#ifdef CONFIG_GACT_PROB
@@ -199,9 +202,12 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &gact->tcf_tm);
if (nla_put_64bit(skb, TCA_GACT_TM, sizeof(t), &t, TCA_GACT_PAD))
goto nla_put_failure;
+ spin_unlock(&gact->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&gact->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 04/14] net: sched: act_ife: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (2 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 03/14] net: sched: act_gact: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 05/14] net: sched: act_ipt: " Vlad Buslov
` (9 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock and rcu to protect params pointer from concurrent
modification during dump and init. Use rcu swap operation to reassign
params pointer under protection of tcf lock. (old params value is not used
by init, so there is no need of standalone rcu dereference step)
Ife action has meta-actions that are compiled as standalone modules. Rtnl
mutex must be released while loading a kernel module. In order to support
execution without rtnl mutex, propagate 'rtnl_held' argument to meta action
loading functions. When requesting meta action module, conditionally
release rtnl lock depending on 'rtnl_held' argument.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_ife.c | 40 +++++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index df4060e32d43..5d200495e467 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -268,7 +268,8 @@ static const char *ife_meta_id2name(u32 metaid)
* under ife->tcf_lock for existing action
*/
static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
- void *val, int len, bool exists)
+ void *val, int len, bool exists,
+ bool rtnl_held)
{
struct tcf_meta_ops *ops = find_ife_oplist(metaid);
int ret = 0;
@@ -278,9 +279,11 @@ static int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
#ifdef CONFIG_MODULES
if (exists)
spin_unlock_bh(&ife->tcf_lock);
- rtnl_unlock();
+ if (rtnl_held)
+ rtnl_unlock();
request_module("ife-meta-%s", ife_meta_id2name(metaid));
- rtnl_lock();
+ if (rtnl_held)
+ rtnl_lock();
if (exists)
spin_lock_bh(&ife->tcf_lock);
ops = find_ife_oplist(metaid);
@@ -421,7 +424,7 @@ static void tcf_ife_cleanup(struct tc_action *a)
/* under ife->tcf_lock for existing action */
static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
- bool exists)
+ bool exists, bool rtnl_held)
{
int len = 0;
int rc = 0;
@@ -433,7 +436,8 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
val = nla_data(tb[i]);
len = nla_len(tb[i]);
- rc = load_metaops_and_vet(ife, i, val, len, exists);
+ rc = load_metaops_and_vet(ife, i, val, len, exists,
+ rtnl_held);
if (rc != 0)
return rc;
@@ -454,7 +458,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
struct tc_action_net *tn = net_generic(net, ife_net_id);
struct nlattr *tb[TCA_IFE_MAX + 1];
struct nlattr *tb2[IFE_META_MAX + 1];
- struct tcf_ife_params *p, *p_old;
+ struct tcf_ife_params *p;
struct tcf_ife_info *ife;
u16 ife_type = ETH_P_IFE;
struct tc_ife *parm;
@@ -558,7 +562,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
return err;
}
- err = populate_metalist(ife, tb2, exists);
+ err = populate_metalist(ife, tb2, exists, rtnl_held);
if (err)
goto metadata_parse_err;
@@ -581,13 +585,13 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
}
ife->tcf_action = parm->action;
+ /* protected by tcf_lock when modifying existing action */
+ rcu_swap_protected(ife->params, p, 1);
+
if (exists)
spin_unlock_bh(&ife->tcf_lock);
-
- p_old = rtnl_dereference(ife->params);
- rcu_assign_pointer(ife->params, p);
- if (p_old)
- kfree_rcu(p_old, rcu);
+ if (p)
+ kfree_rcu(p, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -600,16 +604,20 @@ static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
{
unsigned char *b = skb_tail_pointer(skb);
struct tcf_ife_info *ife = to_ife(a);
- struct tcf_ife_params *p = rtnl_dereference(ife->params);
+ struct tcf_ife_params *p;
struct tc_ife opt = {
.index = ife->tcf_index,
.refcnt = refcount_read(&ife->tcf_refcnt) - ref,
.bindcnt = atomic_read(&ife->tcf_bindcnt) - bind,
- .action = ife->tcf_action,
- .flags = p->flags,
};
struct tcf_t t;
+ spin_lock_bh(&ife->tcf_lock);
+ opt.action = ife->tcf_action;
+ p = rcu_dereference_protected(ife->params,
+ lockdep_is_held(&ife->tcf_lock));
+ opt.flags = p->flags;
+
if (nla_put(skb, TCA_IFE_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -635,9 +643,11 @@ static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
pr_info("Failed to dump metalist\n");
}
+ spin_unlock_bh(&ife->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&ife->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 05/14] net: sched: act_ipt: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (3 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 04/14] net: sched: act_ife: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 06/14] net: sched: act_pedit: " Vlad Buslov
` (8 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock to protect ipt action private data from concurrent
modification during dump. Ipt init already takes tcf spinlock when
modifying ipt state.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_ipt.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 0dc787a57798..e149f0e66cb6 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -288,6 +288,7 @@ static int tcf_ipt_dump(struct sk_buff *skb, struct tc_action *a, int bind,
* for foolproof you need to not assume this
*/
+ spin_lock_bh(&ipt->tcf_lock);
t = kmemdup(ipt->tcfi_t, ipt->tcfi_t->u.user.target_size, GFP_ATOMIC);
if (unlikely(!t))
goto nla_put_failure;
@@ -307,10 +308,12 @@ static int tcf_ipt_dump(struct sk_buff *skb, struct tc_action *a, int bind,
if (nla_put_64bit(skb, TCA_IPT_TM, sizeof(tm), &tm, TCA_IPT_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&ipt->tcf_lock);
kfree(t);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&ipt->tcf_lock);
nlmsg_trim(skb, b);
kfree(t);
return -1;
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 06/14] net: sched: act_pedit: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (4 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 05/14] net: sched: act_ipt: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 07/14] net: sched: act_sample: " Vlad Buslov
` (7 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Rearrange pedit init code to only access pedit action data while holding
tcf spinlock. Change keys allocation type to atomic to allow it to execute
while holding tcf spinlock. Take tcf spinlock in dump function when
accessing pedit action data.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_pedit.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 43ba999b2d23..3f62da72ab6a 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -187,44 +187,38 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
tcf_idr_cleanup(tn, parm->index);
goto out_free;
}
- p = to_pedit(*a);
- keys = kmalloc(ksize, GFP_KERNEL);
- if (!keys) {
- tcf_idr_release(*a, bind);
- ret = -ENOMEM;
- goto out_free;
- }
ret = ACT_P_CREATED;
} else if (err > 0) {
if (bind)
goto out_free;
if (!ovr) {
- tcf_idr_release(*a, bind);
ret = -EEXIST;
- goto out_free;
- }
- p = to_pedit(*a);
- if (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys) {
- keys = kmalloc(ksize, GFP_KERNEL);
- if (!keys) {
- ret = -ENOMEM;
- goto out_free;
- }
+ goto out_release;
}
} else {
return err;
}
+ p = to_pedit(*a);
spin_lock_bh(&p->tcf_lock);
- p->tcfp_flags = parm->flags;
- p->tcf_action = parm->action;
- if (keys) {
+
+ if (ret == ACT_P_CREATED ||
+ (p->tcfp_nkeys && p->tcfp_nkeys != parm->nkeys)) {
+ keys = kmalloc(ksize, GFP_ATOMIC);
+ if (!keys) {
+ spin_unlock_bh(&p->tcf_lock);
+ ret = -ENOMEM;
+ goto out_release;
+ }
kfree(p->tcfp_keys);
p->tcfp_keys = keys;
p->tcfp_nkeys = parm->nkeys;
}
memcpy(p->tcfp_keys, parm->keys, ksize);
+ p->tcfp_flags = parm->flags;
+ p->tcf_action = parm->action;
+
kfree(p->tcfp_keys_ex);
p->tcfp_keys_ex = keys_ex;
@@ -232,6 +226,9 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
+
+out_release:
+ tcf_idr_release(*a, bind);
out_free:
kfree(keys_ex);
return ret;
@@ -410,6 +407,7 @@ static int tcf_pedit_dump(struct sk_buff *skb, struct tc_action *a,
if (unlikely(!opt))
return -ENOBUFS;
+ spin_lock_bh(&p->tcf_lock);
memcpy(opt->keys, p->tcfp_keys,
p->tcfp_nkeys * sizeof(struct tc_pedit_key));
opt->index = p->tcf_index;
@@ -432,11 +430,13 @@ static int tcf_pedit_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &p->tcf_tm);
if (nla_put_64bit(skb, TCA_PEDIT_TM, sizeof(t), &t, TCA_PEDIT_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&p->tcf_lock);
kfree(opt);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&p->tcf_lock);
nlmsg_trim(skb, b);
kfree(opt);
return -1;
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 07/14] net: sched: act_sample: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (5 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 06/14] net: sched: act_pedit: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 08/14] net: sched: act_simple: " Vlad Buslov
` (6 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock to protect private sample action data from concurrent
modification during dump and init.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_sample.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 2608ccc83e5e..81071afe1b43 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -80,11 +80,13 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
}
s = to_sample(*a);
+ spin_lock(&s->tcf_lock);
s->tcf_action = parm->action;
s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
s->psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
psample_group = psample_group_get(net, s->psample_group_num);
if (!psample_group) {
+ spin_unlock(&s->tcf_lock);
tcf_idr_release(*a, bind);
return -ENOMEM;
}
@@ -94,6 +96,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
s->truncate = true;
s->trunc_size = nla_get_u32(tb[TCA_SAMPLE_TRUNC_SIZE]);
}
+ spin_unlock(&s->tcf_lock);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -105,7 +108,8 @@ static void tcf_sample_cleanup(struct tc_action *a)
struct tcf_sample *s = to_sample(a);
struct psample_group *psample_group;
- psample_group = rtnl_dereference(s->psample_group);
+ /* last reference to action, no need to lock */
+ psample_group = rcu_dereference_protected(s->psample_group, 1);
RCU_INIT_POINTER(s->psample_group, NULL);
if (psample_group)
psample_group_put(psample_group);
@@ -174,12 +178,13 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_sample *s = to_sample(a);
struct tc_sample opt = {
.index = s->tcf_index,
- .action = s->tcf_action,
.refcnt = refcount_read(&s->tcf_refcnt) - ref,
.bindcnt = atomic_read(&s->tcf_bindcnt) - bind,
};
struct tcf_t t;
+ spin_lock(&s->tcf_lock);
+ opt.action = s->tcf_action;
if (nla_put(skb, TCA_SAMPLE_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -196,9 +201,12 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_u32(skb, TCA_SAMPLE_PSAMPLE_GROUP, s->psample_group_num))
goto nla_put_failure;
+ spin_unlock(&s->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&s->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 08/14] net: sched: act_simple: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (6 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 07/14] net: sched: act_sample: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 09/14] net: sched: act_skbmod: " Vlad Buslov
` (5 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock to protect private simple action data from concurrent
modification during dump. (simple init already uses tcf spinlock when
changing action state)
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_simple.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index aa51152e0066..18e4452574cd 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -156,10 +156,11 @@ static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
.index = d->tcf_index,
.refcnt = refcount_read(&d->tcf_refcnt) - ref,
.bindcnt = atomic_read(&d->tcf_bindcnt) - bind,
- .action = d->tcf_action,
};
struct tcf_t t;
+ spin_lock_bh(&d->tcf_lock);
+ opt.action = d->tcf_action;
if (nla_put(skb, TCA_DEF_PARMS, sizeof(opt), &opt) ||
nla_put_string(skb, TCA_DEF_DATA, d->tcfd_defdata))
goto nla_put_failure;
@@ -167,9 +168,12 @@ static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &d->tcf_tm);
if (nla_put_64bit(skb, TCA_DEF_TM, sizeof(t), &t, TCA_DEF_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&d->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&d->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 09/14] net: sched: act_skbmod: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (7 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 08/14] net: sched: act_simple: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 10/14] net: sched: act_tunnel_key: " Vlad Buslov
` (4 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Move read of skbmod_p rcu pointer to be protected by tcf spinlock. Use tcf
spinlock to protect private skbmod data from concurrent modification during
dump.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_skbmod.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index c437c6d51a71..e9c86ade3b40 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -156,7 +156,6 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
d = to_skbmod(*a);
- ASSERT_RTNL();
p = kzalloc(sizeof(struct tcf_skbmod_params), GFP_KERNEL);
if (unlikely(!p)) {
tcf_idr_release(*a, bind);
@@ -166,10 +165,10 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
p->flags = lflags;
d->tcf_action = parm->action;
- p_old = rtnl_dereference(d->skbmod_p);
-
if (ovr)
spin_lock_bh(&d->tcf_lock);
+ /* Protected by tcf_lock if overwriting existing action. */
+ p_old = rcu_dereference_protected(d->skbmod_p, 1);
if (lflags & SKBMOD_F_DMAC)
ether_addr_copy(p->eth_dst, daddr);
@@ -205,15 +204,18 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
{
struct tcf_skbmod *d = to_skbmod(a);
unsigned char *b = skb_tail_pointer(skb);
- struct tcf_skbmod_params *p = rtnl_dereference(d->skbmod_p);
+ struct tcf_skbmod_params *p;
struct tc_skbmod opt = {
.index = d->tcf_index,
.refcnt = refcount_read(&d->tcf_refcnt) - ref,
.bindcnt = atomic_read(&d->tcf_bindcnt) - bind,
- .action = d->tcf_action,
};
struct tcf_t t;
+ spin_lock_bh(&d->tcf_lock);
+ opt.action = d->tcf_action;
+ p = rcu_dereference_protected(d->skbmod_p,
+ lockdep_is_held(&d->tcf_lock));
opt.flags = p->flags;
if (nla_put(skb, TCA_SKBMOD_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -231,8 +233,10 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_64bit(skb, TCA_SKBMOD_TM, sizeof(t), &t, TCA_SKBMOD_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&d->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&d->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 10/14] net: sched: act_tunnel_key: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (8 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 09/14] net: sched: act_skbmod: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 11/14] net: sched: act_vlan: " Vlad Buslov
` (3 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf lock to protect tunnel key action struct private data from
concurrent modification in init and dump. Use rcu swap operation to
reassign params pointer under protection of tcf lock. (old params value is
not used by init, so there is no need of standalone rcu dereference step)
Remove rtnl lock assertion that is no longer required.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_tunnel_key.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index d42d9e112789..ba2ae9f75ef5 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -204,7 +204,6 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
{
struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
struct nlattr *tb[TCA_TUNNEL_KEY_MAX + 1];
- struct tcf_tunnel_key_params *params_old;
struct tcf_tunnel_key_params *params_new;
struct metadata_dst *metadata = NULL;
struct tc_tunnel_key *parm;
@@ -346,24 +345,22 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
t = to_tunnel_key(*a);
- ASSERT_RTNL();
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
tcf_idr_release(*a, bind);
NL_SET_ERR_MSG(extack, "Cannot allocate tunnel key parameters");
return -ENOMEM;
}
-
- params_old = rtnl_dereference(t->params);
-
- t->tcf_action = parm->action;
params_new->tcft_action = parm->t_action;
params_new->tcft_enc_metadata = metadata;
- rcu_assign_pointer(t->params, params_new);
-
- if (params_old)
- kfree_rcu(params_old, rcu);
+ spin_lock(&t->tcf_lock);
+ t->tcf_action = parm->action;
+ rcu_swap_protected(t->params, params_new,
+ lockdep_is_held(&t->tcf_lock));
+ spin_unlock(&t->tcf_lock);
+ if (params_new)
+ kfree_rcu(params_new, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -485,12 +482,13 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
.index = t->tcf_index,
.refcnt = refcount_read(&t->tcf_refcnt) - ref,
.bindcnt = atomic_read(&t->tcf_bindcnt) - bind,
- .action = t->tcf_action,
};
struct tcf_t tm;
- params = rtnl_dereference(t->params);
-
+ spin_lock(&t->tcf_lock);
+ params = rcu_dereference_protected(t->params,
+ lockdep_is_held(&t->tcf_lock));
+ opt.action = t->tcf_action;
opt.t_action = params->tcft_action;
if (nla_put(skb, TCA_TUNNEL_KEY_PARMS, sizeof(opt), &opt))
@@ -522,10 +520,12 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
if (nla_put_64bit(skb, TCA_TUNNEL_KEY_TM, sizeof(tm),
&tm, TCA_TUNNEL_KEY_PAD))
goto nla_put_failure;
+ spin_unlock(&t->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock(&t->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 11/14] net: sched: act_vlan: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (9 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 10/14] net: sched: act_tunnel_key: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 12/14] net: sched: act_mirred: " Vlad Buslov
` (2 subsequent siblings)
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock to protect vlan action private data from concurrent
modification during dump and init. Use rcu swap operation to reassign
params pointer under protection of tcf lock. (old params value is not used
by init, so there is no need of standalone rcu dereference step)
Remove rtnl assertion that is no longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_vlan.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 15a0ee214c9c..5bde17fe3608 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -109,7 +109,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
{
struct tc_action_net *tn = net_generic(net, vlan_net_id);
struct nlattr *tb[TCA_VLAN_MAX + 1];
- struct tcf_vlan_params *p, *p_old;
+ struct tcf_vlan_params *p;
struct tc_vlan *parm;
struct tcf_vlan *v;
int action;
@@ -202,26 +202,24 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
v = to_vlan(*a);
- ASSERT_RTNL();
p = kzalloc(sizeof(*p), GFP_KERNEL);
if (!p) {
tcf_idr_release(*a, bind);
return -ENOMEM;
}
- v->tcf_action = parm->action;
-
- p_old = rtnl_dereference(v->vlan_p);
-
p->tcfv_action = action;
p->tcfv_push_vid = push_vid;
p->tcfv_push_prio = push_prio;
p->tcfv_push_proto = push_proto;
- rcu_assign_pointer(v->vlan_p, p);
+ spin_lock(&v->tcf_lock);
+ v->tcf_action = parm->action;
+ rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
+ spin_unlock(&v->tcf_lock);
- if (p_old)
- kfree_rcu(p_old, rcu);
+ if (p)
+ kfree_rcu(p, rcu);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
@@ -243,16 +241,18 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
{
unsigned char *b = skb_tail_pointer(skb);
struct tcf_vlan *v = to_vlan(a);
- struct tcf_vlan_params *p = rtnl_dereference(v->vlan_p);
+ struct tcf_vlan_params *p;
struct tc_vlan opt = {
.index = v->tcf_index,
.refcnt = refcount_read(&v->tcf_refcnt) - ref,
.bindcnt = atomic_read(&v->tcf_bindcnt) - bind,
- .action = v->tcf_action,
- .v_action = p->tcfv_action,
};
struct tcf_t t;
+ spin_lock(&v->tcf_lock);
+ opt.action = v->tcf_action;
+ p = rcu_dereference_protected(v->vlan_p, lockdep_is_held(&v->tcf_lock));
+ opt.v_action = p->tcfv_action;
if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
@@ -268,9 +268,12 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
tcf_tm_dump(&t, &v->tcf_tm);
if (nla_put_64bit(skb, TCA_VLAN_TM, sizeof(t), &t, TCA_VLAN_PAD))
goto nla_put_failure;
+ spin_unlock(&v->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&v->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (10 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 11/14] net: sched: act_vlan: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-07 16:36 ` Jiri Pirko
2018-08-06 6:54 ` [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter Vlad Buslov
2018-08-06 6:54 ` [PATCH net-next 14/14] net: sched: act_police: remove dependency on rtnl lock Vlad Buslov
13 siblings, 1 reply; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Re-introduce mirred list spinlock, that was removed some time ago, in order
to protect it from concurrent modifications, instead of relying on rtnl
lock.
Use tcf spinlock to protect mirred action private data from concurrent
modification in init and dump. Rearrange access to mirred data in order to
be performed only while holding the lock.
Rearrange net dev access to always hold reference while working with it,
instead of relying on rntl lock. Change get dev function to increment net
device reference before returning it to caller, instead of assuming that
caller is protected with rtnl lock.
Provide rcu version of mirred dev and tunnel info access functions. (to be
used by unlocked drivers)
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
include/net/tc_act/tc_mirred.h | 5 +++
include/net/tc_act/tc_tunnel_key.h | 33 ++++++++++++---
net/sched/act_mirred.c | 82 +++++++++++++++++++++++++-------------
net/sched/cls_api.c | 1 +
4 files changed, 88 insertions(+), 33 deletions(-)
diff --git a/include/net/tc_act/tc_mirred.h b/include/net/tc_act/tc_mirred.h
index a2e9cbca5c9e..cb30be55e444 100644
--- a/include/net/tc_act/tc_mirred.h
+++ b/include/net/tc_act/tc_mirred.h
@@ -37,4 +37,9 @@ static inline struct net_device *tcf_mirred_dev(const struct tc_action *a)
return rtnl_dereference(to_mirred(a)->tcfm_dev);
}
+static inline struct net_device *tcf_mirred_dev_rcu(const struct tc_action *a)
+{
+ return rcu_dereference(to_mirred(a)->tcfm_dev);
+}
+
#endif /* __NET_TC_MIR_H */
diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
index 46b8c7f1c8d5..e6e475d788c6 100644
--- a/include/net/tc_act/tc_tunnel_key.h
+++ b/include/net/tc_act/tc_tunnel_key.h
@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
static inline bool is_tcf_tunnel_set(const struct tc_action *a)
{
+ bool ret = false;
#ifdef CONFIG_NET_CLS_ACT
struct tcf_tunnel_key *t = to_tunnel_key(a);
- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
+ struct tcf_tunnel_key_params *params;
+ rcu_read_lock();
+ params = rcu_dereference(t->params);
if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
+ rcu_read_unlock();
#endif
- return false;
+ return ret;
}
static inline bool is_tcf_tunnel_release(const struct tc_action *a)
{
+ bool ret = false;
#ifdef CONFIG_NET_CLS_ACT
struct tcf_tunnel_key *t = to_tunnel_key(a);
- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
+ struct tcf_tunnel_key_params *params;
+ rcu_read_lock();
+ params = rcu_dereference(t->params);
if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
- return params->tcft_action == TCA_TUNNEL_KEY_ACT_RELEASE;
+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_RELEASE;
+ rcu_read_unlock();
+#endif
+ return ret;
+}
+
+static inline
+struct ip_tunnel_info *tcf_tunnel_info_rcu(const struct tc_action *a)
+{
+#ifdef CONFIG_NET_CLS_ACT
+ struct tcf_tunnel_key *t = to_tunnel_key(a);
+ struct tcf_tunnel_key_params *params = rcu_dereference(t->params);
+
+ return ¶ms->tcft_enc_metadata->u.tun_info;
+#else
+ return NULL;
#endif
- return false;
}
static inline struct ip_tunnel_info *tcf_tunnel_info(const struct tc_action *a)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index b26d060da08e..9f622114f5a5 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -30,6 +30,7 @@
#include <net/tc_act/tc_mirred.h>
static LIST_HEAD(mirred_list);
+static DEFINE_SPINLOCK(mirred_list_lock);
static bool tcf_mirred_is_act_redirect(int action)
{
@@ -62,13 +63,23 @@ static bool tcf_mirred_can_reinsert(int action)
return false;
}
+static struct net_device *tcf_mirred_dev_dereference(struct tcf_mirred *m)
+{
+ return rcu_dereference_protected(m->tcfm_dev,
+ lockdep_is_held(&m->tcf_lock));
+}
+
static void tcf_mirred_release(struct tc_action *a)
{
struct tcf_mirred *m = to_mirred(a);
struct net_device *dev;
+ spin_lock(&mirred_list_lock);
list_del(&m->tcfm_list);
- dev = rtnl_dereference(m->tcfm_dev);
+ spin_unlock(&mirred_list_lock);
+
+ /* last reference to action, no need to lock */
+ dev = rcu_dereference_protected(m->tcfm_dev, 1);
if (dev)
dev_put(dev);
}
@@ -128,22 +139,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
NL_SET_ERR_MSG_MOD(extack, "Unknown mirred option");
return -EINVAL;
}
- if (parm->ifindex) {
- dev = __dev_get_by_index(net, parm->ifindex);
- if (dev == NULL) {
- if (exists)
- tcf_idr_release(*a, bind);
- else
- tcf_idr_cleanup(tn, parm->index);
- return -ENODEV;
- }
- mac_header_xmit = dev_is_mac_header_xmit(dev);
- } else {
- dev = NULL;
- }
if (!exists) {
- if (!dev) {
+ if (!parm->ifindex) {
tcf_idr_cleanup(tn, parm->index);
NL_SET_ERR_MSG_MOD(extack, "Specified device does not exist");
return -EINVAL;
@@ -161,19 +159,31 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
}
m = to_mirred(*a);
- ASSERT_RTNL();
+ spin_lock(&m->tcf_lock);
m->tcf_action = parm->action;
m->tcfm_eaction = parm->eaction;
- if (dev != NULL) {
- if (ret != ACT_P_CREATED)
- dev_put(rcu_dereference_protected(m->tcfm_dev, 1));
- dev_hold(dev);
- rcu_assign_pointer(m->tcfm_dev, dev);
+
+ if (parm->ifindex) {
+ dev = dev_get_by_index(net, parm->ifindex);
+ if (!dev) {
+ spin_unlock(&m->tcf_lock);
+ tcf_idr_release(*a, bind);
+ return -ENODEV;
+ }
+ mac_header_xmit = dev_is_mac_header_xmit(dev);
+ rcu_swap_protected(m->tcfm_dev, dev,
+ lockdep_is_held(&m->tcf_lock));
+ if (dev)
+ dev_put(dev);
m->tcfm_mac_header_xmit = mac_header_xmit;
}
+ spin_unlock(&m->tcf_lock);
if (ret == ACT_P_CREATED) {
+ spin_lock(&mirred_list_lock);
list_add(&m->tcfm_list, &mirred_list);
+ spin_unlock(&mirred_list_lock);
+
tcf_idr_insert(tn, *a);
}
@@ -287,26 +297,33 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
{
unsigned char *b = skb_tail_pointer(skb);
struct tcf_mirred *m = to_mirred(a);
- struct net_device *dev = rtnl_dereference(m->tcfm_dev);
struct tc_mirred opt = {
.index = m->tcf_index,
- .action = m->tcf_action,
.refcnt = refcount_read(&m->tcf_refcnt) - ref,
.bindcnt = atomic_read(&m->tcf_bindcnt) - bind,
- .eaction = m->tcfm_eaction,
- .ifindex = dev ? dev->ifindex : 0,
};
+ struct net_device *dev;
struct tcf_t t;
+ spin_lock(&m->tcf_lock);
+ opt.action = m->tcf_action;
+ opt.eaction = m->tcfm_eaction;
+ dev = tcf_mirred_dev_dereference(m);
+ if (dev)
+ opt.ifindex = dev->ifindex;
+
if (nla_put(skb, TCA_MIRRED_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
tcf_tm_dump(&t, &m->tcf_tm);
if (nla_put_64bit(skb, TCA_MIRRED_TM, sizeof(t), &t, TCA_MIRRED_PAD))
goto nla_put_failure;
+ spin_unlock(&m->tcf_lock);
+
return skb->len;
nla_put_failure:
+ spin_unlock(&m->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
@@ -337,15 +354,19 @@ static int mirred_device_event(struct notifier_block *unused,
ASSERT_RTNL();
if (event == NETDEV_UNREGISTER) {
+ spin_lock(&mirred_list_lock);
list_for_each_entry(m, &mirred_list, tcfm_list) {
- if (rcu_access_pointer(m->tcfm_dev) == dev) {
+ spin_lock(&m->tcf_lock);
+ if (tcf_mirred_dev_dereference(m) == dev) {
dev_put(dev);
/* Note : no rcu grace period necessary, as
* net_device are already rcu protected.
*/
RCU_INIT_POINTER(m->tcfm_dev, NULL);
}
+ spin_unlock(&m->tcf_lock);
}
+ spin_unlock(&mirred_list_lock);
}
return NOTIFY_DONE;
@@ -358,8 +379,15 @@ static struct notifier_block mirred_device_notifier = {
static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
{
struct tcf_mirred *m = to_mirred(a);
+ struct net_device *dev;
+
+ rcu_read_lock();
+ dev = rcu_dereference(m->tcfm_dev);
+ if (dev)
+ dev_hold(dev);
+ rcu_read_unlock();
- return rtnl_dereference(m->tcfm_dev);
+ return dev;
}
static int tcf_mirred_delete(struct net *net, u32 index)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index e8b0bbd0883f..0cce0eadc28b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2167,6 +2167,7 @@ static int tc_exts_setup_cb_egdev_call(struct tcf_exts *exts,
if (!dev)
continue;
ret = tc_setup_cb_egdev_call(dev, type, type_data, err_stop);
+ dev_put(dev);
if (ret < 0)
return ret;
ok_count += ret;
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
2018-08-06 6:54 ` [PATCH net-next 12/14] net: sched: act_mirred: " Vlad Buslov
@ 2018-08-07 16:36 ` Jiri Pirko
2018-08-08 7:40 ` Vlad Buslov
0 siblings, 1 reply; 24+ messages in thread
From: Jiri Pirko @ 2018-08-07 16:36 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, marcelo.leitner
Mon, Aug 06, 2018 at 08:54:23AM CEST, vladbu@mellanox.com wrote:
[...]
>diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
>index 46b8c7f1c8d5..e6e475d788c6 100644
>--- a/include/net/tc_act/tc_tunnel_key.h
>+++ b/include/net/tc_act/tc_tunnel_key.h
>@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
>
> static inline bool is_tcf_tunnel_set(const struct tc_action *a)
> {
>+ bool ret = false;
> #ifdef CONFIG_NET_CLS_ACT
> struct tcf_tunnel_key *t = to_tunnel_key(a);
>- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
>+ struct tcf_tunnel_key_params *params;
>
>+ rcu_read_lock();
>+ params = rcu_dereference(t->params);
> if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
>- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>+ rcu_read_unlock();
> #endif
>- return false;
>+ return ret;
> }
>
> static inline bool is_tcf_tunnel_release(const struct tc_action *a)
Why are these tunnel things in a mirred patch?
[...]
> static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
> {
> struct tcf_mirred *m = to_mirred(a);
>+ struct net_device *dev;
>+
>+ rcu_read_lock();
>+ dev = rcu_dereference(m->tcfm_dev);
>+ if (dev)
>+ dev_hold(dev);
>+ rcu_read_unlock();
>
>- return rtnl_dereference(m->tcfm_dev);
>+ return dev;
> }
>
> static int tcf_mirred_delete(struct net *net, u32 index)
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index e8b0bbd0883f..0cce0eadc28b 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -2167,6 +2167,7 @@ static int tc_exts_setup_cb_egdev_call(struct tcf_exts *exts,
> if (!dev)
> continue;
> ret = tc_setup_cb_egdev_call(dev, type, type_data, err_stop);
>+ dev_put(dev);
This looks really odd. a->ops->put_dev is needed.
> if (ret < 0)
> return ret;
> ok_count += ret;
>--
>2.7.5
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
2018-08-07 16:36 ` Jiri Pirko
@ 2018-08-08 7:40 ` Vlad Buslov
2018-08-08 8:03 ` Jiri Pirko
0 siblings, 1 reply; 24+ messages in thread
From: Vlad Buslov @ 2018-08-08 7:40 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, marcelo.leitner
On Tue 07 Aug 2018 at 16:36, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Aug 06, 2018 at 08:54:23AM CEST, vladbu@mellanox.com wrote:
>
> [...]
>
>>diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
>>index 46b8c7f1c8d5..e6e475d788c6 100644
>>--- a/include/net/tc_act/tc_tunnel_key.h
>>+++ b/include/net/tc_act/tc_tunnel_key.h
>>@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
>>
>> static inline bool is_tcf_tunnel_set(const struct tc_action *a)
>> {
>>+ bool ret = false;
>> #ifdef CONFIG_NET_CLS_ACT
>> struct tcf_tunnel_key *t = to_tunnel_key(a);
>>- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
>>+ struct tcf_tunnel_key_params *params;
>>
>>+ rcu_read_lock();
>>+ params = rcu_dereference(t->params);
>> if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
>>- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>+ rcu_read_unlock();
>> #endif
>>- return false;
>>+ return ret;
>> }
>>
>> static inline bool is_tcf_tunnel_release(const struct tc_action *a)
>
> Why are these tunnel things in a mirred patch?
Mistake during re-slit. Will move those to tunnel_key patch.
>
>
> [...]
>
>
>> static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
>> {
>> struct tcf_mirred *m = to_mirred(a);
>>+ struct net_device *dev;
>>+
>>+ rcu_read_lock();
>>+ dev = rcu_dereference(m->tcfm_dev);
>>+ if (dev)
>>+ dev_hold(dev);
>>+ rcu_read_unlock();
>>
>>- return rtnl_dereference(m->tcfm_dev);
>>+ return dev;
>> }
>>
>> static int tcf_mirred_delete(struct net *net, u32 index)
>>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>index e8b0bbd0883f..0cce0eadc28b 100644
>>--- a/net/sched/cls_api.c
>>+++ b/net/sched/cls_api.c
>>@@ -2167,6 +2167,7 @@ static int tc_exts_setup_cb_egdev_call(struct tcf_exts *exts,
>> if (!dev)
>> continue;
>> ret = tc_setup_cb_egdev_call(dev, type, type_data, err_stop);
>>+ dev_put(dev);
>
> This looks really odd. a->ops->put_dev is needed.
Got it. Will implement put_dev in v2.
>
>
>> if (ret < 0)
>> return ret;
>> ok_count += ret;
>>--
>>2.7.5
>>
Thanks for reviewing my code!
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
2018-08-08 7:40 ` Vlad Buslov
@ 2018-08-08 8:03 ` Jiri Pirko
2018-08-08 8:47 ` Vlad Buslov
0 siblings, 1 reply; 24+ messages in thread
From: Jiri Pirko @ 2018-08-08 8:03 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, marcelo.leitner
Wed, Aug 08, 2018 at 09:40:35AM CEST, vladbu@mellanox.com wrote:
>
>On Tue 07 Aug 2018 at 16:36, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Aug 06, 2018 at 08:54:23AM CEST, vladbu@mellanox.com wrote:
>>
>> [...]
>>
>>>diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
>>>index 46b8c7f1c8d5..e6e475d788c6 100644
>>>--- a/include/net/tc_act/tc_tunnel_key.h
>>>+++ b/include/net/tc_act/tc_tunnel_key.h
>>>@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
>>>
>>> static inline bool is_tcf_tunnel_set(const struct tc_action *a)
>>> {
>>>+ bool ret = false;
>>> #ifdef CONFIG_NET_CLS_ACT
>>> struct tcf_tunnel_key *t = to_tunnel_key(a);
>>>- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
>>>+ struct tcf_tunnel_key_params *params;
>>>
>>>+ rcu_read_lock();
>>>+ params = rcu_dereference(t->params);
>>> if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
>>>- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>+ rcu_read_unlock();
>>> #endif
>>>- return false;
>>>+ return ret;
>>> }
>>>
>>> static inline bool is_tcf_tunnel_release(const struct tc_action *a)
>>
>> Why are these tunnel things in a mirred patch?
>
>Mistake during re-slit. Will move those to tunnel_key patch.
Are you sure that the changes are safe? I just quickly looked over it
and it smells:
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:
if (is_tcf_tunnel_set(a)) {
info = tcf_tunnel_info(a);
Why the "t->params" can't be nulled in the middle?
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
2018-08-08 8:03 ` Jiri Pirko
@ 2018-08-08 8:47 ` Vlad Buslov
2018-08-08 8:54 ` Jiri Pirko
0 siblings, 1 reply; 24+ messages in thread
From: Vlad Buslov @ 2018-08-08 8:47 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, marcelo.leitner
On Wed 08 Aug 2018 at 08:03, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Aug 08, 2018 at 09:40:35AM CEST, vladbu@mellanox.com wrote:
>>
>>On Tue 07 Aug 2018 at 16:36, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Mon, Aug 06, 2018 at 08:54:23AM CEST, vladbu@mellanox.com wrote:
>>>
>>> [...]
>>>
>>>>diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
>>>>index 46b8c7f1c8d5..e6e475d788c6 100644
>>>>--- a/include/net/tc_act/tc_tunnel_key.h
>>>>+++ b/include/net/tc_act/tc_tunnel_key.h
>>>>@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
>>>>
>>>> static inline bool is_tcf_tunnel_set(const struct tc_action *a)
>>>> {
>>>>+ bool ret = false;
>>>> #ifdef CONFIG_NET_CLS_ACT
>>>> struct tcf_tunnel_key *t = to_tunnel_key(a);
>>>>- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
>>>>+ struct tcf_tunnel_key_params *params;
>>>>
>>>>+ rcu_read_lock();
>>>>+ params = rcu_dereference(t->params);
>>>> if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
>>>>- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>>+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>>+ rcu_read_unlock();
>>>> #endif
>>>>- return false;
>>>>+ return ret;
>>>> }
>>>>
>>>> static inline bool is_tcf_tunnel_release(const struct tc_action *a)
>>>
>>> Why are these tunnel things in a mirred patch?
>>
>>Mistake during re-slit. Will move those to tunnel_key patch.
>
> Are you sure that the changes are safe? I just quickly looked over it
> and it smells:
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:
> if (is_tcf_tunnel_set(a)) {
> info = tcf_tunnel_info(a);
>
> Why the "t->params" can't be nulled in the middle?
First of all, no API is actually "unlocked" with this patch. It is a
preparation, rtnl mutex is still in use.
Callers of these functions will have to be updated, for example, to use
their _rcu version while holding rcu_read_lock.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
2018-08-08 8:47 ` Vlad Buslov
@ 2018-08-08 8:54 ` Jiri Pirko
2018-08-08 11:21 ` Vlad Buslov
0 siblings, 1 reply; 24+ messages in thread
From: Jiri Pirko @ 2018-08-08 8:54 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, marcelo.leitner
Wed, Aug 08, 2018 at 10:47:04AM CEST, vladbu@mellanox.com wrote:
>
>On Wed 08 Aug 2018 at 08:03, Jiri Pirko <jiri@resnulli.us> wrote:
>> Wed, Aug 08, 2018 at 09:40:35AM CEST, vladbu@mellanox.com wrote:
>>>
>>>On Tue 07 Aug 2018 at 16:36, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> Mon, Aug 06, 2018 at 08:54:23AM CEST, vladbu@mellanox.com wrote:
>>>>
>>>> [...]
>>>>
>>>>>diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
>>>>>index 46b8c7f1c8d5..e6e475d788c6 100644
>>>>>--- a/include/net/tc_act/tc_tunnel_key.h
>>>>>+++ b/include/net/tc_act/tc_tunnel_key.h
>>>>>@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
>>>>>
>>>>> static inline bool is_tcf_tunnel_set(const struct tc_action *a)
>>>>> {
>>>>>+ bool ret = false;
>>>>> #ifdef CONFIG_NET_CLS_ACT
>>>>> struct tcf_tunnel_key *t = to_tunnel_key(a);
>>>>>- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
>>>>>+ struct tcf_tunnel_key_params *params;
>>>>>
>>>>>+ rcu_read_lock();
>>>>>+ params = rcu_dereference(t->params);
>>>>> if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
>>>>>- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>>>+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>>>+ rcu_read_unlock();
>>>>> #endif
>>>>>- return false;
>>>>>+ return ret;
>>>>> }
>>>>>
>>>>> static inline bool is_tcf_tunnel_release(const struct tc_action *a)
>>>>
>>>> Why are these tunnel things in a mirred patch?
>>>
>>>Mistake during re-slit. Will move those to tunnel_key patch.
>>
>> Are you sure that the changes are safe? I just quickly looked over it
>> and it smells:
>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:
>> if (is_tcf_tunnel_set(a)) {
>> info = tcf_tunnel_info(a);
>>
>> Why the "t->params" can't be nulled in the middle?
>
>First of all, no API is actually "unlocked" with this patch. It is a
>preparation, rtnl mutex is still in use.
>
>Callers of these functions will have to be updated, for example, to use
>their _rcu version while holding rcu_read_lock.
I don't see any rcu version of these. I think that it would be good to
convert the callers to rcu and you can avoid these changes.
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 12/14] net: sched: act_mirred: remove dependency on rtnl lock
2018-08-08 8:54 ` Jiri Pirko
@ 2018-08-08 11:21 ` Vlad Buslov
0 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-08 11:21 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, xiyou.wangcong, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, marcelo.leitner
On Wed 08 Aug 2018 at 08:54, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Aug 08, 2018 at 10:47:04AM CEST, vladbu@mellanox.com wrote:
>>
>>On Wed 08 Aug 2018 at 08:03, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Wed, Aug 08, 2018 at 09:40:35AM CEST, vladbu@mellanox.com wrote:
>>>>
>>>>On Tue 07 Aug 2018 at 16:36, Jiri Pirko <jiri@resnulli.us> wrote:
>>>>> Mon, Aug 06, 2018 at 08:54:23AM CEST, vladbu@mellanox.com wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>>diff --git a/include/net/tc_act/tc_tunnel_key.h b/include/net/tc_act/tc_tunnel_key.h
>>>>>>index 46b8c7f1c8d5..e6e475d788c6 100644
>>>>>>--- a/include/net/tc_act/tc_tunnel_key.h
>>>>>>+++ b/include/net/tc_act/tc_tunnel_key.h
>>>>>>@@ -30,26 +30,47 @@ struct tcf_tunnel_key {
>>>>>>
>>>>>> static inline bool is_tcf_tunnel_set(const struct tc_action *a)
>>>>>> {
>>>>>>+ bool ret = false;
>>>>>> #ifdef CONFIG_NET_CLS_ACT
>>>>>> struct tcf_tunnel_key *t = to_tunnel_key(a);
>>>>>>- struct tcf_tunnel_key_params *params = rtnl_dereference(t->params);
>>>>>>+ struct tcf_tunnel_key_params *params;
>>>>>>
>>>>>>+ rcu_read_lock();
>>>>>>+ params = rcu_dereference(t->params);
>>>>>> if (a->ops && a->ops->type == TCA_ACT_TUNNEL_KEY)
>>>>>>- return params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>>>>+ ret = params->tcft_action == TCA_TUNNEL_KEY_ACT_SET;
>>>>>>+ rcu_read_unlock();
>>>>>> #endif
>>>>>>- return false;
>>>>>>+ return ret;
>>>>>> }
>>>>>>
>>>>>> static inline bool is_tcf_tunnel_release(const struct tc_action *a)
>>>>>
>>>>> Why are these tunnel things in a mirred patch?
>>>>
>>>>Mistake during re-slit. Will move those to tunnel_key patch.
>>>
>>> Are you sure that the changes are safe? I just quickly looked over it
>>> and it smells:
>>> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:
>>> if (is_tcf_tunnel_set(a)) {
>>> info = tcf_tunnel_info(a);
>>>
>>> Why the "t->params" can't be nulled in the middle?
>>
>>First of all, no API is actually "unlocked" with this patch. It is a
>>preparation, rtnl mutex is still in use.
>>
>>Callers of these functions will have to be updated, for example, to use
>>their _rcu version while holding rcu_read_lock.
>
> I don't see any rcu version of these. I think that it would be good to
> convert the callers to rcu and you can avoid these changes.
Understood. Will move helper function changes to standalone patch.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (11 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 12/14] net: sched: act_mirred: " Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
2018-08-08 1:37 ` Marcelo Ricardo Leitner
2018-08-06 6:54 ` [PATCH net-next 14/14] net: sched: act_police: remove dependency on rtnl lock Vlad Buslov
13 siblings, 1 reply; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Extend rate estimator 'new' and 'replace' APIs with additional spinlock
parameter to be used by rtnl-unlocked actions to protect rate_est pointer
from concurrent modification.
Extract code that requires synchronization from gen_new_estimator into
__replace_estimator function in order to be used by both locked and
unlocked paths.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
include/net/gen_stats.h | 2 ++
net/core/gen_estimator.c | 58 +++++++++++++++++++++++++++++++++++-----------
net/netfilter/xt_RATEEST.c | 2 +-
net/sched/act_api.c | 2 +-
net/sched/act_police.c | 2 +-
net/sched/sch_api.c | 2 ++
net/sched/sch_cbq.c | 4 ++--
net/sched/sch_drr.c | 4 ++--
net/sched/sch_hfsc.c | 4 ++--
net/sched/sch_htb.c | 4 ++--
net/sched/sch_qfq.c | 4 ++--
11 files changed, 61 insertions(+), 27 deletions(-)
diff --git a/include/net/gen_stats.h b/include/net/gen_stats.h
index 0304ba2ae353..d1ef63d16abc 100644
--- a/include/net/gen_stats.h
+++ b/include/net/gen_stats.h
@@ -59,12 +59,14 @@ int gnet_stats_finish_copy(struct gnet_dump *d);
int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **rate_est,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running, struct nlattr *opt);
void gen_kill_estimator(struct net_rate_estimator __rcu **ptr);
int gen_replace_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **ptr,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running, struct nlattr *opt);
bool gen_estimator_active(struct net_rate_estimator __rcu **ptr);
diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index 98fd12721221..012e37011147 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -107,11 +107,43 @@ static void est_timer(struct timer_list *t)
mod_timer(&est->timer, est->next_jiffies);
}
+static void __replace_estimator(struct net_rate_estimator __rcu **rate_est,
+ struct net_rate_estimator *new)
+{
+ struct net_rate_estimator *old = rcu_dereference_protected(*rate_est,
+ 1);
+
+ if (old) {
+ del_timer_sync(&old->timer);
+ new->avbps = old->avbps;
+ new->avpps = old->avpps;
+ }
+
+ new->next_jiffies = jiffies + ((HZ / 4) << new->intvl_log);
+ timer_setup(&new->timer, est_timer, 0);
+ mod_timer(&new->timer, new->next_jiffies);
+
+ rcu_assign_pointer(*rate_est, new);
+
+ if (old)
+ kfree_rcu(old, rcu);
+}
+
+static void replace_estimator(struct net_rate_estimator __rcu **rate_est,
+ struct net_rate_estimator *new,
+ spinlock_t *rate_est_lock)
+{
+ spin_lock_bh(rate_est_lock);
+ __replace_estimator(rate_est, new);
+ spin_unlock_bh(rate_est_lock);
+}
+
/**
* gen_new_estimator - create a new rate estimator
* @bstats: basic statistics
* @cpu_bstats: bstats per cpu
* @rate_est: rate estimator statistics
+ * @rate_est_lock: rate_est lock (might be NULL)
* @stats_lock: statistics lock
* @running: qdisc running seqcount
* @opt: rate estimator configuration TLV
@@ -128,12 +160,13 @@ static void est_timer(struct timer_list *t)
int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **rate_est,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running,
struct nlattr *opt)
{
struct gnet_estimator *parm = nla_data(opt);
- struct net_rate_estimator *old, *est;
+ struct net_rate_estimator *est;
struct gnet_stats_basic_packed b;
int intvl_log;
@@ -167,20 +200,15 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
local_bh_enable();
est->last_bytes = b.bytes;
est->last_packets = b.packets;
- old = rcu_dereference_protected(*rate_est, 1);
- if (old) {
- del_timer_sync(&old->timer);
- est->avbps = old->avbps;
- est->avpps = old->avpps;
- }
- est->next_jiffies = jiffies + ((HZ/4) << intvl_log);
- timer_setup(&est->timer, est_timer, 0);
- mod_timer(&est->timer, est->next_jiffies);
+ if (rate_est_lock)
+ replace_estimator(rate_est, est, rate_est_lock);
+ else
+ /* If no spinlock argument provided,
+ * then assume that caller is already synchronized.
+ */
+ __replace_estimator(rate_est, est);
- rcu_assign_pointer(*rate_est, est);
- if (old)
- kfree_rcu(old, rcu);
return 0;
}
EXPORT_SYMBOL(gen_new_estimator);
@@ -209,6 +237,7 @@ EXPORT_SYMBOL(gen_kill_estimator);
* @bstats: basic statistics
* @cpu_bstats: bstats per cpu
* @rate_est: rate estimator statistics
+ * @rate_est_lock: rate_est lock (might be NULL)
* @stats_lock: statistics lock
* @running: qdisc running seqcount (might be NULL)
* @opt: rate estimator configuration TLV
@@ -221,10 +250,11 @@ EXPORT_SYMBOL(gen_kill_estimator);
int gen_replace_estimator(struct gnet_stats_basic_packed *bstats,
struct gnet_stats_basic_cpu __percpu *cpu_bstats,
struct net_rate_estimator __rcu **rate_est,
+ spinlock_t *rate_est_lock,
spinlock_t *stats_lock,
seqcount_t *running, struct nlattr *opt)
{
- return gen_new_estimator(bstats, cpu_bstats, rate_est,
+ return gen_new_estimator(bstats, cpu_bstats, rate_est, rate_est_lock,
stats_lock, running, opt);
}
EXPORT_SYMBOL(gen_replace_estimator);
diff --git a/net/netfilter/xt_RATEEST.c b/net/netfilter/xt_RATEEST.c
index dec843cadf46..8e79bd50bc0b 100644
--- a/net/netfilter/xt_RATEEST.c
+++ b/net/netfilter/xt_RATEEST.c
@@ -154,7 +154,7 @@ static int xt_rateest_tg_checkentry(const struct xt_tgchk_param *par)
cfg.est.interval = info->interval;
cfg.est.ewma_log = info->ewma_log;
- ret = gen_new_estimator(&est->bstats, NULL, &est->rate_est,
+ ret = gen_new_estimator(&est->bstats, NULL, &est->rate_est, NULL,
&est->lock, NULL, &cfg.opt);
if (ret < 0)
goto err2;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 229d63c99be2..28c2d41f0cd2 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -401,7 +401,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
p->tcfa_tm.firstuse = 0;
if (est) {
err = gen_new_estimator(&p->tcfa_bstats, p->cpu_bstats,
- &p->tcfa_rate_est,
+ &p->tcfa_rate_est, NULL,
&p->tcfa_lock, NULL, est);
if (err)
goto err3;
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 1f3192ea8df7..3eb5fe60c62c 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -138,7 +138,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
if (est) {
err = gen_replace_estimator(&police->tcf_bstats, NULL,
- &police->tcf_rate_est,
+ &police->tcf_rate_est, NULL,
&police->tcf_lock,
NULL, est);
if (err)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 98541c6399db..57cdc105ce30 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1185,6 +1185,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
sch->cpu_bstats,
&sch->rate_est,
NULL,
+ NULL,
running,
tca[TCA_RATE]);
if (err) {
@@ -1260,6 +1261,7 @@ static int qdisc_change(struct Qdisc *sch, struct nlattr **tca,
sch->cpu_bstats,
&sch->rate_est,
NULL,
+ NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
}
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index f42025d53cfe..2a7ff53b4e7a 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -1503,7 +1503,7 @@ cbq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, struct nlattr **t
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -1605,7 +1605,7 @@ cbq_change_class(struct Qdisc *sch, u32 classid, u32 parentid, struct nlattr **t
if (tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL, &cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err) {
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index e0b0cf8a9939..0896e239063b 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -95,7 +95,7 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (cl != NULL) {
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -129,7 +129,7 @@ static int drr_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL, &cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err) {
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 3278a76f6861..be4ead89b50e 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -972,7 +972,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -1042,7 +1042,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL, &cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err) {
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 43c4bfe625a9..f782a1d31264 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1395,7 +1395,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
if (htb_rate_est || tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL,
&cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE] ? : &est.nla);
if (err) {
@@ -1460,7 +1460,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
} else {
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index bb1a9c11fc54..8026c1eebd79 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -461,7 +461,7 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (cl != NULL) { /* modify existing class */
if (tca[TCA_RATE]) {
err = gen_replace_estimator(&cl->bstats, NULL,
- &cl->rate_est,
+ &cl->rate_est, NULL,
NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
@@ -488,7 +488,7 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
if (tca[TCA_RATE]) {
err = gen_new_estimator(&cl->bstats, NULL,
&cl->rate_est,
- NULL,
+ NULL, NULL,
qdisc_root_sleeping_running(sch),
tca[TCA_RATE]);
if (err)
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter
2018-08-06 6:54 ` [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter Vlad Buslov
@ 2018-08-08 1:37 ` Marcelo Ricardo Leitner
2018-08-08 12:30 ` Vlad Buslov
0 siblings, 1 reply; 24+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-08-08 1:37 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast,
daniel, edumazet, keescook
On Mon, Aug 06, 2018 at 09:54:24AM +0300, Vlad Buslov wrote:
> Extend rate estimator 'new' and 'replace' APIs with additional spinlock
> parameter to be used by rtnl-unlocked actions to protect rate_est pointer
> from concurrent modification.
I'm wondering if this additional parameter is really needed. So far,
the only case in which it is not NULL, the same lock that is used to
protect the stats is also used in this new parameter.
...
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -138,7 +138,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
>
> if (est) {
> err = gen_replace_estimator(&police->tcf_bstats, NULL,
> - &police->tcf_rate_est,
> + &police->tcf_rate_est, NULL,
> &police->tcf_lock,
> NULL, est);
Which is here, and this new NULL arg is replaced by &police->tcf_lock
in the next patch.
Do you foresee a case in which a different lock will be used?
Or maybe it is because the current one is explicitly aimed towards the
stats?
Marcelo
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter
2018-08-08 1:37 ` Marcelo Ricardo Leitner
@ 2018-08-08 12:30 ` Vlad Buslov
2018-08-08 14:18 ` Marcelo Ricardo Leitner
0 siblings, 1 reply; 24+ messages in thread
From: Vlad Buslov @ 2018-08-08 12:30 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast,
daniel, edumazet, keescook
On Wed 08 Aug 2018 at 01:37, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> On Mon, Aug 06, 2018 at 09:54:24AM +0300, Vlad Buslov wrote:
>> Extend rate estimator 'new' and 'replace' APIs with additional spinlock
>> parameter to be used by rtnl-unlocked actions to protect rate_est pointer
>> from concurrent modification.
>
> I'm wondering if this additional parameter is really needed. So far,
> the only case in which it is not NULL, the same lock that is used to
> protect the stats is also used in this new parameter.
>
> ...
>
>> --- a/net/sched/act_police.c
>> +++ b/net/sched/act_police.c
>> @@ -138,7 +138,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
>>
>> if (est) {
>> err = gen_replace_estimator(&police->tcf_bstats, NULL,
>> - &police->tcf_rate_est,
>> + &police->tcf_rate_est, NULL,
>> &police->tcf_lock,
>> NULL, est);
>
> Which is here, and this new NULL arg is replaced by &police->tcf_lock
> in the next patch.
>
> Do you foresee a case in which a different lock will be used?
Not in my use-case, no.
> Or maybe it is because the current one is explicitly aimed towards the
> stats?
Yes, stats lock is only taken when fetching counters. You think better
approach would be to rely on the fact that, in case of police action,
same lock is already passed as stats lock? Having it as standalone
argument looked like cleaner approach to me. If you think this change is
too much code for very little benefit, I can reuse stats lock.
>
> Marcelo
Thank you for reviewing my code!
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter
2018-08-08 12:30 ` Vlad Buslov
@ 2018-08-08 14:18 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 24+ messages in thread
From: Marcelo Ricardo Leitner @ 2018-08-08 14:18 UTC (permalink / raw)
To: Vlad Buslov
Cc: netdev, davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast,
daniel, edumazet, keescook
On Wed, Aug 08, 2018 at 03:30:41PM +0300, Vlad Buslov wrote:
> On Wed 08 Aug 2018 at 01:37, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> > On Mon, Aug 06, 2018 at 09:54:24AM +0300, Vlad Buslov wrote:
> >> Extend rate estimator 'new' and 'replace' APIs with additional spinlock
> >> parameter to be used by rtnl-unlocked actions to protect rate_est pointer
> >> from concurrent modification.
> >
> > I'm wondering if this additional parameter is really needed. So far,
> > the only case in which it is not NULL, the same lock that is used to
> > protect the stats is also used in this new parameter.
> >
> > ...
> >
> >> --- a/net/sched/act_police.c
> >> +++ b/net/sched/act_police.c
> >> @@ -138,7 +138,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
> >>
> >> if (est) {
> >> err = gen_replace_estimator(&police->tcf_bstats, NULL,
> >> - &police->tcf_rate_est,
> >> + &police->tcf_rate_est, NULL,
> >> &police->tcf_lock,
> >> NULL, est);
> >
> > Which is here, and this new NULL arg is replaced by &police->tcf_lock
> > in the next patch.
> >
> > Do you foresee a case in which a different lock will be used?
>
> Not in my use-case, no.
>
> > Or maybe it is because the current one is explicitly aimed towards the
> > stats?
>
> Yes, stats lock is only taken when fetching counters. You think better
> approach would be to rely on the fact that, in case of police action,
> same lock is already passed as stats lock? Having it as standalone
And the fact that we have no foreseeable user of two different locks.
> argument looked like cleaner approach to me. If you think this change is
> too much code for very little benefit, I can reuse stats lock.
That's my current thinking, yes. Especially considering the amount of
parameters this function already has, I would refrain from adding yet
another unless really needed.
Maybe s/stats_lock/lock/ in function parameter (struct member doesn't
need to be changed) and doctext:
* @lock: lock for statistics and control path.
wdyt?
>
> >
> > Marcelo
>
> Thank you for reviewing my code!
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next 14/14] net: sched: act_police: remove dependency on rtnl lock
2018-08-06 6:54 [PATCH net-next 00/14] Remove rtnl lock dependency from all action implementations Vlad Buslov
` (12 preceding siblings ...)
2018-08-06 6:54 ` [PATCH net-next 13/14] net: core: add new/replace rate estimator lock parameter Vlad Buslov
@ 2018-08-06 6:54 ` Vlad Buslov
13 siblings, 0 replies; 24+ messages in thread
From: Vlad Buslov @ 2018-08-06 6:54 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, pablo, kadlec, fw, ast, daniel,
edumazet, keescook, marcelo.leitner, Vlad Buslov
Use tcf spinlock to protect police action private data from concurrent
modification during dump. (init already uses tcf spinlock when changing
police action state)
Pass tcf spinlock as estimator lock argument to gen_replace_estimator()
during action init.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_police.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 3eb5fe60c62c..4777da7caadf 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -138,7 +138,8 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
if (est) {
err = gen_replace_estimator(&police->tcf_bstats, NULL,
- &police->tcf_rate_est, NULL,
+ &police->tcf_rate_est,
+ &police->tcf_lock,
&police->tcf_lock,
NULL, est);
if (err)
@@ -274,14 +275,15 @@ static int tcf_act_police_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_police *police = to_police(a);
struct tc_police opt = {
.index = police->tcf_index,
- .action = police->tcf_action,
- .mtu = police->tcfp_mtu,
- .burst = PSCHED_NS2TICKS(police->tcfp_burst),
.refcnt = refcount_read(&police->tcf_refcnt) - ref,
.bindcnt = atomic_read(&police->tcf_bindcnt) - bind,
};
struct tcf_t t;
+ spin_lock_bh(&police->tcf_lock);
+ opt.action = police->tcf_action;
+ opt.mtu = police->tcfp_mtu;
+ opt.burst = PSCHED_NS2TICKS(police->tcfp_burst);
if (police->rate_present)
psched_ratecfg_getrate(&opt.rate, &police->rate);
if (police->peak_present)
@@ -301,10 +303,12 @@ static int tcf_act_police_dump(struct sk_buff *skb, struct tc_action *a,
t.expires = jiffies_to_clock_t(police->tcf_tm.expires);
if (nla_put_64bit(skb, TCA_POLICE_TM, sizeof(t), &t, TCA_POLICE_PAD))
goto nla_put_failure;
+ spin_unlock_bh(&police->tcf_lock);
return skb->len;
nla_put_failure:
+ spin_unlock_bh(&police->tcf_lock);
nlmsg_trim(skb, b);
return -1;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 24+ messages in thread