* [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-12 3:23 ` kbuild test robot
` (2 more replies)
2018-12-11 10:11 ` [PATCH net-next v2 02/17] net: sched: protect block state with spinlock Vlad Buslov
` (15 subsequent siblings)
16 siblings, 3 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
As a part of the effort to remove dependency on rtnl lock, cls API is being
converted to use fine-grained locking mechanisms instead of global rtnl
lock. However, chain_head_change callback for ingress Qdisc is a sleeping
function and cannot be executed while holding a spinlock.
Extend cls API with new workqueue intended to be used for tcf_proto
lifetime management. Modify tcf_proto_destroy() to deallocate proto
asynchronously on workqueue in order to ensure that all chain_head_change
callbacks involving the proto complete before it is freed. Convert
mini_qdisc_pair_swap(), that is used as a chain_head_change callback for
ingress and clsact Qdiscs, to use a workqueue. Move Qdisc deallocation to
tc_proto_wq ordered workqueue that is used to destroy tcf proto instances.
This is necessary to ensure that Qdisc is destroyed after all instances of
chain/proto that it contains in order to prevent use-after-free error in
tc_chain_notify_delete().
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Suggested-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Use smp_store_release() instead of xchg() for setting
miniqp->tp_head.
- Move Qdisc deallocation to tc_proto_wq ordered workqueue that is
used to destroy tcf proto instances. This is necessary to ensure
that Qdisc is destroyed after all instances of chain/proto that it
contains in order to prevent use-after-free error in
tc_chain_notify_delete().
- Cache parent net device ifindex in block to prevent use-after-free
of dev queue in tc_chain_notify_delete().
include/net/sch_generic.h | 14 +++++++++++-
net/sched/cls_api.c | 52 ++++++++++++++++++++++++++++++++++++-------
net/sched/sch_generic.c | 56 +++++++++++++++++++++++++++++++++++++++++++----
net/sched/sch_ingress.c | 4 ++++
4 files changed, 113 insertions(+), 13 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 9481f2c142e2..80fc06468c79 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -109,7 +109,13 @@ struct Qdisc {
spinlock_t busylock ____cacheline_aligned_in_smp;
spinlock_t seqlock;
- struct rcu_head rcu;
+ int ifindex; /* cached interface index of parent net
+ * device
+ */
+ union {
+ struct rcu_head rcu;
+ struct work_struct work;
+ };
};
static inline void qdisc_refcount_inc(struct Qdisc *qdisc)
@@ -322,6 +328,7 @@ struct tcf_proto {
const struct tcf_proto_ops *ops;
struct tcf_chain *chain;
struct rcu_head rcu;
+ struct work_struct work;
};
struct qdisc_skb_cb {
@@ -400,6 +407,8 @@ tc_cls_offload_cnt_update(struct tcf_block *block, u32 *cnt,
}
}
+bool tc_queue_proto_work(struct work_struct *work);
+
static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
{
struct qdisc_skb_cb *qcb;
@@ -1175,12 +1184,15 @@ struct mini_Qdisc_pair {
struct mini_Qdisc miniq1;
struct mini_Qdisc miniq2;
struct mini_Qdisc __rcu **p_miniq;
+ struct tcf_proto *tp_head;
+ struct work_struct work;
};
void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
struct tcf_proto *tp_head);
void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
struct mini_Qdisc __rcu **p_miniq);
+void mini_qdisc_pair_cleanup(struct mini_Qdisc_pair *miniqp);
static inline void skb_tc_reinsert(struct sk_buff *skb, struct tcf_result *res)
{
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d92f44ac4c39..08f245abf04a 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -108,12 +108,14 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops)
EXPORT_SYMBOL(register_tcf_proto_ops);
static struct workqueue_struct *tc_filter_wq;
+static struct workqueue_struct *tc_proto_wq;
int unregister_tcf_proto_ops(struct tcf_proto_ops *ops)
{
struct tcf_proto_ops *t;
int rc = -ENOENT;
+ flush_workqueue(tc_proto_wq);
/* Wait for outstanding call_rcu()s, if any, from a
* tcf_proto_ops's destroy() handler.
*/
@@ -140,6 +142,12 @@ bool tcf_queue_work(struct rcu_work *rwork, work_func_t func)
}
EXPORT_SYMBOL(tcf_queue_work);
+bool tc_queue_proto_work(struct work_struct *work)
+{
+ return queue_work(tc_proto_wq, work);
+}
+EXPORT_SYMBOL(tc_queue_proto_work);
+
/* Select new prio value from the range, managed by kernel. */
static inline u32 tcf_auto_prio(struct tcf_proto *tp)
@@ -152,6 +160,23 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp)
return TC_H_MAJ(first);
}
+static void tcf_chain_put(struct tcf_chain *chain);
+
+static void tcf_proto_destroy_work(struct work_struct *work)
+{
+ struct tcf_proto *tp = container_of(work, struct tcf_proto, work);
+ struct tcf_chain *chain = tp->chain;
+
+ rtnl_lock();
+
+ tp->ops->destroy(tp, NULL);
+ module_put(tp->ops->owner);
+ kfree_rcu(tp, rcu);
+ tcf_chain_put(chain);
+
+ rtnl_unlock();
+}
+
static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
u32 prio, struct tcf_chain *chain,
struct netlink_ext_ack *extack)
@@ -172,6 +197,7 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
tp->protocol = protocol;
tp->prio = prio;
tp->chain = chain;
+ INIT_WORK(&tp->work, tcf_proto_destroy_work);
err = tp->ops->init(tp);
if (err) {
@@ -188,9 +214,7 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
static void tcf_proto_destroy(struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
- tp->ops->destroy(tp, extack);
- module_put(tp->ops->owner);
- kfree_rcu(tp, rcu);
+ tc_queue_proto_work(&tp->work);
}
struct tcf_filter_chain_list_item {
@@ -362,7 +386,6 @@ static void tcf_chain_flush(struct tcf_chain *chain)
RCU_INIT_POINTER(chain->filter_chain, tp->next);
tcf_proto_destroy(tp, NULL);
tp = rtnl_dereference(chain->filter_chain);
- tcf_chain_put(chain);
}
}
@@ -1378,7 +1401,6 @@ static void tcf_chain_tp_remove(struct tcf_chain *chain,
if (tp == chain->filter_chain)
tcf_chain0_head_change(chain, next);
RCU_INIT_POINTER(*chain_info->pprev, next);
- tcf_chain_put(chain);
}
static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
@@ -1425,7 +1447,7 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
tcm->tcm__pad1 = 0;
tcm->tcm__pad2 = 0;
if (q) {
- tcm->tcm_ifindex = qdisc_dev(q)->ifindex;
+ tcm->tcm_ifindex = q->ifindex;
tcm->tcm_parent = parent;
} else {
tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK;
@@ -1667,8 +1689,14 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_NEWTFILTER, false);
} else {
- if (tp_created)
+ if (tp_created) {
+ /* tp wasn't inserted to chain tp list. Take reference
+ * to chain manually for tcf_proto_destroy() to
+ * release.
+ */
+ tcf_chain_hold(chain);
tcf_proto_destroy(tp, NULL);
+ }
}
errout:
@@ -2074,7 +2102,7 @@ static int tc_chain_fill_node(struct tcf_chain *chain, struct net *net,
tcm->tcm__pad2 = 0;
tcm->tcm_handle = 0;
if (block->q) {
- tcm->tcm_ifindex = qdisc_dev(block->q)->ifindex;
+ tcm->tcm_ifindex = block->q->ifindex;
tcm->tcm_parent = block->q->handle;
} else {
tcm->tcm_ifindex = TCM_IFINDEX_MAGIC_BLOCK;
@@ -2598,6 +2626,12 @@ static int __init tc_filter_init(void)
if (!tc_filter_wq)
return -ENOMEM;
+ tc_proto_wq = alloc_ordered_workqueue("tc_proto_workqueue", 0);
+ if (!tc_proto_wq) {
+ err = -ENOMEM;
+ goto err_proto_wq;
+ }
+
err = register_pernet_subsys(&tcf_net_ops);
if (err)
goto err_register_pernet_subsys;
@@ -2621,6 +2655,8 @@ static int __init tc_filter_init(void)
err_rhash_setup_block_ht:
unregister_pernet_subsys(&tcf_net_ops);
err_register_pernet_subsys:
+ destroy_workqueue(tc_proto_wq);
+err_proto_wq:
destroy_workqueue(tc_filter_wq);
return err;
}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index de1663f7d3ad..617b1dbceb01 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -28,6 +28,7 @@
#include <linux/if_vlan.h>
#include <linux/skb_array.h>
#include <linux/if_macvlan.h>
+#include <asm/barrier.h>
#include <net/sch_generic.h>
#include <net/pkt_sched.h>
#include <net/dst.h>
@@ -881,6 +882,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
sch->enqueue = ops->enqueue;
sch->dequeue = ops->dequeue;
sch->dev_queue = dev_queue;
+ sch->ifindex = dev->ifindex;
dev_hold(dev);
refcount_set(&sch->refcnt, 1);
@@ -953,11 +955,19 @@ void qdisc_free(struct Qdisc *qdisc)
kfree((char *) qdisc - qdisc->padded);
}
+static void qdisc_free_work(struct work_struct *work)
+{
+ struct Qdisc *qdisc = container_of(work, struct Qdisc, work);
+
+ qdisc_free(qdisc);
+}
+
static void qdisc_free_cb(struct rcu_head *head)
{
struct Qdisc *q = container_of(head, struct Qdisc, rcu);
- qdisc_free(q);
+ INIT_WORK(&q->work, qdisc_free_work);
+ tc_queue_proto_work(&q->work);
}
static void qdisc_destroy(struct Qdisc *qdisc)
@@ -1363,10 +1373,14 @@ static void mini_qdisc_rcu_func(struct rcu_head *head)
{
}
-void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
- struct tcf_proto *tp_head)
+void __mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
+ struct tcf_proto *tp_head)
{
- struct mini_Qdisc *miniq_old = rtnl_dereference(*miniqp->p_miniq);
+ /* p_miniq is either changed on ordered workqueue or during miniqp
+ * cleanup. In both cases no concurrent modification is possible.
+ */
+ struct mini_Qdisc *miniq_old =
+ rcu_dereference_protected(*miniqp->p_miniq, 1);
struct mini_Qdisc *miniq;
if (!tp_head) {
@@ -1394,6 +1408,32 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
*/
call_rcu_bh(&miniq_old->rcu, mini_qdisc_rcu_func);
}
+
+void mini_qdisc_pair_swap_work(struct work_struct *work)
+{
+ struct mini_Qdisc_pair *miniqp = container_of(work,
+ struct mini_Qdisc_pair,
+ work);
+ struct tcf_proto *tp_head;
+
+ /* Reset tp_head pointer to error in order to differentiate it from NULL
+ * pointer, which is a valid value. Paired with smp_store_release().
+ */
+ tp_head = xchg(&miniqp->tp_head, ERR_PTR(-ENOENT));
+ if (!IS_ERR(tp_head))
+ __mini_qdisc_pair_swap(miniqp, tp_head);
+}
+
+void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
+ struct tcf_proto *tp_head)
+{
+ /* Set tp_head to new value. We don't care if tp_head already contains
+ * some valid pointer, we just need to set miniq->filter_list to current
+ * tp_head.
+ */
+ smp_store_release(&miniqp->tp_head, tp_head);
+ tc_queue_proto_work(&miniqp->work);
+}
EXPORT_SYMBOL(mini_qdisc_pair_swap);
void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
@@ -1404,5 +1444,13 @@ void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
miniqp->miniq2.cpu_bstats = qdisc->cpu_bstats;
miniqp->miniq2.cpu_qstats = qdisc->cpu_qstats;
miniqp->p_miniq = p_miniq;
+ INIT_WORK(&miniqp->work, mini_qdisc_pair_swap_work);
}
EXPORT_SYMBOL(mini_qdisc_pair_init);
+
+void mini_qdisc_pair_cleanup(struct mini_Qdisc_pair *miniqp)
+{
+ cancel_work_sync(&miniqp->work);
+ __mini_qdisc_pair_swap(miniqp, NULL);
+}
+EXPORT_SYMBOL(mini_qdisc_pair_cleanup);
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index ce3f55259d0d..8edc3c46bd9c 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -99,6 +99,7 @@ static void ingress_destroy(struct Qdisc *sch)
struct ingress_sched_data *q = qdisc_priv(sch);
tcf_block_put_ext(q->block, sch, &q->block_info);
+ mini_qdisc_pair_cleanup(&q->miniqp);
net_dec_ingress_queue();
}
@@ -245,6 +246,9 @@ static void clsact_destroy(struct Qdisc *sch)
tcf_block_put_ext(q->egress_block, sch, &q->egress_block_info);
tcf_block_put_ext(q->ingress_block, sch, &q->ingress_block_info);
+ mini_qdisc_pair_cleanup(&q->miniqp_ingress);
+ mini_qdisc_pair_cleanup(&q->miniqp_egress);
+
net_dec_ingress_queue();
net_dec_egress_queue();
}
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-11 10:11 ` [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue Vlad Buslov
@ 2018-12-12 3:23 ` kbuild test robot
2018-12-13 23:32 ` Cong Wang
2018-12-19 8:12 ` kbuild test robot
2 siblings, 0 replies; 30+ messages in thread
From: kbuild test robot @ 2018-12-12 3:23 UTC (permalink / raw)
To: Vlad Buslov
Cc: kbuild-all, netdev, jhs, xiyou.wangcong, jiri, davem, ast, daniel,
Vlad Buslov
[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]
Hi Vlad,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Vlad-Buslov/Refactor-classifier-API-to-work-with-chain-classifiers-without-rtnl-lock/20181211-231158
config: mips-fuloong2e_defconfig (attached as .config)
compiler: mips64el-linux-gnuabi64-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=mips
All errors (new ones prefixed by >>):
net/sched/sch_generic.o: In function `qdisc_free_cb':
>> sch_generic.c:(.text+0xd34): undefined reference to `tc_queue_proto_work'
net/sched/sch_generic.o: In function `mini_qdisc_pair_swap':
sch_generic.c:(.text+0xd48): undefined reference to `tc_queue_proto_work'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 17988 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-11 10:11 ` [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue Vlad Buslov
2018-12-12 3:23 ` kbuild test robot
@ 2018-12-13 23:32 ` Cong Wang
2018-12-16 16:32 ` Vlad Buslov
2018-12-19 8:12 ` kbuild test robot
2 siblings, 1 reply; 30+ messages in thread
From: Cong Wang @ 2018-12-13 23:32 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Alexei Starovoitov, Daniel Borkmann
On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> As a part of the effort to remove dependency on rtnl lock, cls API is being
> converted to use fine-grained locking mechanisms instead of global rtnl
> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
> function and cannot be executed while holding a spinlock.
Why does it have to be a spinlock not a mutex?
I've read your cover letter and this changelog, I don't find any answer.
>
> Extend cls API with new workqueue intended to be used for tcf_proto
> lifetime management. Modify tcf_proto_destroy() to deallocate proto
> asynchronously on workqueue in order to ensure that all chain_head_change
> callbacks involving the proto complete before it is freed. Convert
> mini_qdisc_pair_swap(), that is used as a chain_head_change callback for
> ingress and clsact Qdiscs, to use a workqueue. Move Qdisc deallocation to
> tc_proto_wq ordered workqueue that is used to destroy tcf proto instances.
> This is necessary to ensure that Qdisc is destroyed after all instances of
> chain/proto that it contains in order to prevent use-after-free error in
> tc_chain_notify_delete().
Please avoid async unless you have to, there are almost always bugs
when playing with deferred workqueue or any other callbacks.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-13 23:32 ` Cong Wang
@ 2018-12-16 16:32 ` Vlad Buslov
2018-12-16 18:52 ` Cong Wang
0 siblings, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2018-12-16 16:32 UTC (permalink / raw)
To: Cong Wang, Jiri Pirko
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Alexei Starovoitov, Daniel Borkmann
On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> As a part of the effort to remove dependency on rtnl lock, cls API is being
>> converted to use fine-grained locking mechanisms instead of global rtnl
>> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
>> function and cannot be executed while holding a spinlock.
>
>
> Why does it have to be a spinlock not a mutex?
>
> I've read your cover letter and this changelog, I don't find any
> answer.
My initial implementation used mutex. However, it was changed to
spinlock by Jiri's request during internal review.
>
>>
>> Extend cls API with new workqueue intended to be used for tcf_proto
>> lifetime management. Modify tcf_proto_destroy() to deallocate proto
>> asynchronously on workqueue in order to ensure that all chain_head_change
>> callbacks involving the proto complete before it is freed. Convert
>> mini_qdisc_pair_swap(), that is used as a chain_head_change callback for
>> ingress and clsact Qdiscs, to use a workqueue. Move Qdisc deallocation to
>> tc_proto_wq ordered workqueue that is used to destroy tcf proto instances.
>> This is necessary to ensure that Qdisc is destroyed after all instances of
>> chain/proto that it contains in order to prevent use-after-free error in
>> tc_chain_notify_delete().
>
>
> Please avoid async unless you have to, there are almost always bugs
> when playing with deferred workqueue or any other callbacks.
Indeed, async Qdisc and tp deallocation introduces additional
complexity. What approach would you recommend to make chain_head_change
callback atomic?
>
> Thanks.
Thank you for reviewing my code!
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-16 16:32 ` Vlad Buslov
@ 2018-12-16 18:52 ` Cong Wang
2018-12-17 10:22 ` Jiri Pirko
0 siblings, 1 reply; 30+ messages in thread
From: Cong Wang @ 2018-12-16 18:52 UTC (permalink / raw)
To: Vlad Buslov
Cc: Jiri Pirko, Linux Kernel Network Developers, Jamal Hadi Salim,
David Miller, Alexei Starovoitov, Daniel Borkmann
On Sun, Dec 16, 2018 at 8:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >> As a part of the effort to remove dependency on rtnl lock, cls API is being
> >> converted to use fine-grained locking mechanisms instead of global rtnl
> >> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
> >> function and cannot be executed while holding a spinlock.
> >
> >
> > Why does it have to be a spinlock not a mutex?
> >
> > I've read your cover letter and this changelog, I don't find any
> > answer.
>
> My initial implementation used mutex. However, it was changed to
> spinlock by Jiri's request during internal review.
>
So what's the answer to my question? :)
> >
> >>
> >> Extend cls API with new workqueue intended to be used for tcf_proto
> >> lifetime management. Modify tcf_proto_destroy() to deallocate proto
> >> asynchronously on workqueue in order to ensure that all chain_head_change
> >> callbacks involving the proto complete before it is freed. Convert
> >> mini_qdisc_pair_swap(), that is used as a chain_head_change callback for
> >> ingress and clsact Qdiscs, to use a workqueue. Move Qdisc deallocation to
> >> tc_proto_wq ordered workqueue that is used to destroy tcf proto instances.
> >> This is necessary to ensure that Qdisc is destroyed after all instances of
> >> chain/proto that it contains in order to prevent use-after-free error in
> >> tc_chain_notify_delete().
> >
> >
> > Please avoid async unless you have to, there are almost always bugs
> > when playing with deferred workqueue or any other callbacks.
>
> Indeed, async Qdisc and tp deallocation introduces additional
> complexity. What approach would you recommend to make chain_head_change
> callback atomic?
I don't look into any of your code yet, from my understanding of your
changelog, it seems all these workqueue stuffs can be gone if you can
make it a mutex instead of a spinlock.
This is why I stopped here and wait for your answer to my above question.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-16 18:52 ` Cong Wang
@ 2018-12-17 10:22 ` Jiri Pirko
2018-12-19 4:27 ` Cong Wang
0 siblings, 1 reply; 30+ messages in thread
From: Jiri Pirko @ 2018-12-17 10:22 UTC (permalink / raw)
To: Cong Wang
Cc: Vlad Buslov, Linux Kernel Network Developers, Jamal Hadi Salim,
David Miller, Alexei Starovoitov, Daniel Borkmann
Sun, Dec 16, 2018 at 07:52:18PM CET, xiyou.wangcong@gmail.com wrote:
>On Sun, Dec 16, 2018 at 8:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >>
>> >> As a part of the effort to remove dependency on rtnl lock, cls API is being
>> >> converted to use fine-grained locking mechanisms instead of global rtnl
>> >> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
>> >> function and cannot be executed while holding a spinlock.
>> >
>> >
>> > Why does it have to be a spinlock not a mutex?
>> >
>> > I've read your cover letter and this changelog, I don't find any
>> > answer.
>>
>> My initial implementation used mutex. However, it was changed to
>> spinlock by Jiri's request during internal review.
>>
>
>So what's the answer to my question? :)
Yeah, my concern agains mutexes was that it would be needed to have one
per every block and per every chain. I find it quite heavy and I believe
it is better to use spinlock in those cases. This patch is a side effect
of that. Do you think it would be better to have mutexes instead of
spinlocks?
>
>
>> >
>> >>
>> >> Extend cls API with new workqueue intended to be used for tcf_proto
>> >> lifetime management. Modify tcf_proto_destroy() to deallocate proto
>> >> asynchronously on workqueue in order to ensure that all chain_head_change
>> >> callbacks involving the proto complete before it is freed. Convert
>> >> mini_qdisc_pair_swap(), that is used as a chain_head_change callback for
>> >> ingress and clsact Qdiscs, to use a workqueue. Move Qdisc deallocation to
>> >> tc_proto_wq ordered workqueue that is used to destroy tcf proto instances.
>> >> This is necessary to ensure that Qdisc is destroyed after all instances of
>> >> chain/proto that it contains in order to prevent use-after-free error in
>> >> tc_chain_notify_delete().
>> >
>> >
>> > Please avoid async unless you have to, there are almost always bugs
>> > when playing with deferred workqueue or any other callbacks.
>>
>> Indeed, async Qdisc and tp deallocation introduces additional
>> complexity. What approach would you recommend to make chain_head_change
>> callback atomic?
>
>I don't look into any of your code yet, from my understanding of your
>changelog, it seems all these workqueue stuffs can be gone if you can
>make it a mutex instead of a spinlock.
>
>This is why I stopped here and wait for your answer to my above question.
>
>Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-17 10:22 ` Jiri Pirko
@ 2018-12-19 4:27 ` Cong Wang
2018-12-20 13:55 ` Vlad Buslov
0 siblings, 1 reply; 30+ messages in thread
From: Cong Wang @ 2018-12-19 4:27 UTC (permalink / raw)
To: Jiri Pirko
Cc: Vlad Buslov, Linux Kernel Network Developers, Jamal Hadi Salim,
David Miller, Alexei Starovoitov, Daniel Borkmann
On Mon, Dec 17, 2018 at 2:30 AM Jiri Pirko <jiri@resnulli.us> wrote:
>
> Sun, Dec 16, 2018 at 07:52:18PM CET, xiyou.wangcong@gmail.com wrote:
> >On Sun, Dec 16, 2018 at 8:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >> On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> > On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >> >>
> >> >> As a part of the effort to remove dependency on rtnl lock, cls API is being
> >> >> converted to use fine-grained locking mechanisms instead of global rtnl
> >> >> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
> >> >> function and cannot be executed while holding a spinlock.
> >> >
> >> >
> >> > Why does it have to be a spinlock not a mutex?
> >> >
> >> > I've read your cover letter and this changelog, I don't find any
> >> > answer.
> >>
> >> My initial implementation used mutex. However, it was changed to
> >> spinlock by Jiri's request during internal review.
> >>
> >
> >So what's the answer to my question? :)
>
> Yeah, my concern agains mutexes was that it would be needed to have one
> per every block and per every chain. I find it quite heavy and I believe
> it is better to use spinlock in those cases. This patch is a side effect
> of that. Do you think it would be better to have mutexes instead of
> spinlocks?
My only concern with spinlock is we have to go async as we
can't block. This is almost always error-prone especially
when dealing with tcf block. I had to give up with spinlock
for idrinfo->lock, please take a look at:
commit 95278ddaa15cfa23e4a06ee9ed7b6ee0197c500b
Author: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue Oct 2 12:50:19 2018 -0700
net_sched: convert idrinfo->lock from spinlock to a mutex
There are indeed some cases in kernel we do take multiple
mutex'es, for example,
/*
* bd_mutex locking:
*
* mutex_lock(part->bd_mutex)
* mutex_lock_nested(whole->bd_mutex, 1)
*/
So, how heavy are they comparing with spinlocks?
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-19 4:27 ` Cong Wang
@ 2018-12-20 13:55 ` Vlad Buslov
2019-01-14 19:00 ` Vlad Buslov
0 siblings, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2018-12-20 13:55 UTC (permalink / raw)
To: Cong Wang, Jiri Pirko
Cc: Jiri Pirko, Linux Kernel Network Developers, Jamal Hadi Salim,
David Miller, Alexei Starovoitov, Daniel Borkmann
On Wed 19 Dec 2018 at 04:27, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Dec 17, 2018 at 2:30 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> Sun, Dec 16, 2018 at 07:52:18PM CET, xiyou.wangcong@gmail.com wrote:
>> >On Sun, Dec 16, 2018 at 8:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >>
>> >> On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> >> > On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >> >>
>> >> >> As a part of the effort to remove dependency on rtnl lock, cls API is being
>> >> >> converted to use fine-grained locking mechanisms instead of global rtnl
>> >> >> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
>> >> >> function and cannot be executed while holding a spinlock.
>> >> >
>> >> >
>> >> > Why does it have to be a spinlock not a mutex?
>> >> >
>> >> > I've read your cover letter and this changelog, I don't find any
>> >> > answer.
>> >>
>> >> My initial implementation used mutex. However, it was changed to
>> >> spinlock by Jiri's request during internal review.
>> >>
>> >
>> >So what's the answer to my question? :)
>>
>> Yeah, my concern agains mutexes was that it would be needed to have one
>> per every block and per every chain. I find it quite heavy and I believe
>> it is better to use spinlock in those cases. This patch is a side effect
>> of that. Do you think it would be better to have mutexes instead of
>> spinlocks?
>
> My only concern with spinlock is we have to go async as we
> can't block. This is almost always error-prone especially
> when dealing with tcf block. I had to give up with spinlock
> for idrinfo->lock, please take a look at:
>
> commit 95278ddaa15cfa23e4a06ee9ed7b6ee0197c500b
> Author: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Tue Oct 2 12:50:19 2018 -0700
>
> net_sched: convert idrinfo->lock from spinlock to a mutex
>
>
> There are indeed some cases in kernel we do take multiple
> mutex'es, for example,
>
> /*
> * bd_mutex locking:
> *
> * mutex_lock(part->bd_mutex)
> * mutex_lock_nested(whole->bd_mutex, 1)
> */
>
> So, how heavy are they comparing with spinlocks?
>
> Thanks.
Hi Cong,
I did quick-and-dirty mutex-based implementation of my cls API patchset
(removed async miniqp refactoring, changed block and chain lock types to
mutex lock) and performed some benchmarks to determine exact mutex
impact on cls API performance (both rule and chains API).
1. Parallel flower insertion rate (create 5 million flower filters on
same chain/prio with 10 tc instances) - same performance:
SPINLOCK
$ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
real 0m12.183s
user 0m35.013s
sys 1m24.947s
MUTEX
$ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
real 0m12.246s
user 0m35.417s
sys 1m25.330s
2. Parallel flower insertion rate (create 100k flower filters, each on
new instance of chain/tp, 10 tc instances) - mutex implementation is
~17% slower:
SPINLOCK:
$ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
real 2m19.285s
user 0m1.863s
sys 5m41.157s
MUTEX:
multichain$ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
real 2m43.495s
user 0m1.664s
sys 8m32.483s
3. Chains insertion rate with cls chain API (create 100k empty chains, 1
tc instance) - ~3% difference:
SPINLOCK:
$ time sudo tc -b add_chains
real 0m46.822s
user 0m0.239s
sys 0m46.437s
MUTEX:
$ time sudo tc -b add_chains
real 0m48.600s
user 0m0.230s
sys 0m48.227s
Only case where performance is significantly impacted by mutex is second
test. This happens because chain lookup is a linear search that is
performed while holding highly contested block lock. Perf profile for
mutex version confirms this:
+ 91.83% 0.00% tc [kernel.vmlinux] [k] entry_SYSCALL_64_after_hwframe
+ 91.83% 0.00% tc [kernel.vmlinux] [k] do_syscall_64
+ 91.80% 0.00% tc libc-2.25.so [.] __libc_sendmsg
+ 91.78% 0.00% tc [kernel.vmlinux] [k] __sys_sendmsg
+ 91.77% 0.00% tc [kernel.vmlinux] [k] ___sys_sendmsg
+ 91.77% 0.00% tc [kernel.vmlinux] [k] sock_sendmsg
+ 91.77% 0.00% tc [kernel.vmlinux] [k] netlink_sendmsg
+ 91.77% 0.01% tc [kernel.vmlinux] [k] netlink_unicast
+ 91.77% 0.00% tc [kernel.vmlinux] [k] netlink_rcv_skb
+ 91.71% 0.01% tc [kernel.vmlinux] [k] rtnetlink_rcv_msg
+ 91.69% 0.03% tc [kernel.vmlinux] [k] tc_new_tfilter
+ 90.96% 26.45% tc [kernel.vmlinux] [k] __tcf_chain_get
+ 64.30% 0.01% tc [kernel.vmlinux] [k] __mutex_lock.isra.7
+ 39.36% 39.18% tc [kernel.vmlinux] [k] osq_lock
+ 24.92% 24.81% tc [kernel.vmlinux] [k] mutex_spin_on_owner
Based on these results we can conclude that use-cases with significant
amount of chains on single block will be affected by using mutex in cls
API.
Regards,
Vlad
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-20 13:55 ` Vlad Buslov
@ 2019-01-14 19:00 ` Vlad Buslov
2019-01-14 23:00 ` Cong Wang
0 siblings, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2019-01-14 19:00 UTC (permalink / raw)
To: Cong Wang, Jiri Pirko
Cc: Jiri Pirko, Linux Kernel Network Developers, Jamal Hadi Salim,
David Miller, Alexei Starovoitov, Daniel Borkmann
On Thu 20 Dec 2018 at 13:55, Vlad Buslov <vladbu@mellanox.com> wrote:
> On Wed 19 Dec 2018 at 04:27, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> On Mon, Dec 17, 2018 at 2:30 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>>
>>> Sun, Dec 16, 2018 at 07:52:18PM CET, xiyou.wangcong@gmail.com wrote:
>>> >On Sun, Dec 16, 2018 at 8:32 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>> >>
>>> >> On Thu 13 Dec 2018 at 23:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> >> > On Tue, Dec 11, 2018 at 2:19 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>> >> >>
>>> >> >> As a part of the effort to remove dependency on rtnl lock, cls API is being
>>> >> >> converted to use fine-grained locking mechanisms instead of global rtnl
>>> >> >> lock. However, chain_head_change callback for ingress Qdisc is a sleeping
>>> >> >> function and cannot be executed while holding a spinlock.
>>> >> >
>>> >> >
>>> >> > Why does it have to be a spinlock not a mutex?
>>> >> >
>>> >> > I've read your cover letter and this changelog, I don't find any
>>> >> > answer.
>>> >>
>>> >> My initial implementation used mutex. However, it was changed to
>>> >> spinlock by Jiri's request during internal review.
>>> >>
>>> >
>>> >So what's the answer to my question? :)
>>>
>>> Yeah, my concern agains mutexes was that it would be needed to have one
>>> per every block and per every chain. I find it quite heavy and I believe
>>> it is better to use spinlock in those cases. This patch is a side effect
>>> of that. Do you think it would be better to have mutexes instead of
>>> spinlocks?
>>
>> My only concern with spinlock is we have to go async as we
>> can't block. This is almost always error-prone especially
>> when dealing with tcf block. I had to give up with spinlock
>> for idrinfo->lock, please take a look at:
>>
>> commit 95278ddaa15cfa23e4a06ee9ed7b6ee0197c500b
>> Author: Cong Wang <xiyou.wangcong@gmail.com>
>> Date: Tue Oct 2 12:50:19 2018 -0700
>>
>> net_sched: convert idrinfo->lock from spinlock to a mutex
>>
>>
>> There are indeed some cases in kernel we do take multiple
>> mutex'es, for example,
>>
>> /*
>> * bd_mutex locking:
>> *
>> * mutex_lock(part->bd_mutex)
>> * mutex_lock_nested(whole->bd_mutex, 1)
>> */
>>
>> So, how heavy are they comparing with spinlocks?
>>
>> Thanks.
>
> Hi Cong,
>
> I did quick-and-dirty mutex-based implementation of my cls API patchset
> (removed async miniqp refactoring, changed block and chain lock types to
> mutex lock) and performed some benchmarks to determine exact mutex
> impact on cls API performance (both rule and chains API).
>
> 1. Parallel flower insertion rate (create 5 million flower filters on
> same chain/prio with 10 tc instances) - same performance:
>
> SPINLOCK
> $ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
>
> real 0m12.183s
> user 0m35.013s
> sys 1m24.947s
>
> MUTEX
> $ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
>
> real 0m12.246s
> user 0m35.417s
> sys 1m25.330s
>
> 2. Parallel flower insertion rate (create 100k flower filters, each on
> new instance of chain/tp, 10 tc instances) - mutex implementation is
> ~17% slower:
>
> SPINLOCK:
> $ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
>
> real 2m19.285s
> user 0m1.863s
> sys 5m41.157s
>
> MUTEX:
> multichain$ time ls add_sw* | xargs -n 1 -P 100 sudo tc -force -b
>
> real 2m43.495s
> user 0m1.664s
> sys 8m32.483s
>
> 3. Chains insertion rate with cls chain API (create 100k empty chains, 1
> tc instance) - ~3% difference:
>
> SPINLOCK:
> $ time sudo tc -b add_chains
>
> real 0m46.822s
> user 0m0.239s
> sys 0m46.437s
>
> MUTEX:
> $ time sudo tc -b add_chains
>
> real 0m48.600s
> user 0m0.230s
> sys 0m48.227s
>
>
> Only case where performance is significantly impacted by mutex is second
> test. This happens because chain lookup is a linear search that is
> performed while holding highly contested block lock. Perf profile for
> mutex version confirms this:
>
> + 91.83% 0.00% tc [kernel.vmlinux] [k] entry_SYSCALL_64_after_hwframe
> + 91.83% 0.00% tc [kernel.vmlinux] [k] do_syscall_64
> + 91.80% 0.00% tc libc-2.25.so [.] __libc_sendmsg
> + 91.78% 0.00% tc [kernel.vmlinux] [k] __sys_sendmsg
> + 91.77% 0.00% tc [kernel.vmlinux] [k] ___sys_sendmsg
> + 91.77% 0.00% tc [kernel.vmlinux] [k] sock_sendmsg
> + 91.77% 0.00% tc [kernel.vmlinux] [k] netlink_sendmsg
> + 91.77% 0.01% tc [kernel.vmlinux] [k] netlink_unicast
> + 91.77% 0.00% tc [kernel.vmlinux] [k] netlink_rcv_skb
> + 91.71% 0.01% tc [kernel.vmlinux] [k] rtnetlink_rcv_msg
> + 91.69% 0.03% tc [kernel.vmlinux] [k] tc_new_tfilter
> + 90.96% 26.45% tc [kernel.vmlinux] [k] __tcf_chain_get
> + 64.30% 0.01% tc [kernel.vmlinux] [k] __mutex_lock.isra.7
> + 39.36% 39.18% tc [kernel.vmlinux] [k] osq_lock
> + 24.92% 24.81% tc [kernel.vmlinux] [k] mutex_spin_on_owner
>
> Based on these results we can conclude that use-cases with significant
> amount of chains on single block will be affected by using mutex in cls
> API.
>
> Regards,
> Vlad
Hi Cong,
Any comments regarding benchmark results? Are you okay with
spinlock-based implementation?
Thanks,
Vlad
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2019-01-14 19:00 ` Vlad Buslov
@ 2019-01-14 23:00 ` Cong Wang
0 siblings, 0 replies; 30+ messages in thread
From: Cong Wang @ 2019-01-14 23:00 UTC (permalink / raw)
To: Vlad Buslov
Cc: Jiri Pirko, Linux Kernel Network Developers, Jamal Hadi Salim,
David Miller, Alexei Starovoitov, Daniel Borkmann
On Mon, Jan 14, 2019 at 11:00 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> Hi Cong,
>
> Any comments regarding benchmark results? Are you okay with
> spinlock-based implementation?
>
Hi,
Sorry for somehow forgetting to response to your last email.
I suggested you to stick to mutex not because it is better than spinlock,
it is because its non-atomic context makes the code better to understand
and easier to review.
If mutex is really slower for you, you have to make a trade-off between
performance and code complexity. And, you can also choose to use
mutex as a starter here, then try to move to spinlock later as a second
step. You don't have to break down RTNL to a small-scope lock and
convert it to a spinlock in one patchset, splitting into two patchsets
makes more sense to me and is much easier for me to review.
And, unlike the RX/TX path, these control paths are not critical,
I don't know if we care about the ~17% difference that much. Not
saying your work is not important, I am saying you are the first one
who wants to improve the performance of these control paths, so I
guess perhaps others don't care much.
BTW, as always, please provide a git branch for your patches,
I definitely don't want to apply 17 patches manually one-by-one
here. :)
Thanks!
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue
2018-12-11 10:11 ` [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue Vlad Buslov
2018-12-12 3:23 ` kbuild test robot
2018-12-13 23:32 ` Cong Wang
@ 2018-12-19 8:12 ` kbuild test robot
2 siblings, 0 replies; 30+ messages in thread
From: kbuild test robot @ 2018-12-19 8:12 UTC (permalink / raw)
To: Vlad Buslov
Cc: kbuild-all, netdev, jhs, xiyou.wangcong, jiri, davem, ast, daniel,
Vlad Buslov
[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]
Hi Vlad,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Vlad-Buslov/Refactor-classifier-API-to-work-with-chain-classifiers-without-rtnl-lock/20181211-231158
config: parisc-c3000_defconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=parisc
All errors (new ones prefixed by >>):
net/sched/sch_generic.o: In function `.LC17':
>> (.data.rel.ro+0x38): undefined reference to `tc_queue_proto_work'
net/sched/sch_generic.o: In function `.LC18':
(.data.rel.ro+0x3c): undefined reference to `tc_queue_proto_work'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14742 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH net-next v2 02/17] net: sched: protect block state with spinlock
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 03/17] net: sched: refactor tc_ctl_chain() to use block->lock Vlad Buslov
` (14 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Currently, tcf_block doesn't use any synchronization mechanisms to protect
code that manages lifetime of its chains. block->chain_list and multiple
variables in tcf_chain that control its lifetime assume external
synchronization provided by global rtnl lock. Converting chain reference
counting to atomic reference counters is not possible because cls API uses
multiple counters and flags to control chain lifetime, so all of them must
be synchronized in chain get/put code.
Use single per-block lock to protect block data and manage lifetime of all
chains on the block. Always take block->lock when accessing chain_list.
Chain get and put modify chain lifetime-management data and parent block's
chain_list, so take the lock in these functions. Verify block->lock state
with assertions in functions that expect to be called with the lock taken
and are called from multiple places. Take block->lock when accessing
filter_chain_list.
block->lock is a spinlock which means blocking functions like classifier
ops callbacks cannot be called while holding it. Rearrange chain get and
put functions code to only access protected chain data while holding block
lock and move blocking calls outside critical section:
- Check if chain was explicitly created inside put function while holding
block lock. Add additional argument to __tcf_chain_put() to only put
explicitly created chain.
- Rearrange code to only access chain reference counter and chain action
reference counter while holding block lock.
- Split tcf_chain_destroy() helper to two functions: one that requires
block->lock, and another one that needs to call sleeping functions and
can be executed after lock is released. First helper is used to detach
chain from block and make it inaccessible for concurrent users, second
actually deallocates chain memory (and parent block, if applicable).
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Use lockdep_assert_held() instead of spin_is_locked() for assertion.
- Use 'free_block' argument in tcf_chain_destroy() instead of checking
block's reference count and chain_list for second time.
include/net/sch_generic.h | 4 ++
net/sched/cls_api.c | 94 ++++++++++++++++++++++++++++++++++++++---------
2 files changed, 81 insertions(+), 17 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 80fc06468c79..61efc4fd469a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -359,6 +359,10 @@ struct tcf_chain {
};
struct tcf_block {
+ /* Lock protects tcf_block and lifetime-management data of chains
+ * attached to the block (refcnt, action_refcnt, explicitly_created).
+ */
+ spinlock_t lock;
struct list_head chain_list;
u32 index; /* block index for shared blocks */
refcount_t refcnt;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 08f245abf04a..a5aed85f0e5a 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -217,6 +217,9 @@ static void tcf_proto_destroy(struct tcf_proto *tp,
tc_queue_proto_work(&tp->work);
}
+#define ASSERT_BLOCK_LOCKED(block) \
+ lockdep_assert_held(&(block)->lock)
+
struct tcf_filter_chain_list_item {
struct list_head list;
tcf_chain_head_change_t *chain_head_change;
@@ -228,7 +231,9 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
{
struct tcf_chain *chain;
- chain = kzalloc(sizeof(*chain), GFP_KERNEL);
+ ASSERT_BLOCK_LOCKED(block);
+
+ chain = kzalloc(sizeof(*chain), GFP_ATOMIC);
if (!chain)
return NULL;
list_add_tail(&chain->list, &block->chain_list);
@@ -259,25 +264,45 @@ static void tcf_chain0_head_change(struct tcf_chain *chain,
tcf_chain_head_change_item(item, tp_head);
}
-static void tcf_chain_destroy(struct tcf_chain *chain)
+/* Returns true if block can be safely freed. */
+
+static bool tcf_chain_detach(struct tcf_chain *chain)
{
struct tcf_block *block = chain->block;
+ ASSERT_BLOCK_LOCKED(block);
+
list_del(&chain->list);
if (!chain->index)
block->chain0.chain = NULL;
+
+ if (list_empty(&block->chain_list) &&
+ refcount_read(&block->refcnt) == 0)
+ return true;
+
+ return false;
+}
+
+static void tcf_chain_destroy(struct tcf_chain *chain, bool free_block)
+{
+ struct tcf_block *block = chain->block;
+
kfree(chain);
- if (list_empty(&block->chain_list) && !refcount_read(&block->refcnt))
+ if (free_block)
kfree_rcu(block, rcu);
}
static void tcf_chain_hold(struct tcf_chain *chain)
{
+ ASSERT_BLOCK_LOCKED(chain->block);
+
++chain->refcnt;
}
static bool tcf_chain_held_by_acts_only(struct tcf_chain *chain)
{
+ ASSERT_BLOCK_LOCKED(chain->block);
+
/* In case all the references are action references, this
* chain should not be shown to the user.
*/
@@ -289,6 +314,8 @@ static struct tcf_chain *tcf_chain_lookup(struct tcf_block *block,
{
struct tcf_chain *chain;
+ ASSERT_BLOCK_LOCKED(block);
+
list_for_each_entry(chain, &block->chain_list, list) {
if (chain->index == chain_index)
return chain;
@@ -303,31 +330,40 @@ static struct tcf_chain *__tcf_chain_get(struct tcf_block *block,
u32 chain_index, bool create,
bool by_act)
{
- struct tcf_chain *chain = tcf_chain_lookup(block, chain_index);
+ struct tcf_chain *chain = NULL;
+ bool is_first_reference;
+ spin_lock(&block->lock);
+ chain = tcf_chain_lookup(block, chain_index);
if (chain) {
tcf_chain_hold(chain);
} else {
if (!create)
- return NULL;
+ goto errout;
chain = tcf_chain_create(block, chain_index);
if (!chain)
- return NULL;
+ goto errout;
}
if (by_act)
++chain->action_refcnt;
+ is_first_reference = chain->refcnt - chain->action_refcnt == 1;
+ spin_unlock(&block->lock);
/* Send notification only in case we got the first
* non-action reference. Until then, the chain acts only as
* a placeholder for actions pointing to it and user ought
* not know about them.
*/
- if (chain->refcnt - chain->action_refcnt == 1 && !by_act)
+ if (is_first_reference && !by_act)
tc_chain_notify(chain, NULL, 0, NLM_F_CREATE | NLM_F_EXCL,
RTM_NEWCHAIN, false);
return chain;
+
+errout:
+ spin_unlock(&block->lock);
+ return chain;
}
static struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
@@ -344,37 +380,59 @@ EXPORT_SYMBOL(tcf_chain_get_by_act);
static void tc_chain_tmplt_del(struct tcf_chain *chain);
-static void __tcf_chain_put(struct tcf_chain *chain, bool by_act)
+static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
+ bool explicitly_created)
{
+ struct tcf_block *block = chain->block;
+ bool is_last, free_block = false;
+ unsigned int refcnt;
+
+ spin_lock(&block->lock);
+ if (explicitly_created) {
+ if (!chain->explicitly_created) {
+ spin_unlock(&block->lock);
+ return;
+ }
+ chain->explicitly_created = false;
+ }
+
if (by_act)
chain->action_refcnt--;
- chain->refcnt--;
+
+ /* tc_chain_notify_delete can't be called while holding block lock.
+ * However, when block is unlocked chain can be changed concurrently, so
+ * save these to temporary variables.
+ */
+ refcnt = --chain->refcnt;
+ is_last = refcnt - chain->action_refcnt == 0;
+ if (refcnt == 0)
+ free_block = tcf_chain_detach(chain);
+ spin_unlock(&block->lock);
/* The last dropped non-action reference will trigger notification. */
- if (chain->refcnt - chain->action_refcnt == 0 && !by_act)
+ if (is_last && !by_act)
tc_chain_notify(chain, NULL, 0, 0, RTM_DELCHAIN, false);
- if (chain->refcnt == 0) {
+ if (refcnt == 0) {
tc_chain_tmplt_del(chain);
- tcf_chain_destroy(chain);
+ tcf_chain_destroy(chain, free_block);
}
}
static void tcf_chain_put(struct tcf_chain *chain)
{
- __tcf_chain_put(chain, false);
+ __tcf_chain_put(chain, false, false);
}
void tcf_chain_put_by_act(struct tcf_chain *chain)
{
- __tcf_chain_put(chain, true);
+ __tcf_chain_put(chain, true, false);
}
EXPORT_SYMBOL(tcf_chain_put_by_act);
static void tcf_chain_put_explicitly_created(struct tcf_chain *chain)
{
- if (chain->explicitly_created)
- tcf_chain_put(chain);
+ __tcf_chain_put(chain, false, true);
}
static void tcf_chain_flush(struct tcf_chain *chain)
@@ -787,6 +845,7 @@ static struct tcf_block *tcf_block_create(struct net *net, struct Qdisc *q,
NL_SET_ERR_MSG(extack, "Memory allocation for block failed");
return ERR_PTR(-ENOMEM);
}
+ spin_lock_init(&block->lock);
INIT_LIST_HEAD(&block->chain_list);
INIT_LIST_HEAD(&block->cb_list);
INIT_LIST_HEAD(&block->owner_list);
@@ -850,7 +909,7 @@ static void tcf_block_put_all_chains(struct tcf_block *block)
static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q,
struct tcf_block_ext_info *ei)
{
- if (refcount_dec_and_test(&block->refcnt)) {
+ if (refcount_dec_and_lock(&block->refcnt, &block->lock)) {
/* Flushing/putting all chains will cause the block to be
* deallocated when last chain is freed. However, if chain_list
* is empty, block has to be manually deallocated. After block
@@ -859,6 +918,7 @@ static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q,
*/
bool free_block = list_empty(&block->chain_list);
+ spin_unlock(&block->lock);
if (tcf_block_shared(block))
tcf_block_remove(block, block->net);
if (!free_block)
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 03/17] net: sched: refactor tc_ctl_chain() to use block->lock
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 01/17] net: sched: refactor mini_qdisc_pair_swap() to use workqueue Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 02/17] net: sched: protect block state with spinlock Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 04/17] net: sched: protect block->chain0 with block->lock Vlad Buslov
` (13 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
In order to remove dependency on rtnl lock, modify chain API to use
block->lock to protect chain from concurrent modification. Rearrange
tc_ctl_chain() code to call tcf_chain_hold() while holding block->lock.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_api.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index a5aed85f0e5a..0e1e7f26ee68 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2293,6 +2293,8 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
err = -EINVAL;
goto errout_block;
}
+
+ spin_lock(&block->lock);
chain = tcf_chain_lookup(block, chain_index);
if (n->nlmsg_type == RTM_NEWCHAIN) {
if (chain) {
@@ -2304,41 +2306,49 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
} else {
NL_SET_ERR_MSG(extack, "Filter chain already exists");
err = -EEXIST;
- goto errout_block;
+ goto errout_block_locked;
}
} else {
if (!(n->nlmsg_flags & NLM_F_CREATE)) {
NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain");
err = -ENOENT;
- goto errout_block;
+ goto errout_block_locked;
}
chain = tcf_chain_create(block, chain_index);
if (!chain) {
NL_SET_ERR_MSG(extack, "Failed to create filter chain");
err = -ENOMEM;
- goto errout_block;
+ goto errout_block_locked;
}
}
} else {
if (!chain || tcf_chain_held_by_acts_only(chain)) {
NL_SET_ERR_MSG(extack, "Cannot find specified filter chain");
err = -EINVAL;
- goto errout_block;
+ goto errout_block_locked;
}
tcf_chain_hold(chain);
}
+ if (n->nlmsg_type == RTM_NEWCHAIN) {
+ /* Modifying chain requires holding parent block lock. In case
+ * the chain was successfully added, take a reference to the
+ * chain. This ensures that an empty chain does not disappear at
+ * the end of this function.
+ */
+ tcf_chain_hold(chain);
+ chain->explicitly_created = true;
+ }
+ spin_unlock(&block->lock);
+
switch (n->nlmsg_type) {
case RTM_NEWCHAIN:
err = tc_chain_tmplt_add(chain, net, tca, extack);
- if (err)
+ if (err) {
+ tcf_chain_put_explicitly_created(chain);
goto errout;
- /* In case the chain was successfully added, take a reference
- * to the chain. This ensures that an empty chain
- * does not disappear at the end of this function.
- */
- tcf_chain_hold(chain);
- chain->explicitly_created = true;
+ }
+
tc_chain_notify(chain, NULL, 0, NLM_F_CREATE | NLM_F_EXCL,
RTM_NEWCHAIN, false);
break;
@@ -2373,6 +2383,10 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
/* Replay the request. */
goto replay;
return err;
+
+errout_block_locked:
+ spin_unlock(&block->lock);
+ goto errout_block;
}
/* called with RTNL */
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 04/17] net: sched: protect block->chain0 with block->lock
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (2 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 03/17] net: sched: refactor tc_ctl_chain() to use block->lock Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 05/17] net: sched: traverse chains in block with tcf_get_next_chain() Vlad Buslov
` (12 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
In order to remove dependency on rtnl lock, use block->lock to protect
chain0 struct from concurrent modification. Rearrange code in chain0
callback add and del functions to only access chain0 when block->lock is
held.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_api.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0e1e7f26ee68..c93570384967 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -260,8 +260,11 @@ static void tcf_chain0_head_change(struct tcf_chain *chain,
if (chain->index)
return;
+
+ spin_lock(&block->lock);
list_for_each_entry(item, &block->chain0.filter_chain_list, list)
tcf_chain_head_change_item(item, tp_head);
+ spin_unlock(&block->lock);
}
/* Returns true if block can be safely freed. */
@@ -765,8 +768,8 @@ tcf_chain0_head_change_cb_add(struct tcf_block *block,
struct tcf_block_ext_info *ei,
struct netlink_ext_ack *extack)
{
- struct tcf_chain *chain0 = block->chain0.chain;
struct tcf_filter_chain_list_item *item;
+ struct tcf_chain *chain0;
item = kmalloc(sizeof(*item), GFP_KERNEL);
if (!item) {
@@ -775,9 +778,14 @@ tcf_chain0_head_change_cb_add(struct tcf_block *block,
}
item->chain_head_change = ei->chain_head_change;
item->chain_head_change_priv = ei->chain_head_change_priv;
+
+ spin_lock(&block->lock);
+ chain0 = block->chain0.chain;
if (chain0 && chain0->filter_chain)
tcf_chain_head_change_item(item, chain0->filter_chain);
list_add(&item->list, &block->chain0.filter_chain_list);
+ spin_unlock(&block->lock);
+
return 0;
}
@@ -785,20 +793,23 @@ static void
tcf_chain0_head_change_cb_del(struct tcf_block *block,
struct tcf_block_ext_info *ei)
{
- struct tcf_chain *chain0 = block->chain0.chain;
struct tcf_filter_chain_list_item *item;
+ spin_lock(&block->lock);
list_for_each_entry(item, &block->chain0.filter_chain_list, list) {
if ((!ei->chain_head_change && !ei->chain_head_change_priv) ||
(item->chain_head_change == ei->chain_head_change &&
item->chain_head_change_priv == ei->chain_head_change_priv)) {
- if (chain0)
+ if (block->chain0.chain)
tcf_chain_head_change_item(item, NULL);
list_del(&item->list);
+ spin_unlock(&block->lock);
+
kfree(item);
return;
}
}
+ spin_unlock(&block->lock);
WARN_ON(1);
}
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 05/17] net: sched: traverse chains in block with tcf_get_next_chain()
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (3 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 04/17] net: sched: protect block->chain0 with block->lock Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 06/17] net: sched: protect chain template accesses with block lock Vlad Buslov
` (11 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
All users of block->chain_list rely on rtnl lock and assume that no new
chains are added when traversing the list. Use tcf_get_next_chain() to
traverse chain list without relying on rtnl mutex. This function iterates
over chains by taking reference to current iterator chain only and doesn't
assume external synchronization of chain list.
Don't take reference to all chains in block when flushing and use
tcf_get_next_chain() to safely iterate over chain list instead. Remove
tcf_block_put_all_chains() that is no longer used.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/pkt_cls.h | 2 ++
net/sched/cls_api.c | 96 +++++++++++++++++++++++++++++++++++++--------------
net/sched/sch_api.c | 4 ++-
3 files changed, 76 insertions(+), 26 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index ea191d8cfcc9..cde724c46ab5 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -43,6 +43,8 @@ bool tcf_queue_work(struct rcu_work *rwork, work_func_t func);
struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block,
u32 chain_index);
void tcf_chain_put_by_act(struct tcf_chain *chain);
+struct tcf_chain *tcf_get_next_chain(struct tcf_block *block,
+ struct tcf_chain *chain);
void tcf_block_netif_keep_dst(struct tcf_block *block);
int tcf_block_get(struct tcf_block **p_block,
struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index c93570384967..4900ac659b58 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -892,28 +892,62 @@ static struct tcf_block *tcf_block_refcnt_get(struct net *net, u32 block_index)
return block;
}
-static void tcf_block_flush_all_chains(struct tcf_block *block)
+static struct tcf_chain *
+__tcf_get_next_chain(struct tcf_block *block, struct tcf_chain *chain)
{
- struct tcf_chain *chain;
+ spin_lock(&block->lock);
+ if (chain)
+ chain = list_is_last(&chain->list, &block->chain_list) ?
+ NULL : list_next_entry(chain, list);
+ else
+ chain = list_first_entry_or_null(&block->chain_list,
+ struct tcf_chain, list);
- /* Hold a refcnt for all chains, so that they don't disappear
- * while we are iterating.
- */
- list_for_each_entry(chain, &block->chain_list, list)
+ /* skip all action-only chains */
+ while (chain && tcf_chain_held_by_acts_only(chain))
+ chain = list_is_last(&chain->list, &block->chain_list) ?
+ NULL : list_next_entry(chain, list);
+
+ if (chain)
tcf_chain_hold(chain);
+ spin_unlock(&block->lock);
- list_for_each_entry(chain, &block->chain_list, list)
- tcf_chain_flush(chain);
+ return chain;
}
-static void tcf_block_put_all_chains(struct tcf_block *block)
+/* Function to be used by all clients that want to iterate over all chains on
+ * block. It properly obtains block->lock and takes reference to chain before
+ * returning it. Users of this function must be tolerant to concurrent chain
+ * insertion/deletion or ensure that no concurrent chain modification is
+ * possible. Note that all netlink dump callbacks cannot guarantee to provide
+ * consistent dump because rtnl lock is released each time skb is filled with
+ * data and sent to user-space.
+ */
+
+struct tcf_chain *
+tcf_get_next_chain(struct tcf_block *block, struct tcf_chain *chain)
{
- struct tcf_chain *chain, *tmp;
+ struct tcf_chain *chain_next = __tcf_get_next_chain(block, chain);
- /* At this point, all the chains should have refcnt >= 1. */
- list_for_each_entry_safe(chain, tmp, &block->chain_list, list) {
- tcf_chain_put_explicitly_created(chain);
+ if (chain)
tcf_chain_put(chain);
+
+ return chain_next;
+}
+EXPORT_SYMBOL(tcf_get_next_chain);
+
+static void tcf_block_flush_all_chains(struct tcf_block *block)
+{
+ struct tcf_chain *chain;
+
+ /* Last reference to block. At this point chains cannot be added or
+ * removed concurrently.
+ */
+ for (chain = tcf_get_next_chain(block, NULL);
+ chain;
+ chain = tcf_get_next_chain(block, chain)) {
+ tcf_chain_put_explicitly_created(chain);
+ tcf_chain_flush(chain);
}
}
@@ -932,8 +966,6 @@ static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q,
spin_unlock(&block->lock);
if (tcf_block_shared(block))
tcf_block_remove(block, block->net);
- if (!free_block)
- tcf_block_flush_all_chains(block);
if (q)
tcf_block_offload_unbind(block, q, ei);
@@ -941,7 +973,7 @@ static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q,
if (free_block)
kfree_rcu(block, rcu);
else
- tcf_block_put_all_chains(block);
+ tcf_block_flush_all_chains(block);
} else if (q) {
tcf_block_offload_unbind(block, q, ei);
}
@@ -1275,11 +1307,15 @@ tcf_block_playback_offloads(struct tcf_block *block, tc_setup_cb_t *cb,
void *cb_priv, bool add, bool offload_in_use,
struct netlink_ext_ack *extack)
{
- struct tcf_chain *chain;
+ struct tcf_chain *chain, *chain_prev;
struct tcf_proto *tp;
int err;
- list_for_each_entry(chain, &block->chain_list, list) {
+ for (chain = __tcf_get_next_chain(block, NULL);
+ chain;
+ chain_prev = chain,
+ chain = __tcf_get_next_chain(block, chain),
+ tcf_chain_put(chain_prev)) {
for (tp = rtnl_dereference(chain->filter_chain); tp;
tp = rtnl_dereference(tp->next)) {
if (tp->ops->reoffload) {
@@ -1298,6 +1334,7 @@ tcf_block_playback_offloads(struct tcf_block *block, tc_setup_cb_t *cb,
return 0;
err_playback_remove:
+ tcf_chain_put(chain);
tcf_block_playback_offloads(block, cb, cb_priv, false, offload_in_use,
extack);
return err;
@@ -2061,11 +2098,11 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
/* called with RTNL */
static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
{
+ struct tcf_chain *chain, *chain_prev;
struct net *net = sock_net(skb->sk);
struct nlattr *tca[TCA_MAX + 1];
struct Qdisc *q = NULL;
struct tcf_block *block;
- struct tcf_chain *chain;
struct tcmsg *tcm = nlmsg_data(cb->nlh);
long index_start;
long index;
@@ -2129,12 +2166,17 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
index_start = cb->args[0];
index = 0;
- list_for_each_entry(chain, &block->chain_list, list) {
+ for (chain = __tcf_get_next_chain(block, NULL);
+ chain;
+ chain_prev = chain,
+ chain = __tcf_get_next_chain(block, chain),
+ tcf_chain_put(chain_prev)) {
if (tca[TCA_CHAIN] &&
nla_get_u32(tca[TCA_CHAIN]) != chain->index)
continue;
if (!tcf_chain_dump(chain, q, parent, skb, cb,
index_start, &index)) {
+ tcf_chain_put(chain);
err = -EMSGSIZE;
break;
}
@@ -2403,11 +2445,11 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
/* called with RTNL */
static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
{
+ struct tcf_chain *chain, *chain_prev;
struct net *net = sock_net(skb->sk);
struct nlattr *tca[TCA_MAX + 1];
struct Qdisc *q = NULL;
struct tcf_block *block;
- struct tcf_chain *chain;
struct tcmsg *tcm = nlmsg_data(cb->nlh);
long index_start;
long index;
@@ -2471,7 +2513,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
index_start = cb->args[0];
index = 0;
- list_for_each_entry(chain, &block->chain_list, list) {
+ for (chain = __tcf_get_next_chain(block, NULL);
+ chain;
+ chain_prev = chain,
+ chain = __tcf_get_next_chain(block, chain),
+ tcf_chain_put(chain_prev)) {
if ((tca[TCA_CHAIN] &&
nla_get_u32(tca[TCA_CHAIN]) != chain->index))
continue;
@@ -2479,14 +2525,14 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
index++;
continue;
}
- if (tcf_chain_held_by_acts_only(chain))
- continue;
err = tc_chain_fill_node(chain, net, skb, block,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
RTM_NEWCHAIN);
- if (err <= 0)
+ if (err <= 0) {
+ tcf_chain_put(chain);
break;
+ }
index++;
}
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 9c88cec7e8a2..f80fd2d1b7e6 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1912,7 +1912,9 @@ static void tc_bind_tclass(struct Qdisc *q, u32 portid, u32 clid,
block = cops->tcf_block(q, cl, NULL);
if (!block)
return;
- list_for_each_entry(chain, &block->chain_list, list) {
+ for (chain = tcf_get_next_chain(block, NULL);
+ chain;
+ chain = tcf_get_next_chain(block, chain)) {
struct tcf_proto *tp;
for (tp = rtnl_dereference(chain->filter_chain);
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 06/17] net: sched: protect chain template accesses with block lock
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (4 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 05/17] net: sched: traverse chains in block with tcf_get_next_chain() Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 07/17] net: sched: lock the chain when accessing filter_chain list Vlad Buslov
` (10 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
When cls API is called without protection of rtnl lock, parallel
modification of chain is possible, which means that chain template can be
changed concurrently in certain circumstances. For example, when chain is
'deleted' by new user-space chain API, the chain might continue to be used
if it is referenced by actions, and can be 're-created' again by user. In
such case same chain structure is reused and its template is changed. To
protect from described scenario, cache chain template while holding block
lock. Introduce standalone tc_chain_notify_delete() function that works
with cached template values, instead of chains themselves.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
net/sched/cls_api.c | 73 +++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 57 insertions(+), 16 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 4900ac659b58..f0c0fcd8339e 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -381,14 +381,22 @@ struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block, u32 chain_index)
}
EXPORT_SYMBOL(tcf_chain_get_by_act);
-static void tc_chain_tmplt_del(struct tcf_chain *chain);
+static void tc_chain_tmplt_del(const struct tcf_proto_ops *tmplt_ops,
+ void *tmplt_priv);
+static int tc_chain_notify_delete(const struct tcf_proto_ops *tmplt_ops,
+ void *tmplt_priv, u32 chain_index,
+ struct tcf_block *block, struct sk_buff *oskb,
+ u32 seq, u16 flags, bool unicast);
static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
bool explicitly_created)
{
struct tcf_block *block = chain->block;
+ const struct tcf_proto_ops *tmplt_ops;
bool is_last, free_block = false;
unsigned int refcnt;
+ void *tmplt_priv;
+ u32 chain_index;
spin_lock(&block->lock);
if (explicitly_created) {
@@ -408,16 +416,21 @@ static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
*/
refcnt = --chain->refcnt;
is_last = refcnt - chain->action_refcnt == 0;
+ tmplt_ops = chain->tmplt_ops;
+ tmplt_priv = chain->tmplt_priv;
+ chain_index = chain->index;
+
if (refcnt == 0)
free_block = tcf_chain_detach(chain);
spin_unlock(&block->lock);
/* The last dropped non-action reference will trigger notification. */
if (is_last && !by_act)
- tc_chain_notify(chain, NULL, 0, 0, RTM_DELCHAIN, false);
+ tc_chain_notify_delete(tmplt_ops, tmplt_priv, chain_index,
+ block, NULL, 0, 0, false);
if (refcnt == 0) {
- tc_chain_tmplt_del(chain);
+ tc_chain_tmplt_del(tmplt_ops, tmplt_priv);
tcf_chain_destroy(chain, free_block);
}
}
@@ -2193,8 +2206,10 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
return skb->len;
}
-static int tc_chain_fill_node(struct tcf_chain *chain, struct net *net,
- struct sk_buff *skb, struct tcf_block *block,
+static int tc_chain_fill_node(const struct tcf_proto_ops *tmplt_ops,
+ void *tmplt_priv, u32 chain_index,
+ struct net *net, struct sk_buff *skb,
+ struct tcf_block *block,
u32 portid, u32 seq, u16 flags, int event)
{
unsigned char *b = skb_tail_pointer(skb);
@@ -2203,8 +2218,8 @@ static int tc_chain_fill_node(struct tcf_chain *chain, struct net *net,
struct tcmsg *tcm;
void *priv;
- ops = chain->tmplt_ops;
- priv = chain->tmplt_priv;
+ ops = tmplt_ops;
+ priv = tmplt_priv;
nlh = nlmsg_put(skb, portid, seq, event, sizeof(*tcm), flags);
if (!nlh)
@@ -2222,7 +2237,7 @@ static int tc_chain_fill_node(struct tcf_chain *chain, struct net *net,
tcm->tcm_block_index = block->index;
}
- if (nla_put_u32(skb, TCA_CHAIN, chain->index))
+ if (nla_put_u32(skb, TCA_CHAIN, chain_index))
goto nla_put_failure;
if (ops) {
@@ -2253,7 +2268,8 @@ static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
if (!skb)
return -ENOBUFS;
- if (tc_chain_fill_node(chain, net, skb, block, portid,
+ if (tc_chain_fill_node(chain->tmplt_ops, chain->tmplt_priv,
+ chain->index, net, skb, block, portid,
seq, flags, event) <= 0) {
kfree_skb(skb);
return -EINVAL;
@@ -2265,6 +2281,31 @@ static int tc_chain_notify(struct tcf_chain *chain, struct sk_buff *oskb,
return rtnetlink_send(skb, net, portid, RTNLGRP_TC, flags & NLM_F_ECHO);
}
+static int tc_chain_notify_delete(const struct tcf_proto_ops *tmplt_ops,
+ void *tmplt_priv, u32 chain_index,
+ struct tcf_block *block, struct sk_buff *oskb,
+ u32 seq, u16 flags, bool unicast)
+{
+ u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
+ struct net *net = block->net;
+ struct sk_buff *skb;
+
+ skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+ if (!skb)
+ return -ENOBUFS;
+
+ if (tc_chain_fill_node(tmplt_ops, tmplt_priv, chain_index, net, skb,
+ block, portid, seq, flags, RTM_DELCHAIN) <= 0) {
+ kfree_skb(skb);
+ return -EINVAL;
+ }
+
+ if (unicast)
+ return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
+
+ return rtnetlink_send(skb, net, portid, RTNLGRP_TC, flags & NLM_F_ECHO);
+}
+
static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net,
struct nlattr **tca,
struct netlink_ext_ack *extack)
@@ -2294,16 +2335,15 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net,
return 0;
}
-static void tc_chain_tmplt_del(struct tcf_chain *chain)
+static void tc_chain_tmplt_del(const struct tcf_proto_ops *tmplt_ops,
+ void *tmplt_priv)
{
- const struct tcf_proto_ops *ops = chain->tmplt_ops;
-
/* If template ops are set, no work to do for us. */
- if (!ops)
+ if (!tmplt_ops)
return;
- ops->tmplt_destroy(chain->tmplt_priv);
- module_put(ops->owner);
+ tmplt_ops->tmplt_destroy(tmplt_priv);
+ module_put(tmplt_ops->owner);
}
/* Add/delete/get a chain */
@@ -2525,7 +2565,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
index++;
continue;
}
- err = tc_chain_fill_node(chain, net, skb, block,
+ err = tc_chain_fill_node(chain->tmplt_ops, chain->tmplt_priv,
+ chain->index, net, skb, block,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
RTM_NEWCHAIN);
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 07/17] net: sched: lock the chain when accessing filter_chain list
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (5 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 06/17] net: sched: protect chain template accesses with block lock Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 08/17] net: sched: introduce reference counting for tcf proto Vlad Buslov
` (9 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Always lock chain when accessing filter_chain list, instead of relying on
rtnl lock. Dereference filter_chain with tcf_chain_dereference() lockdep
macro to verify that all users of chain_list have the lock taken.
Rearrange tp insert/remove code in tc_new_tfilter/tc_del_tfilter to execute
all necessary code while holding chain lock in order to prevent
invalidation of chain_info structure by potential concurrent change.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Refactor tcf_chain0_head_change_cb_add() to not take block->lock and
chain0->filter_chain_lock in correct order.
include/net/sch_generic.h | 17 +++++++
net/sched/cls_api.c | 116 +++++++++++++++++++++++++++++++---------------
2 files changed, 95 insertions(+), 38 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 61efc4fd469a..03c0f8325091 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -347,6 +347,8 @@ struct qdisc_skb_cb {
typedef void tcf_chain_head_change_t(struct tcf_proto *tp_head, void *priv);
struct tcf_chain {
+ /* Protects filter_chain and filter_chain_list. */
+ spinlock_t filter_chain_lock;
struct tcf_proto __rcu *filter_chain;
struct list_head list;
struct tcf_block *block;
@@ -380,6 +382,21 @@ struct tcf_block {
struct rcu_head rcu;
};
+#ifdef CONFIG_PROVE_LOCKING
+static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
+{
+ return lockdep_is_held(&chain->filter_chain_lock);
+}
+#else
+static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
+{
+ return true;
+}
+#endif /* #ifdef CONFIG_PROVE_LOCKING */
+
+#define tcf_chain_dereference(p, chain) \
+ rcu_dereference_protected(p, lockdep_tcf_chain_is_locked(chain))
+
static inline void tcf_block_offload_inc(struct tcf_block *block, u32 *flags)
{
if (*flags & TCA_CLS_FLAGS_IN_HW)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index f0c0fcd8339e..9a77ef0c18fa 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -237,6 +237,7 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
if (!chain)
return NULL;
list_add_tail(&chain->list, &block->chain_list);
+ spin_lock_init(&chain->filter_chain_lock);
chain->block = block;
chain->index = chain_index;
chain->refcnt = 1;
@@ -453,9 +454,13 @@ static void tcf_chain_put_explicitly_created(struct tcf_chain *chain)
static void tcf_chain_flush(struct tcf_chain *chain)
{
- struct tcf_proto *tp = rtnl_dereference(chain->filter_chain);
+ struct tcf_proto *tp;
+ spin_lock(&chain->filter_chain_lock);
+ tp = tcf_chain_dereference(chain->filter_chain, chain);
tcf_chain0_head_change(chain, NULL);
+ spin_unlock(&chain->filter_chain_lock);
+
while (tp) {
RCU_INIT_POINTER(chain->filter_chain, tp->next);
tcf_proto_destroy(tp, NULL);
@@ -794,11 +799,29 @@ tcf_chain0_head_change_cb_add(struct tcf_block *block,
spin_lock(&block->lock);
chain0 = block->chain0.chain;
- if (chain0 && chain0->filter_chain)
- tcf_chain_head_change_item(item, chain0->filter_chain);
- list_add(&item->list, &block->chain0.filter_chain_list);
+ if (chain0)
+ tcf_chain_hold(chain0);
+ else
+ list_add(&item->list, &block->chain0.filter_chain_list);
spin_unlock(&block->lock);
+ if (chain0) {
+ struct tcf_proto *tp_head;
+
+ spin_lock(&chain0->filter_chain_lock);
+
+ tp_head = tcf_chain_dereference(chain0->filter_chain, chain0);
+ if (tp_head)
+ tcf_chain_head_change_item(item, tp_head);
+
+ spin_lock(&block->lock);
+ list_add(&item->list, &block->chain0.filter_chain_list);
+ spin_unlock(&block->lock);
+
+ spin_unlock(&chain0->filter_chain_lock);
+ tcf_chain_put(chain0);
+ }
+
return 0;
}
@@ -1497,9 +1520,10 @@ struct tcf_chain_info {
struct tcf_proto __rcu *next;
};
-static struct tcf_proto *tcf_chain_tp_prev(struct tcf_chain_info *chain_info)
+static struct tcf_proto *tcf_chain_tp_prev(struct tcf_chain *chain,
+ struct tcf_chain_info *chain_info)
{
- return rtnl_dereference(*chain_info->pprev);
+ return tcf_chain_dereference(*chain_info->pprev, chain);
}
static void tcf_chain_tp_insert(struct tcf_chain *chain,
@@ -1508,7 +1532,7 @@ static void tcf_chain_tp_insert(struct tcf_chain *chain,
{
if (*chain_info->pprev == chain->filter_chain)
tcf_chain0_head_change(chain, tp);
- RCU_INIT_POINTER(tp->next, tcf_chain_tp_prev(chain_info));
+ RCU_INIT_POINTER(tp->next, tcf_chain_tp_prev(chain, chain_info));
rcu_assign_pointer(*chain_info->pprev, tp);
tcf_chain_hold(chain);
}
@@ -1517,7 +1541,7 @@ static void tcf_chain_tp_remove(struct tcf_chain *chain,
struct tcf_chain_info *chain_info,
struct tcf_proto *tp)
{
- struct tcf_proto *next = rtnl_dereference(chain_info->next);
+ struct tcf_proto *next = tcf_chain_dereference(chain_info->next, chain);
if (tp == chain->filter_chain)
tcf_chain0_head_change(chain, next);
@@ -1534,7 +1558,8 @@ static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
/* Check the chain for existence of proto-tcf with this priority */
for (pprev = &chain->filter_chain;
- (tp = rtnl_dereference(*pprev)); pprev = &tp->next) {
+ (tp = tcf_chain_dereference(*pprev, chain));
+ pprev = &tp->next) {
if (tp->prio >= prio) {
if (tp->prio == prio) {
if (prio_allocate ||
@@ -1742,12 +1767,13 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
goto errout;
}
+ spin_lock(&chain->filter_chain_lock);
tp = tcf_chain_tp_find(chain, &chain_info, protocol,
prio, prio_allocate);
if (IS_ERR(tp)) {
NL_SET_ERR_MSG(extack, "Filter with specified priority/protocol not found");
err = PTR_ERR(tp);
- goto errout;
+ goto errout_locked;
}
if (tp == NULL) {
@@ -1756,29 +1782,37 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (tca[TCA_KIND] == NULL || !protocol) {
NL_SET_ERR_MSG(extack, "Filter kind and protocol must be specified");
err = -EINVAL;
- goto errout;
+ goto errout_locked;
}
if (!(n->nlmsg_flags & NLM_F_CREATE)) {
NL_SET_ERR_MSG(extack, "Need both RTM_NEWTFILTER and NLM_F_CREATE to create a new filter");
err = -ENOENT;
- goto errout;
+ goto errout_locked;
}
if (prio_allocate)
- prio = tcf_auto_prio(tcf_chain_tp_prev(&chain_info));
+ prio = tcf_auto_prio(tcf_chain_tp_prev(chain,
+ &chain_info));
+ spin_unlock(&chain->filter_chain_lock);
tp = tcf_proto_create(nla_data(tca[TCA_KIND]),
protocol, prio, chain, extack);
if (IS_ERR(tp)) {
err = PTR_ERR(tp);
goto errout;
}
+
+ spin_lock(&chain->filter_chain_lock);
+ tcf_chain_tp_insert(chain, &chain_info, tp);
+ spin_unlock(&chain->filter_chain_lock);
tp_created = 1;
} else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind)) {
NL_SET_ERR_MSG(extack, "Specified filter kind does not match existing one");
err = -EINVAL;
- goto errout;
+ goto errout_locked;
+ } else {
+ spin_unlock(&chain->filter_chain_lock);
}
fh = tp->ops->get(tp, t->tcm_handle);
@@ -1804,21 +1838,11 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
n->nlmsg_flags & NLM_F_CREATE ? TCA_ACT_NOREPLACE : TCA_ACT_REPLACE,
extack);
- if (err == 0) {
- if (tp_created)
- tcf_chain_tp_insert(chain, &chain_info, tp);
+ if (err == 0)
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_NEWTFILTER, false);
- } else {
- if (tp_created) {
- /* tp wasn't inserted to chain tp list. Take reference
- * to chain manually for tcf_proto_destroy() to
- * release.
- */
- tcf_chain_hold(chain);
- tcf_proto_destroy(tp, NULL);
- }
- }
+ else if (tp_created)
+ tcf_proto_destroy(tp, NULL);
errout:
if (chain)
@@ -1828,6 +1852,10 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
/* Replay the request. */
goto replay;
return err;
+
+errout_locked:
+ spin_unlock(&chain->filter_chain_lock);
+ goto errout;
}
static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
@@ -1903,31 +1931,34 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
goto errout;
}
+ spin_lock(&chain->filter_chain_lock);
tp = tcf_chain_tp_find(chain, &chain_info, protocol,
prio, false);
if (!tp || IS_ERR(tp)) {
NL_SET_ERR_MSG(extack, "Filter with specified priority/protocol not found");
err = tp ? PTR_ERR(tp) : -ENOENT;
- goto errout;
+ goto errout_locked;
} else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind)) {
NL_SET_ERR_MSG(extack, "Specified filter kind does not match existing one");
err = -EINVAL;
+ goto errout_locked;
+ } else if (t->tcm_handle == 0) {
+ tcf_chain_tp_remove(chain, &chain_info, tp);
+ spin_unlock(&chain->filter_chain_lock);
+
+ tfilter_notify(net, skb, n, tp, block, q, parent, fh,
+ RTM_DELTFILTER, false);
+ tcf_proto_destroy(tp, extack);
+ err = 0;
goto errout;
}
+ spin_unlock(&chain->filter_chain_lock);
fh = tp->ops->get(tp, t->tcm_handle);
if (!fh) {
- if (t->tcm_handle == 0) {
- tcf_chain_tp_remove(chain, &chain_info, tp);
- tfilter_notify(net, skb, n, tp, block, q, parent, fh,
- RTM_DELTFILTER, false);
- tcf_proto_destroy(tp, extack);
- err = 0;
- } else {
- NL_SET_ERR_MSG(extack, "Specified filter handle not found");
- err = -ENOENT;
- }
+ NL_SET_ERR_MSG(extack, "Specified filter handle not found");
+ err = -ENOENT;
} else {
bool last;
@@ -1937,7 +1968,10 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (err)
goto errout;
if (last) {
+ spin_lock(&chain->filter_chain_lock);
tcf_chain_tp_remove(chain, &chain_info, tp);
+ spin_unlock(&chain->filter_chain_lock);
+
tcf_proto_destroy(tp, extack);
}
}
@@ -1947,6 +1981,10 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tcf_chain_put(chain);
tcf_block_release(q, block);
return err;
+
+errout_locked:
+ spin_unlock(&chain->filter_chain_lock);
+ goto errout;
}
static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
@@ -2004,8 +2042,10 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
goto errout;
}
+ spin_lock(&chain->filter_chain_lock);
tp = tcf_chain_tp_find(chain, &chain_info, protocol,
prio, false);
+ spin_unlock(&chain->filter_chain_lock);
if (!tp || IS_ERR(tp)) {
NL_SET_ERR_MSG(extack, "Filter with specified priority/protocol not found");
err = tp ? PTR_ERR(tp) : -ENOENT;
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 08/17] net: sched: introduce reference counting for tcf proto
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (6 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 07/17] net: sched: lock the chain when accessing filter_chain list Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 09/17] net: sched: traverse classifiers in chain with tcf_get_next_proto() Vlad Buslov
` (8 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Add reference counter to tcf proto. Use it to manage tcf proto life cycle
in cls API.
Implement helper get/put functions for tcf proto and use them to modify cls
API to always take reference to tcf proto while using it. This change
allows to concurrently modify proto, instead of relying on rtnl lock for
protection.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/sch_generic.h | 1 +
net/sched/cls_api.c | 44 +++++++++++++++++++++++++++++++++++++-------
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 03c0f8325091..a6dade2c22c8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -327,6 +327,7 @@ struct tcf_proto {
void *data;
const struct tcf_proto_ops *ops;
struct tcf_chain *chain;
+ refcount_t refcnt;
struct rcu_head rcu;
struct work_struct work;
};
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 9a77ef0c18fa..9a09926538ef 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -198,6 +198,7 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
tp->prio = prio;
tp->chain = chain;
INIT_WORK(&tp->work, tcf_proto_destroy_work);
+ refcount_set(&tp->refcnt, 1);
err = tp->ops->init(tp);
if (err) {
@@ -211,12 +212,24 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
return ERR_PTR(err);
}
+static void tcf_proto_get(struct tcf_proto *tp)
+{
+ refcount_inc(&tp->refcnt);
+}
+
static void tcf_proto_destroy(struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
tc_queue_proto_work(&tp->work);
}
+static void tcf_proto_put(struct tcf_proto *tp,
+ struct netlink_ext_ack *extack)
+{
+ if (refcount_dec_and_test(&tp->refcnt))
+ tcf_proto_destroy(tp, extack);
+}
+
#define ASSERT_BLOCK_LOCKED(block) \
lockdep_assert_held(&(block)->lock)
@@ -458,13 +471,13 @@ static void tcf_chain_flush(struct tcf_chain *chain)
spin_lock(&chain->filter_chain_lock);
tp = tcf_chain_dereference(chain->filter_chain, chain);
+ RCU_INIT_POINTER(chain->filter_chain, NULL);
tcf_chain0_head_change(chain, NULL);
spin_unlock(&chain->filter_chain_lock);
while (tp) {
- RCU_INIT_POINTER(chain->filter_chain, tp->next);
- tcf_proto_destroy(tp, NULL);
- tp = rtnl_dereference(chain->filter_chain);
+ tcf_proto_put(tp, NULL);
+ tp = tp->next;
}
}
@@ -1532,9 +1545,9 @@ static void tcf_chain_tp_insert(struct tcf_chain *chain,
{
if (*chain_info->pprev == chain->filter_chain)
tcf_chain0_head_change(chain, tp);
+ tcf_proto_get(tp);
RCU_INIT_POINTER(tp->next, tcf_chain_tp_prev(chain, chain_info));
rcu_assign_pointer(*chain_info->pprev, tp);
- tcf_chain_hold(chain);
}
static void tcf_chain_tp_remove(struct tcf_chain *chain,
@@ -1572,7 +1585,12 @@ static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
}
}
chain_info->pprev = pprev;
- chain_info->next = tp ? tp->next : NULL;
+ if (tp) {
+ chain_info->next = tp->next;
+ tcf_proto_get(tp);
+ } else {
+ chain_info->next = NULL;
+ }
return tp;
}
@@ -1847,6 +1865,12 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
errout:
if (chain)
tcf_chain_put(chain);
+ if (chain) {
+ if (tp && !IS_ERR(tp))
+ tcf_proto_put(tp, NULL);
+ if (!tp_created)
+ tcf_chain_put(chain);
+ }
tcf_block_release(q, block);
if (err == -EAGAIN)
/* Replay the request. */
@@ -1977,8 +2001,11 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
}
errout:
- if (chain)
+ if (chain) {
+ if (tp && !IS_ERR(tp))
+ tcf_proto_put(tp, NULL);
tcf_chain_put(chain);
+ }
tcf_block_release(q, block);
return err;
@@ -2069,8 +2096,11 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
}
errout:
- if (chain)
+ if (chain) {
+ if (tp && !IS_ERR(tp))
+ tcf_proto_put(tp, NULL);
tcf_chain_put(chain);
+ }
tcf_block_release(q, block);
return err;
}
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 09/17] net: sched: traverse classifiers in chain with tcf_get_next_proto()
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (7 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 08/17] net: sched: introduce reference counting for tcf proto Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-14 10:15 ` kbuild test robot
2018-12-11 10:11 ` [PATCH net-next v2 10/17] net: sched: refactor tp insert/delete for concurrent execution Vlad Buslov
` (7 subsequent siblings)
16 siblings, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
All users of chain->filters_chain rely on rtnl lock and assume that no new
classifier instances are added when traversing the list. Use
tcf_get_next_proto() to traverse filters list without relying on rtnl
mutex. This function iterates over classifiers by taking reference to
current iterator classifier only and doesn't assume external
synchronization of filters list.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/pkt_cls.h | 2 ++
net/sched/cls_api.c | 70 +++++++++++++++++++++++++++++++++++++++++++--------
net/sched/sch_api.c | 4 +--
3 files changed, 64 insertions(+), 12 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index cde724c46ab5..411ac4334942 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -45,6 +45,8 @@ struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block,
void tcf_chain_put_by_act(struct tcf_chain *chain);
struct tcf_chain *tcf_get_next_chain(struct tcf_block *block,
struct tcf_chain *chain);
+struct tcf_proto *tcf_get_next_proto(struct tcf_chain *chain,
+ struct tcf_proto *tp);
void tcf_block_netif_keep_dst(struct tcf_block *block);
int tcf_block_get(struct tcf_block **p_block,
struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 9a09926538ef..d5026525b94c 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -985,6 +985,45 @@ tcf_get_next_chain(struct tcf_block *block, struct tcf_chain *chain)
}
EXPORT_SYMBOL(tcf_get_next_chain);
+static struct tcf_proto *
+__tcf_get_next_proto(struct tcf_chain *chain, struct tcf_proto *tp)
+{
+ ASSERT_RTNL();
+ spin_lock(&chain->filter_chain_lock);
+
+ if (!tp)
+ tp = tcf_chain_dereference(chain->filter_chain, chain);
+ else
+ tp = tcf_chain_dereference(tp->next, chain);
+
+ if (tp)
+ tcf_proto_get(tp);
+
+ spin_unlock(&chain->filter_chain_lock);
+
+ return tp;
+}
+
+/* Function to be used by all clients that want to iterate over all tp's on
+ * chain. Users of this function must be tolerant to concurrent tp
+ * insertion/deletion or ensure that no concurrent chain modification is
+ * possible. Note that all netlink dump callbacks cannot guarantee to provide
+ * consistent dump because rtnl lock is released each time skb is filled with
+ * data and sent to user-space.
+ */
+
+struct tcf_proto *
+tcf_get_next_proto(struct tcf_chain *chain, struct tcf_proto *tp)
+{
+ struct tcf_proto *tp_next = __tcf_get_next_proto(chain, tp);
+
+ if (tp)
+ tcf_proto_put(tp, NULL);
+
+ return tp_next;
+}
+EXPORT_SYMBOL(tcf_get_next_proto);
+
static void tcf_block_flush_all_chains(struct tcf_block *block)
{
struct tcf_chain *chain;
@@ -1357,7 +1396,7 @@ tcf_block_playback_offloads(struct tcf_block *block, tc_setup_cb_t *cb,
struct netlink_ext_ack *extack)
{
struct tcf_chain *chain, *chain_prev;
- struct tcf_proto *tp;
+ struct tcf_proto *tp, *tp_prev;
int err;
for (chain = __tcf_get_next_chain(block, NULL);
@@ -1365,8 +1404,10 @@ tcf_block_playback_offloads(struct tcf_block *block, tc_setup_cb_t *cb,
chain_prev = chain,
chain = __tcf_get_next_chain(block, chain),
tcf_chain_put(chain_prev)) {
- for (tp = rtnl_dereference(chain->filter_chain); tp;
- tp = rtnl_dereference(tp->next)) {
+ for (tp = __tcf_get_next_proto(chain, NULL); tp;
+ tp_prev = tp,
+ tp = __tcf_get_next_proto(chain, tp),
+ tcf_proto_put(tp_prev, NULL)) {
if (tp->ops->reoffload) {
err = tp->ops->reoffload(tp, add, cb, cb_priv,
extack);
@@ -1383,6 +1424,7 @@ tcf_block_playback_offloads(struct tcf_block *block, tc_setup_cb_t *cb,
return 0;
err_playback_remove:
+ tcf_proto_put(tp, NULL);
tcf_chain_put(chain);
tcf_block_playback_offloads(block, cb, cb_priv, false, offload_in_use,
extack);
@@ -1706,8 +1748,8 @@ static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
{
struct tcf_proto *tp;
- for (tp = rtnl_dereference(chain->filter_chain);
- tp; tp = rtnl_dereference(tp->next))
+ for (tp = tcf_get_next_proto(chain, NULL);
+ tp; tp = tcf_get_next_proto(chain, tp))
tfilter_notify(net, oskb, n, tp, block,
q, parent, NULL, event, false);
}
@@ -2132,11 +2174,15 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
struct net *net = sock_net(skb->sk);
struct tcf_block *block = chain->block;
struct tcmsg *tcm = nlmsg_data(cb->nlh);
+ struct tcf_proto *tp, *tp_prev;
struct tcf_dump_args arg;
- struct tcf_proto *tp;
- for (tp = rtnl_dereference(chain->filter_chain);
- tp; tp = rtnl_dereference(tp->next), (*p_index)++) {
+ for (tp = __tcf_get_next_proto(chain, NULL);
+ tp;
+ tp_prev = tp,
+ tp = __tcf_get_next_proto(chain, tp),
+ tcf_proto_put(tp_prev, NULL),
+ (*p_index)++) {
if (*p_index < index_start)
continue;
if (TC_H_MAJ(tcm->tcm_info) &&
@@ -2153,7 +2199,7 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
RTM_NEWTFILTER) <= 0)
- return false;
+ goto errout;
cb->args[1] = 1;
}
@@ -2173,9 +2219,13 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
cb->args[2] = arg.w.cookie;
cb->args[1] = arg.w.count + 1;
if (arg.w.stop)
- return false;
+ goto errout;
}
return true;
+
+errout:
+ tcf_proto_put(tp, NULL);
+ return false;
}
/* called with RTNL */
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f80fd2d1b7e6..f472d3cf1ccb 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1917,8 +1917,8 @@ static void tc_bind_tclass(struct Qdisc *q, u32 portid, u32 clid,
chain = tcf_get_next_chain(block, chain)) {
struct tcf_proto *tp;
- for (tp = rtnl_dereference(chain->filter_chain);
- tp; tp = rtnl_dereference(tp->next)) {
+ for (tp = tcf_get_next_proto(chain, NULL);
+ tp; tp = tcf_get_next_proto(chain, tp)) {
struct tcf_bind_args arg = {};
arg.w.fn = tcf_node_bind;
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH net-next v2 09/17] net: sched: traverse classifiers in chain with tcf_get_next_proto()
2018-12-11 10:11 ` [PATCH net-next v2 09/17] net: sched: traverse classifiers in chain with tcf_get_next_proto() Vlad Buslov
@ 2018-12-14 10:15 ` kbuild test robot
0 siblings, 0 replies; 30+ messages in thread
From: kbuild test robot @ 2018-12-14 10:15 UTC (permalink / raw)
To: Vlad Buslov
Cc: kbuild-all, netdev, jhs, xiyou.wangcong, jiri, davem, ast, daniel,
Vlad Buslov
[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]
Hi Vlad,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/0day-ci/linux/commits/Vlad-Buslov/Refactor-classifier-API-to-work-with-chain-classifiers-without-rtnl-lock/20181211-231158
config: microblaze-mmu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=microblaze
All errors (new ones prefixed by >>):
net/sched/sch_generic.o: In function `qdisc_free_cb':
>> (.text+0xe00): undefined reference to `tc_queue_proto_work'
net/sched/sch_generic.o: In function `mini_qdisc_pair_swap':
(.text+0xe24): undefined reference to `tc_queue_proto_work'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12856 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH net-next v2 10/17] net: sched: refactor tp insert/delete for concurrent execution
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (8 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 09/17] net: sched: traverse classifiers in chain with tcf_get_next_proto() Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 11/17] net: sched: prevent insertion of new classifiers during chain flush Vlad Buslov
` (6 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Implement unique insertion function to atomically attach tcf proto to chain
after verifying that no other tcf proto with specified priority exists.
Implement delete function that verifies that tp is actually empty before
deleting it. Use these functions to refactor cls API to account for
concurrent tp and rule update instead of relying on rtnl lock. Add new
'deleting' flag to tcf proto. Use it to restart search when iterating over
tp's on chain to prevent accessing potentially inval tp->next pointer.
Extend tcf proto with spinlock that is intended to be used to protects its
data from concurrent modification instead of relying on rtnl mutex. Use it
to protect 'deleting' flag. Add lockdep macros to validate that lock is
held when accessing protected fields.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Always set 'tp_created' flag when creating tp to prevent releasing
the chain twice when tp with same priority was inserted
concurrently.
include/net/sch_generic.h | 18 +++++
net/sched/cls_api.c | 178 +++++++++++++++++++++++++++++++++++++++-------
2 files changed, 171 insertions(+), 25 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index a6dade2c22c8..53e5d5515f2a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -327,6 +327,11 @@ struct tcf_proto {
void *data;
const struct tcf_proto_ops *ops;
struct tcf_chain *chain;
+ /* Lock protects tcf_proto shared state and can be used by unlocked
+ * classifiers to protect their private data.
+ */
+ spinlock_t lock;
+ bool deleting;
refcount_t refcnt;
struct rcu_head rcu;
struct work_struct work;
@@ -388,16 +393,29 @@ static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
{
return lockdep_is_held(&chain->filter_chain_lock);
}
+
+static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
+{
+ return lockdep_is_held(&tp->lock);
+}
#else
static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
{
return true;
}
+
+static inline bool lockdep_tcf_proto_is_locked(struct tcf_proto *tp)
+{
+ return true;
+}
#endif /* #ifdef CONFIG_PROVE_LOCKING */
#define tcf_chain_dereference(p, chain) \
rcu_dereference_protected(p, lockdep_tcf_chain_is_locked(chain))
+#define tcf_proto_dereference(p, tp) \
+ rcu_dereference_protected(p, lockdep_tcf_proto_is_locked(tp))
+
static inline void tcf_block_offload_inc(struct tcf_block *block, u32 *flags)
{
if (*flags & TCA_CLS_FLAGS_IN_HW)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d5026525b94c..ecb83a304dfc 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -198,6 +198,7 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
tp->prio = prio;
tp->chain = chain;
INIT_WORK(&tp->work, tcf_proto_destroy_work);
+ spin_lock_init(&tp->lock);
refcount_set(&tp->refcnt, 1);
err = tp->ops->init(tp);
@@ -230,6 +231,49 @@ static void tcf_proto_put(struct tcf_proto *tp,
tcf_proto_destroy(tp, extack);
}
+static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
+{
+ return -1;
+}
+
+static bool tcf_proto_is_empty(struct tcf_proto *tp)
+{
+ struct tcf_walker walker = { .fn = walker_noop, };
+
+ if (tp->ops->walk) {
+ tp->ops->walk(tp, &walker);
+ return !walker.stop;
+ }
+ return true;
+}
+
+static bool tcf_proto_check_delete(struct tcf_proto *tp)
+{
+ spin_lock(&tp->lock);
+ if (tcf_proto_is_empty(tp))
+ tp->deleting = true;
+ spin_unlock(&tp->lock);
+ return tp->deleting;
+}
+
+static void tcf_proto_mark_delete(struct tcf_proto *tp)
+{
+ spin_lock(&tp->lock);
+ tp->deleting = true;
+ spin_unlock(&tp->lock);
+}
+
+static bool tcf_proto_is_deleting(struct tcf_proto *tp)
+{
+ bool deleting;
+
+ spin_lock(&tp->lock);
+ deleting = tp->deleting;
+ spin_unlock(&tp->lock);
+
+ return deleting;
+}
+
#define ASSERT_BLOCK_LOCKED(block) \
lockdep_assert_held(&(block)->lock)
@@ -988,13 +1032,27 @@ EXPORT_SYMBOL(tcf_get_next_chain);
static struct tcf_proto *
__tcf_get_next_proto(struct tcf_chain *chain, struct tcf_proto *tp)
{
+ u32 prio = 0;
+
ASSERT_RTNL();
spin_lock(&chain->filter_chain_lock);
- if (!tp)
+ if (!tp) {
tp = tcf_chain_dereference(chain->filter_chain, chain);
- else
+ } else if (tcf_proto_is_deleting(tp)) {
+ /* 'deleting' flag is set and chain->filter_chain_lock was
+ * unlocked, which means next pointer could be invalid. Restart
+ * search.
+ */
+ prio = tp->prio + 1;
+ tp = tcf_chain_dereference(chain->filter_chain, chain);
+
+ for (; tp; tp = tcf_chain_dereference(tp->next, chain))
+ if (!tp->deleting && tp->prio >= prio)
+ break;
+ } else {
tp = tcf_chain_dereference(tp->next, chain);
+ }
if (tp)
tcf_proto_get(tp);
@@ -1598,6 +1656,7 @@ static void tcf_chain_tp_remove(struct tcf_chain *chain,
{
struct tcf_proto *next = tcf_chain_dereference(chain_info->next, chain);
+ tcf_proto_mark_delete(tp);
if (tp == chain->filter_chain)
tcf_chain0_head_change(chain, next);
RCU_INIT_POINTER(*chain_info->pprev, next);
@@ -1606,6 +1665,79 @@ static void tcf_chain_tp_remove(struct tcf_chain *chain,
static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
struct tcf_chain_info *chain_info,
u32 protocol, u32 prio,
+ bool prio_allocate);
+
+/* Try to insert new proto.
+ * If proto with specified priority already exists, free new proto
+ * and return existing one.
+ */
+
+static struct tcf_proto *tcf_chain_tp_insert_unique(struct tcf_chain *chain,
+ struct tcf_proto *tp_new,
+ u32 protocol, u32 prio)
+{
+ struct tcf_chain_info chain_info;
+ struct tcf_proto *tp;
+
+ spin_lock(&chain->filter_chain_lock);
+
+ tp = tcf_chain_tp_find(chain, &chain_info,
+ protocol, prio, false);
+ if (!tp)
+ tcf_chain_tp_insert(chain, &chain_info, tp_new);
+ spin_unlock(&chain->filter_chain_lock);
+
+ if (tp) {
+ tcf_proto_destroy(tp_new, NULL);
+ tp_new = tp;
+ }
+
+ return tp_new;
+}
+
+static void tcf_chain_tp_delete_empty(struct tcf_chain *chain,
+ struct tcf_proto *tp,
+ struct netlink_ext_ack *extack)
+{
+ struct tcf_chain_info chain_info;
+ struct tcf_proto *tp_iter;
+ struct tcf_proto **pprev;
+ struct tcf_proto *next;
+
+ spin_lock(&chain->filter_chain_lock);
+
+ /* Atomically find and remove tp from chain. */
+ for (pprev = &chain->filter_chain;
+ (tp_iter = tcf_chain_dereference(*pprev, chain));
+ pprev = &tp_iter->next) {
+ if (tp_iter == tp) {
+ chain_info.pprev = pprev;
+ chain_info.next = tp_iter->next;
+ WARN_ON(tp_iter->deleting);
+ break;
+ }
+ }
+ /* Verify that tp still exists and no new filters were inserted
+ * concurrently.
+ * Mark tp for deletion if it is empty.
+ */
+ if (!tp_iter || !tcf_proto_check_delete(tp)) {
+ spin_unlock(&chain->filter_chain_lock);
+ return;
+ }
+
+ next = tcf_chain_dereference(chain_info.next, chain);
+ if (tp == chain->filter_chain)
+ tcf_chain0_head_change(chain, next);
+ RCU_INIT_POINTER(*chain_info.pprev, next);
+ spin_unlock(&chain->filter_chain_lock);
+
+ tcf_proto_put(tp, extack);
+}
+
+static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
+ struct tcf_chain_info *chain_info,
+ u32 protocol, u32 prio,
bool prio_allocate)
{
struct tcf_proto **pprev;
@@ -1790,6 +1922,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
prio = TC_H_MAJ(t->tcm_info);
prio_allocate = false;
parent = t->tcm_parent;
+ tp = NULL;
cl = 0;
if (prio == 0) {
@@ -1837,6 +1970,8 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
}
if (tp == NULL) {
+ struct tcf_proto *tp_new = NULL;
+
/* Proto-tcf does not exist, create new one */
if (tca[TCA_KIND] == NULL || !protocol) {
@@ -1856,25 +1991,25 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
&chain_info));
spin_unlock(&chain->filter_chain_lock);
- tp = tcf_proto_create(nla_data(tca[TCA_KIND]),
- protocol, prio, chain, extack);
- if (IS_ERR(tp)) {
- err = PTR_ERR(tp);
+ tp_new = tcf_proto_create(nla_data(tca[TCA_KIND]),
+ protocol, prio, chain, extack);
+ if (IS_ERR(tp_new)) {
+ err = PTR_ERR(tp_new);
goto errout;
}
- spin_lock(&chain->filter_chain_lock);
- tcf_chain_tp_insert(chain, &chain_info, tp);
- spin_unlock(&chain->filter_chain_lock);
tp_created = 1;
- } else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind)) {
- NL_SET_ERR_MSG(extack, "Specified filter kind does not match existing one");
- err = -EINVAL;
- goto errout_locked;
+ tp = tcf_chain_tp_insert_unique(chain, tp_new, protocol, prio);
} else {
spin_unlock(&chain->filter_chain_lock);
}
+ if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind)) {
+ NL_SET_ERR_MSG(extack, "Specified filter kind does not match existing one");
+ err = -EINVAL;
+ goto errout;
+ }
+
fh = tp->ops->get(tp, t->tcm_handle);
if (!fh) {
@@ -1901,12 +2036,10 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (err == 0)
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_NEWTFILTER, false);
- else if (tp_created)
- tcf_proto_destroy(tp, NULL);
errout:
- if (chain)
- tcf_chain_put(chain);
+ if (err && tp_created)
+ tcf_chain_tp_delete_empty(chain, tp, NULL);
if (chain) {
if (tp && !IS_ERR(tp))
tcf_proto_put(tp, NULL);
@@ -2012,9 +2145,9 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tcf_chain_tp_remove(chain, &chain_info, tp);
spin_unlock(&chain->filter_chain_lock);
+ tcf_proto_put(tp, NULL);
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_DELTFILTER, false);
- tcf_proto_destroy(tp, extack);
err = 0;
goto errout;
}
@@ -2033,13 +2166,8 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
extack);
if (err)
goto errout;
- if (last) {
- spin_lock(&chain->filter_chain_lock);
- tcf_chain_tp_remove(chain, &chain_info, tp);
- spin_unlock(&chain->filter_chain_lock);
-
- tcf_proto_destroy(tp, extack);
- }
+ if (last)
+ tcf_chain_tp_delete_empty(chain, tp, extack);
}
errout:
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 11/17] net: sched: prevent insertion of new classifiers during chain flush
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (9 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 10/17] net: sched: refactor tp insert/delete for concurrent execution Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 12/17] net: sched: track rtnl lock status when validating extensions Vlad Buslov
` (5 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Extend tcf_chain with 'flushing' flag. Use the flag to prevent insertion of
new classifier instances when chain flushing is in progress in order to
prevent resource leak when tcf_proto is created by unlocked users
concurrently.
Return EAGAIN error from tcf_chain_tp_insert_unique() to restart
tc_new_tfilter() and lookup the chain/proto again.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Add additional check to prevent creation of new proto instance when
parent chain is being flushed to reduce CPU usage.
- Don't call tcf_chain_delete_empty() if tp insertion failed.
include/net/sch_generic.h | 1 +
net/sched/cls_api.c | 35 +++++++++++++++++++++++++++++------
2 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 53e5d5515f2a..51f0c812667b 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -362,6 +362,7 @@ struct tcf_chain {
unsigned int refcnt;
unsigned int action_refcnt;
bool explicitly_created;
+ bool flushing;
const struct tcf_proto_ops *tmplt_ops;
void *tmplt_priv;
};
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index ecb83a304dfc..bbcac772cc21 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -483,9 +483,12 @@ static void __tcf_chain_put(struct tcf_chain *chain, bool by_act,
spin_unlock(&block->lock);
/* The last dropped non-action reference will trigger notification. */
- if (is_last && !by_act)
+ if (is_last && !by_act) {
tc_chain_notify_delete(tmplt_ops, tmplt_priv, chain_index,
block, NULL, 0, 0, false);
+ /* Last reference to chain, no need to lock. */
+ chain->flushing = false;
+ }
if (refcnt == 0) {
tc_chain_tmplt_del(tmplt_ops, tmplt_priv);
@@ -517,6 +520,7 @@ static void tcf_chain_flush(struct tcf_chain *chain)
tp = tcf_chain_dereference(chain->filter_chain, chain);
RCU_INIT_POINTER(chain->filter_chain, NULL);
tcf_chain0_head_change(chain, NULL);
+ chain->flushing = true;
spin_unlock(&chain->filter_chain_lock);
while (tp) {
@@ -1639,15 +1643,20 @@ static struct tcf_proto *tcf_chain_tp_prev(struct tcf_chain *chain,
return tcf_chain_dereference(*chain_info->pprev, chain);
}
-static void tcf_chain_tp_insert(struct tcf_chain *chain,
- struct tcf_chain_info *chain_info,
- struct tcf_proto *tp)
+static int tcf_chain_tp_insert(struct tcf_chain *chain,
+ struct tcf_chain_info *chain_info,
+ struct tcf_proto *tp)
{
+ if (chain->flushing)
+ return -EAGAIN;
+
if (*chain_info->pprev == chain->filter_chain)
tcf_chain0_head_change(chain, tp);
tcf_proto_get(tp);
RCU_INIT_POINTER(tp->next, tcf_chain_tp_prev(chain, chain_info));
rcu_assign_pointer(*chain_info->pprev, tp);
+
+ return 0;
}
static void tcf_chain_tp_remove(struct tcf_chain *chain,
@@ -1678,18 +1687,22 @@ static struct tcf_proto *tcf_chain_tp_insert_unique(struct tcf_chain *chain,
{
struct tcf_chain_info chain_info;
struct tcf_proto *tp;
+ int err = 0;
spin_lock(&chain->filter_chain_lock);
tp = tcf_chain_tp_find(chain, &chain_info,
protocol, prio, false);
if (!tp)
- tcf_chain_tp_insert(chain, &chain_info, tp_new);
+ err = tcf_chain_tp_insert(chain, &chain_info, tp_new);
spin_unlock(&chain->filter_chain_lock);
if (tp) {
tcf_proto_destroy(tp_new, NULL);
tp_new = tp;
+ } else if (err) {
+ tcf_proto_destroy(tp_new, NULL);
+ tp_new = ERR_PTR(err);
}
return tp_new;
@@ -1972,6 +1985,11 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (tp == NULL) {
struct tcf_proto *tp_new = NULL;
+ if (chain->flushing) {
+ err = -EAGAIN;
+ goto errout_locked;
+ }
+
/* Proto-tcf does not exist, create new one */
if (tca[TCA_KIND] == NULL || !protocol) {
@@ -1995,11 +2013,15 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
protocol, prio, chain, extack);
if (IS_ERR(tp_new)) {
err = PTR_ERR(tp_new);
- goto errout;
+ goto errout_tp;
}
tp_created = 1;
tp = tcf_chain_tp_insert_unique(chain, tp_new, protocol, prio);
+ if (IS_ERR(tp)) {
+ err = PTR_ERR(tp);
+ goto errout_tp;
+ }
} else {
spin_unlock(&chain->filter_chain_lock);
}
@@ -2040,6 +2062,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
errout:
if (err && tp_created)
tcf_chain_tp_delete_empty(chain, tp, NULL);
+errout_tp:
if (chain) {
if (tp && !IS_ERR(tp))
tcf_proto_put(tp, NULL);
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 12/17] net: sched: track rtnl lock status when validating extensions
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (10 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 11/17] net: sched: prevent insertion of new classifiers during chain flush Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 13/17] net: sched: extend proto ops with 'put' callback Vlad Buslov
` (4 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Actions API is already updated to not rely on rtnl lock for
synchronization. However, it need to be provided with rtnl status when
called from classifiers API in order to be able to correctly release the
lock when loading kernel module.
Extend extension validation function with 'rtnl_held' flag which is passed
to actions API. Add new 'rtnl_held' parameter to tcf_exts_validate() in cls
API. No classifier is currently updated to support unlocked execution, so
pass hardcoded 'true' flag parameter value.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/pkt_cls.h | 2 +-
net/sched/cls_api.c | 9 +++++----
net/sched/cls_basic.c | 2 +-
net/sched/cls_bpf.c | 3 ++-
net/sched/cls_cgroup.c | 2 +-
net/sched/cls_flow.c | 2 +-
net/sched/cls_flower.c | 3 ++-
net/sched/cls_fw.c | 2 +-
net/sched/cls_matchall.c | 3 ++-
net/sched/cls_route.c | 2 +-
net/sched/cls_rsvp.h | 3 ++-
net/sched/cls_tcindex.c | 2 +-
net/sched/cls_u32.c | 2 +-
13 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 411ac4334942..5c3cd5b96ab3 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -415,7 +415,7 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
struct nlattr **tb, struct nlattr *rate_tlv,
- struct tcf_exts *exts, bool ovr,
+ struct tcf_exts *exts, bool ovr, bool rtnl_held,
struct netlink_ext_ack *extack);
void tcf_exts_destroy(struct tcf_exts *exts);
void tcf_exts_change(struct tcf_exts *dst, struct tcf_exts *src);
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index bbcac772cc21..bc8d1d1600a4 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -2871,7 +2871,7 @@ EXPORT_SYMBOL(tcf_exts_destroy);
int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
struct nlattr *rate_tlv, struct tcf_exts *exts, bool ovr,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
#ifdef CONFIG_NET_CLS_ACT
{
@@ -2881,7 +2881,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
if (exts->police && tb[exts->police]) {
act = tcf_action_init_1(net, tp, tb[exts->police],
rate_tlv, "police", ovr,
- TCA_ACT_BIND, true, extack);
+ TCA_ACT_BIND, rtnl_held,
+ extack);
if (IS_ERR(act))
return PTR_ERR(act);
@@ -2893,8 +2894,8 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
err = tcf_action_init(net, tp, tb[exts->action],
rate_tlv, NULL, ovr, TCA_ACT_BIND,
- exts->actions, &attr_size, true,
- extack);
+ exts->actions, &attr_size,
+ rtnl_held, extack);
if (err < 0)
return err;
exts->nr_actions = err;
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 6a5dce8baf19..90e44888f85d 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -148,7 +148,7 @@ static int basic_set_parms(struct net *net, struct tcf_proto *tp,
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, extack);
+ err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, true, extack);
if (err < 0)
return err;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index fa6fe2fe0f32..9c0ac7c23ad7 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -417,7 +417,8 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
if ((!is_bpf && !is_ebpf) || (is_bpf && is_ebpf))
return -EINVAL;
- ret = tcf_exts_validate(net, tp, tb, est, &prog->exts, ovr, extack);
+ ret = tcf_exts_validate(net, tp, tb, est, &prog->exts, ovr, true,
+ extack);
if (ret < 0)
return ret;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 3bc01bdde165..663ee1c6d606 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -110,7 +110,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
goto errout;
err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &new->exts, ovr,
- extack);
+ true, extack);
if (err < 0)
goto errout;
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 2bb043cd436b..39a6407d4832 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -445,7 +445,7 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
goto err2;
err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &fnew->exts, ovr,
- extack);
+ true, extack);
if (err < 0)
goto err2;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 85e9f8e1da10..c4e28e5c8582 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -1261,7 +1261,8 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, extack);
+ err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, true,
+ extack);
if (err < 0)
return err;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 29eeeaf3ea44..c8173ebb69f2 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -217,7 +217,7 @@ static int fw_set_parms(struct net *net, struct tcf_proto *tp,
int err;
err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &f->exts, ovr,
- extack);
+ true, extack);
if (err < 0)
return err;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 856fa79d4ffd..5621c38a303a 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -142,7 +142,8 @@ static int mall_set_parms(struct net *net, struct tcf_proto *tp,
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &head->exts, ovr, extack);
+ err = tcf_exts_validate(net, tp, tb, est, &head->exts, ovr, true,
+ extack);
if (err < 0)
return err;
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 0404aa5fa7cb..44b26038c4c4 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -393,7 +393,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
struct route4_bucket *b;
int err;
- err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, extack);
+ err = tcf_exts_validate(net, tp, tb, est, &f->exts, ovr, true, extack);
if (err < 0)
return err;
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index e9ccf7daea7d..9dd9530e6a52 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -502,7 +502,8 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
err = tcf_exts_init(&e, TCA_RSVP_ACT, TCA_RSVP_POLICE);
if (err < 0)
return err;
- err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, ovr, extack);
+ err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, ovr, true,
+ extack);
if (err < 0)
goto errout2;
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 9ccc93f257db..b7dc667b6ec0 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -314,7 +314,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
err = tcf_exts_init(&e, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
if (err < 0)
return err;
- err = tcf_exts_validate(net, tp, tb, est, &e, ovr, extack);
+ err = tcf_exts_validate(net, tp, tb, est, &e, ovr, true, extack);
if (err < 0)
goto errout;
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 4c54bc440798..6305b6758b98 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -726,7 +726,7 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
{
int err;
- err = tcf_exts_validate(net, tp, tb, est, &n->exts, ovr, extack);
+ err = tcf_exts_validate(net, tp, tb, est, &n->exts, ovr, true, extack);
if (err < 0)
return err;
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 13/17] net: sched: extend proto ops with 'put' callback
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (11 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 12/17] net: sched: track rtnl lock status when validating extensions Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 14/17] net: sched: extend proto ops to support unlocked classifiers Vlad Buslov
` (3 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Add optional tp->ops->put() API to be implemented for filter reference
counting. This new function is called by cls API to release filter
reference for filters returned by tp->ops->change() or tp->ops->get()
functions. Implement tfilter_put() helper to call tp->ops->put() only for
classifiers that implement it.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/sch_generic.h | 1 +
net/sched/cls_api.c | 12 +++++++++++-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 51f0c812667b..f2e765efa38f 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -282,6 +282,7 @@ struct tcf_proto_ops {
struct netlink_ext_ack *extack);
void* (*get)(struct tcf_proto*, u32 handle);
+ void (*put)(struct tcf_proto *tp, void *f);
int (*change)(struct net *net, struct sk_buff *,
struct tcf_proto*, unsigned long,
u32 handle, struct nlattr **,
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index bc8d1d1600a4..2f181ca69a41 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1899,6 +1899,12 @@ static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
q, parent, NULL, event, false);
}
+static void tfilter_put(struct tcf_proto *tp, void *fh)
+{
+ if (tp->ops->put && fh)
+ tp->ops->put(tp, fh);
+}
+
static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
struct netlink_ext_ack *extack)
{
@@ -2041,6 +2047,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
goto errout;
}
} else if (n->nlmsg_flags & NLM_F_EXCL) {
+ tfilter_put(tp, fh);
NL_SET_ERR_MSG(extack, "Filter already exists");
err = -EEXIST;
goto errout;
@@ -2055,9 +2062,11 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
n->nlmsg_flags & NLM_F_CREATE ? TCA_ACT_NOREPLACE : TCA_ACT_REPLACE,
extack);
- if (err == 0)
+ if (err == 0) {
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
RTM_NEWTFILTER, false);
+ tfilter_put(tp, fh);
+ }
errout:
if (err && tp_created)
@@ -2288,6 +2297,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
NL_SET_ERR_MSG(extack, "Failed to send filter notify message");
}
+ tfilter_put(tp, fh);
errout:
if (chain) {
if (tp && !IS_ERR(tp))
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 14/17] net: sched: extend proto ops to support unlocked classifiers
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (12 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 13/17] net: sched: extend proto ops with 'put' callback Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 15/17] net: sched: add flags to Qdisc class ops struct Vlad Buslov
` (2 subsequent siblings)
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Add 'rtnl_held' flag to tcf proto change, delete, destroy, dump, walk
functions to track rtnl lock status. This allows classifiers to release
rtnl lock when necessary and to pass rtnl lock status to extensions and
driver offload callbacks.
Add flags field to tcf proto ops. Add flag value to indicate that
classifier doesn't require rtnl lock.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/sch_generic.h | 17 +++++++---
net/sched/cls_api.c | 84 +++++++++++++++++++++++++++--------------------
net/sched/cls_basic.c | 12 ++++---
net/sched/cls_bpf.c | 12 ++++---
net/sched/cls_cgroup.c | 11 ++++---
net/sched/cls_flow.c | 13 +++++---
net/sched/cls_flower.c | 13 +++++---
net/sched/cls_fw.c | 13 +++++---
net/sched/cls_matchall.c | 13 +++++---
net/sched/cls_route.c | 12 ++++---
net/sched/cls_rsvp.h | 13 +++++---
net/sched/cls_tcindex.c | 15 +++++----
net/sched/cls_u32.c | 12 ++++---
net/sched/sch_api.c | 2 +-
14 files changed, 144 insertions(+), 98 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f2e765efa38f..0bb69fefb188 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -278,7 +278,7 @@ struct tcf_proto_ops {
const struct tcf_proto *,
struct tcf_result *);
int (*init)(struct tcf_proto*);
- void (*destroy)(struct tcf_proto *tp,
+ void (*destroy)(struct tcf_proto *tp, bool rtnl_held,
struct netlink_ext_ack *extack);
void* (*get)(struct tcf_proto*, u32 handle);
@@ -286,12 +286,13 @@ struct tcf_proto_ops {
int (*change)(struct net *net, struct sk_buff *,
struct tcf_proto*, unsigned long,
u32 handle, struct nlattr **,
- void **, bool,
+ void **, bool, bool,
struct netlink_ext_ack *);
int (*delete)(struct tcf_proto *tp, void *arg,
- bool *last,
+ bool *last, bool rtnl_held,
struct netlink_ext_ack *);
- void (*walk)(struct tcf_proto*, struct tcf_walker *arg);
+ void (*walk)(struct tcf_proto *tp,
+ struct tcf_walker *arg, bool rtnl_held);
int (*reoffload)(struct tcf_proto *tp, bool add,
tc_setup_cb_t *cb, void *cb_priv,
struct netlink_ext_ack *extack);
@@ -304,12 +305,18 @@ struct tcf_proto_ops {
/* rtnetlink specific */
int (*dump)(struct net*, struct tcf_proto*, void *,
- struct sk_buff *skb, struct tcmsg*);
+ struct sk_buff *skb, struct tcmsg*,
+ bool);
int (*tmplt_dump)(struct sk_buff *skb,
struct net *net,
void *tmplt_priv);
struct module *owner;
+ int flags;
+};
+
+enum tcf_proto_ops_flags {
+ TCF_PROTO_OPS_DOIT_UNLOCKED = 1,
};
struct tcf_proto {
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 2f181ca69a41..c63673b7500c 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -61,7 +61,8 @@ static const struct tcf_proto_ops *__tcf_proto_lookup_ops(const char *kind)
}
static const struct tcf_proto_ops *
-tcf_proto_lookup_ops(const char *kind, struct netlink_ext_ack *extack)
+tcf_proto_lookup_ops(const char *kind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
const struct tcf_proto_ops *ops;
@@ -69,9 +70,11 @@ tcf_proto_lookup_ops(const char *kind, struct netlink_ext_ack *extack)
if (ops)
return ops;
#ifdef CONFIG_MODULES
- rtnl_unlock();
+ if (rtnl_held)
+ rtnl_unlock();
request_module("cls_%s", kind);
- rtnl_lock();
+ if (rtnl_held)
+ rtnl_lock();
ops = __tcf_proto_lookup_ops(kind);
/* We dropped the RTNL semaphore in order to perform
* the module load. So, even if we succeeded in loading
@@ -169,7 +172,7 @@ static void tcf_proto_destroy_work(struct work_struct *work)
rtnl_lock();
- tp->ops->destroy(tp, NULL);
+ tp->ops->destroy(tp, true, NULL);
module_put(tp->ops->owner);
kfree_rcu(tp, rcu);
tcf_chain_put(chain);
@@ -179,6 +182,7 @@ static void tcf_proto_destroy_work(struct work_struct *work)
static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
u32 prio, struct tcf_chain *chain,
+ bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tcf_proto *tp;
@@ -188,7 +192,7 @@ static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
if (!tp)
return ERR_PTR(-ENOBUFS);
- tp->ops = tcf_proto_lookup_ops(kind, extack);
+ tp->ops = tcf_proto_lookup_ops(kind, rtnl_held, extack);
if (IS_ERR(tp->ops)) {
err = PTR_ERR(tp->ops);
goto errout;
@@ -236,21 +240,21 @@ static int walker_noop(struct tcf_proto *tp, void *d, struct tcf_walker *arg)
return -1;
}
-static bool tcf_proto_is_empty(struct tcf_proto *tp)
+static bool tcf_proto_is_empty(struct tcf_proto *tp, bool rtnl_held)
{
struct tcf_walker walker = { .fn = walker_noop, };
if (tp->ops->walk) {
- tp->ops->walk(tp, &walker);
+ tp->ops->walk(tp, &walker, rtnl_held);
return !walker.stop;
}
return true;
}
-static bool tcf_proto_check_delete(struct tcf_proto *tp)
+static bool tcf_proto_check_delete(struct tcf_proto *tp, bool rtnl_held)
{
spin_lock(&tp->lock);
- if (tcf_proto_is_empty(tp))
+ if (tcf_proto_is_empty(tp, rtnl_held))
tp->deleting = true;
spin_unlock(&tp->lock);
return tp->deleting;
@@ -1709,7 +1713,7 @@ static struct tcf_proto *tcf_chain_tp_insert_unique(struct tcf_chain *chain,
}
static void tcf_chain_tp_delete_empty(struct tcf_chain *chain,
- struct tcf_proto *tp,
+ struct tcf_proto *tp, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tcf_chain_info chain_info;
@@ -1734,7 +1738,7 @@ static void tcf_chain_tp_delete_empty(struct tcf_chain *chain,
* concurrently.
* Mark tp for deletion if it is empty.
*/
- if (!tp_iter || !tcf_proto_check_delete(tp)) {
+ if (!tp_iter || !tcf_proto_check_delete(tp, rtnl_held)) {
spin_unlock(&chain->filter_chain_lock);
return;
}
@@ -1784,7 +1788,8 @@ static struct tcf_proto *tcf_chain_tp_find(struct tcf_chain *chain,
static int tcf_fill_node(struct net *net, struct sk_buff *skb,
struct tcf_proto *tp, struct tcf_block *block,
struct Qdisc *q, u32 parent, void *fh,
- u32 portid, u32 seq, u16 flags, int event)
+ u32 portid, u32 seq, u16 flags, int event,
+ bool rtnl_held)
{
struct tcmsg *tcm;
struct nlmsghdr *nlh;
@@ -1812,7 +1817,8 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
if (!fh) {
tcm->tcm_handle = 0;
} else {
- if (tp->ops->dump && tp->ops->dump(net, tp, fh, skb, tcm) < 0)
+ if (tp->ops->dump &&
+ tp->ops->dump(net, tp, fh, skb, tcm, rtnl_held) < 0)
goto nla_put_failure;
}
nlh->nlmsg_len = skb_tail_pointer(skb) - b;
@@ -1827,7 +1833,8 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
static int tfilter_notify(struct net *net, struct sk_buff *oskb,
struct nlmsghdr *n, struct tcf_proto *tp,
struct tcf_block *block, struct Qdisc *q,
- u32 parent, void *fh, int event, bool unicast)
+ u32 parent, void *fh, int event, bool unicast,
+ bool rtnl_held)
{
struct sk_buff *skb;
u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -1837,7 +1844,8 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
return -ENOBUFS;
if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
- n->nlmsg_seq, n->nlmsg_flags, event) <= 0) {
+ n->nlmsg_seq, n->nlmsg_flags, event,
+ rtnl_held) <= 0) {
kfree_skb(skb);
return -EINVAL;
}
@@ -1853,7 +1861,7 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
struct nlmsghdr *n, struct tcf_proto *tp,
struct tcf_block *block, struct Qdisc *q,
u32 parent, void *fh, bool unicast, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct sk_buff *skb;
u32 portid = oskb ? NETLINK_CB(oskb).portid : 0;
@@ -1864,13 +1872,14 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
return -ENOBUFS;
if (tcf_fill_node(net, skb, tp, block, q, parent, fh, portid,
- n->nlmsg_seq, n->nlmsg_flags, RTM_DELTFILTER) <= 0) {
+ n->nlmsg_seq, n->nlmsg_flags, RTM_DELTFILTER,
+ rtnl_held) <= 0) {
NL_SET_ERR_MSG(extack, "Failed to build del event notification");
kfree_skb(skb);
return -EINVAL;
}
- err = tp->ops->delete(tp, fh, last, extack);
+ err = tp->ops->delete(tp, fh, last, rtnl_held, extack);
if (err) {
kfree_skb(skb);
return err;
@@ -1889,14 +1898,15 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
static void tfilter_notify_chain(struct net *net, struct sk_buff *oskb,
struct tcf_block *block, struct Qdisc *q,
u32 parent, struct nlmsghdr *n,
- struct tcf_chain *chain, int event)
+ struct tcf_chain *chain, int event,
+ bool rtnl_held)
{
struct tcf_proto *tp;
for (tp = tcf_get_next_proto(chain, NULL);
tp; tp = tcf_get_next_proto(chain, tp))
tfilter_notify(net, oskb, n, tp, block,
- q, parent, NULL, event, false);
+ q, parent, NULL, event, false, rtnl_held);
}
static void tfilter_put(struct tcf_proto *tp, void *fh)
@@ -1925,6 +1935,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
void *fh;
int err;
int tp_created;
+ bool rtnl_held = true;
if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
return -EPERM;
@@ -2016,7 +2027,8 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
spin_unlock(&chain->filter_chain_lock);
tp_new = tcf_proto_create(nla_data(tca[TCA_KIND]),
- protocol, prio, chain, extack);
+ protocol, prio, chain, rtnl_held,
+ extack);
if (IS_ERR(tp_new)) {
err = PTR_ERR(tp_new);
goto errout_tp;
@@ -2061,16 +2073,16 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh,
n->nlmsg_flags & NLM_F_CREATE ? TCA_ACT_NOREPLACE : TCA_ACT_REPLACE,
- extack);
+ rtnl_held, extack);
if (err == 0) {
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
- RTM_NEWTFILTER, false);
+ RTM_NEWTFILTER, false, rtnl_held);
tfilter_put(tp, fh);
}
errout:
if (err && tp_created)
- tcf_chain_tp_delete_empty(chain, tp, NULL);
+ tcf_chain_tp_delete_empty(chain, tp, rtnl_held, NULL);
errout_tp:
if (chain) {
if (tp && !IS_ERR(tp))
@@ -2107,6 +2119,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
unsigned long cl = 0;
void *fh = NULL;
int err;
+ bool rtnl_held = true;
if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
return -EPERM;
@@ -2156,7 +2169,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (prio == 0) {
tfilter_notify_chain(net, skb, block, q, parent, n,
- chain, RTM_DELTFILTER);
+ chain, RTM_DELTFILTER, rtnl_held);
tcf_chain_flush(chain);
err = 0;
goto errout;
@@ -2179,7 +2192,7 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tcf_proto_put(tp, NULL);
tfilter_notify(net, skb, n, tp, block, q, parent, fh,
- RTM_DELTFILTER, false);
+ RTM_DELTFILTER, false, rtnl_held);
err = 0;
goto errout;
}
@@ -2195,11 +2208,12 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
err = tfilter_del_notify(net, skb, n, tp, block,
q, parent, fh, false, &last,
- extack);
+ rtnl_held, extack);
+
if (err)
goto errout;
if (last)
- tcf_chain_tp_delete_empty(chain, tp, extack);
+ tcf_chain_tp_delete_empty(chain, tp, rtnl_held, extack);
}
errout:
@@ -2234,6 +2248,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
unsigned long cl = 0;
void *fh = NULL;
int err;
+ bool rtnl_held = true;
err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, rtm_tca_policy, extack);
if (err < 0)
@@ -2292,7 +2307,7 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
err = -ENOENT;
} else {
err = tfilter_notify(net, skb, n, tp, block, q, parent,
- fh, RTM_NEWTFILTER, true);
+ fh, RTM_NEWTFILTER, true, rtnl_held);
if (err < 0)
NL_SET_ERR_MSG(extack, "Failed to send filter notify message");
}
@@ -2325,7 +2340,7 @@ static int tcf_node_dump(struct tcf_proto *tp, void *n, struct tcf_walker *arg)
return tcf_fill_node(net, a->skb, tp, a->block, a->q, a->parent,
n, NETLINK_CB(a->cb->skb).portid,
a->cb->nlh->nlmsg_seq, NLM_F_MULTI,
- RTM_NEWTFILTER);
+ RTM_NEWTFILTER, true);
}
static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
@@ -2359,9 +2374,8 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
if (tcf_fill_node(net, skb, tp, block, q, parent, NULL,
NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
- RTM_NEWTFILTER) <= 0)
+ RTM_NEWTFILTER, true) <= 0)
goto errout;
-
cb->args[1] = 1;
}
if (!tp->ops->walk)
@@ -2376,7 +2390,7 @@ static bool tcf_chain_dump(struct tcf_chain *chain, struct Qdisc *q, u32 parent,
arg.w.skip = cb->args[1] - 1;
arg.w.count = 0;
arg.w.cookie = cb->args[2];
- tp->ops->walk(tp, &arg.w);
+ tp->ops->walk(tp, &arg.w, true);
cb->args[2] = arg.w.cookie;
cb->args[1] = arg.w.count + 1;
if (arg.w.stop)
@@ -2598,7 +2612,7 @@ static int tc_chain_tmplt_add(struct tcf_chain *chain, struct net *net,
if (!tca[TCA_KIND])
return 0;
- ops = tcf_proto_lookup_ops(nla_data(tca[TCA_KIND]), extack);
+ ops = tcf_proto_lookup_ops(nla_data(tca[TCA_KIND]), true, extack);
if (IS_ERR(ops))
return PTR_ERR(ops);
if (!ops->tmplt_create || !ops->tmplt_destroy || !ops->tmplt_dump) {
@@ -2728,7 +2742,7 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
break;
case RTM_DELCHAIN:
tfilter_notify_chain(net, skb, block, q, parent, n,
- chain, RTM_DELTFILTER);
+ chain, RTM_DELTFILTER, true);
/* Flush the chain first as the user requested chain removal. */
tcf_chain_flush(chain);
/* In case the chain was successfully deleted, put a reference
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index 90e44888f85d..7bf9e4335d74 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -102,7 +102,8 @@ static void basic_delete_filter_work(struct work_struct *work)
rtnl_unlock();
}
-static void basic_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void basic_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct basic_head *head = rtnl_dereference(tp->root);
struct basic_filter *f, *n;
@@ -121,7 +122,7 @@ static void basic_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
}
static int basic_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct basic_head *head = rtnl_dereference(tp->root);
struct basic_filter *f = arg;
@@ -168,7 +169,7 @@ static int basic_set_parms(struct net *net, struct tcf_proto *tp,
static int basic_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base, u32 handle,
struct nlattr **tca, void **arg, bool ovr,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
int err;
struct basic_head *head = rtnl_dereference(tp->root);
@@ -236,7 +237,8 @@ static int basic_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static void basic_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void basic_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct basic_head *head = rtnl_dereference(tp->root);
struct basic_filter *f;
@@ -263,7 +265,7 @@ static void basic_bind_class(void *fh, u32 classid, unsigned long cl)
}
static int basic_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct basic_filter *f = fh;
struct nlattr *nest;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 9c0ac7c23ad7..455bb0bca959 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -298,7 +298,7 @@ static void __cls_bpf_delete(struct tcf_proto *tp, struct cls_bpf_prog *prog,
}
static int cls_bpf_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct cls_bpf_head *head = rtnl_dereference(tp->root);
@@ -307,7 +307,7 @@ static int cls_bpf_delete(struct tcf_proto *tp, void *arg, bool *last,
return 0;
}
-static void cls_bpf_destroy(struct tcf_proto *tp,
+static void cls_bpf_destroy(struct tcf_proto *tp, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct cls_bpf_head *head = rtnl_dereference(tp->root);
@@ -456,7 +456,8 @@ static int cls_bpf_set_parms(struct net *net, struct tcf_proto *tp,
static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base,
u32 handle, struct nlattr **tca,
- void **arg, bool ovr, struct netlink_ext_ack *extack)
+ void **arg, bool ovr, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct cls_bpf_head *head = rtnl_dereference(tp->root);
struct cls_bpf_prog *oldprog = *arg;
@@ -576,7 +577,7 @@ static int cls_bpf_dump_ebpf_info(const struct cls_bpf_prog *prog,
}
static int cls_bpf_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *tm)
+ struct sk_buff *skb, struct tcmsg *tm, bool rtnl_held)
{
struct cls_bpf_prog *prog = fh;
struct nlattr *nest;
@@ -636,7 +637,8 @@ static void cls_bpf_bind_class(void *fh, u32 classid, unsigned long cl)
prog->res.class = cl;
}
-static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct cls_bpf_head *head = rtnl_dereference(tp->root);
struct cls_bpf_prog *prog;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index 663ee1c6d606..1cef3b416094 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -78,7 +78,7 @@ static void cls_cgroup_destroy_work(struct work_struct *work)
static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base,
u32 handle, struct nlattr **tca,
- void **arg, bool ovr,
+ void **arg, bool ovr, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct nlattr *tb[TCA_CGROUP_MAX + 1];
@@ -130,7 +130,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static void cls_cgroup_destroy(struct tcf_proto *tp,
+static void cls_cgroup_destroy(struct tcf_proto *tp, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct cls_cgroup_head *head = rtnl_dereference(tp->root);
@@ -145,12 +145,13 @@ static void cls_cgroup_destroy(struct tcf_proto *tp,
}
static int cls_cgroup_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
return -EOPNOTSUPP;
}
-static void cls_cgroup_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void cls_cgroup_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct cls_cgroup_head *head = rtnl_dereference(tp->root);
@@ -166,7 +167,7 @@ static void cls_cgroup_walk(struct tcf_proto *tp, struct tcf_walker *arg)
}
static int cls_cgroup_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct cls_cgroup_head *head = rtnl_dereference(tp->root);
struct nlattr *nest;
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 39a6407d4832..204e2edae8d5 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -391,7 +391,8 @@ static void flow_destroy_filter_work(struct work_struct *work)
static int flow_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base,
u32 handle, struct nlattr **tca,
- void **arg, bool ovr, struct netlink_ext_ack *extack)
+ void **arg, bool ovr, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *fold, *fnew;
@@ -566,7 +567,7 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
}
static int flow_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *f = arg;
@@ -590,7 +591,8 @@ static int flow_init(struct tcf_proto *tp)
return 0;
}
-static void flow_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void flow_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *f, *next;
@@ -617,7 +619,7 @@ static void *flow_get(struct tcf_proto *tp, u32 handle)
}
static int flow_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct flow_filter *f = fh;
struct nlattr *nest;
@@ -677,7 +679,8 @@ static int flow_dump(struct net *net, struct tcf_proto *tp, void *fh,
return -1;
}
-static void flow_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void flow_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *f;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index c4e28e5c8582..72cb116cc30e 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -454,7 +454,8 @@ static void fl_destroy_sleepable(struct work_struct *work)
module_put(THIS_MODULE);
}
-static void fl_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void fl_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
struct fl_flow_mask *mask, *next_mask;
@@ -1289,7 +1290,8 @@ static int fl_set_parms(struct net *net, struct tcf_proto *tp,
static int fl_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base,
u32 handle, struct nlattr **tca,
- void **arg, bool ovr, struct netlink_ext_ack *extack)
+ void **arg, bool ovr, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
struct cls_fl_filter *fold = *arg;
@@ -1416,7 +1418,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
}
static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
struct cls_fl_filter *f = arg;
@@ -1429,7 +1431,8 @@ static int fl_delete(struct tcf_proto *tp, void *arg, bool *last,
return 0;
}
-static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void fl_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct cls_fl_head *head = rtnl_dereference(tp->root);
struct cls_fl_filter *f;
@@ -2005,7 +2008,7 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
}
static int fl_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct cls_fl_filter *f = fh;
struct nlattr *nest;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index c8173ebb69f2..317151bae73b 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -139,7 +139,8 @@ static void fw_delete_filter_work(struct work_struct *work)
rtnl_unlock();
}
-static void fw_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void fw_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct fw_head *head = rtnl_dereference(tp->root);
struct fw_filter *f;
@@ -163,7 +164,7 @@ static void fw_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
}
static int fw_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct fw_head *head = rtnl_dereference(tp->root);
struct fw_filter *f = arg;
@@ -250,7 +251,8 @@ static int fw_set_parms(struct net *net, struct tcf_proto *tp,
static int fw_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base,
u32 handle, struct nlattr **tca, void **arg,
- bool ovr, struct netlink_ext_ack *extack)
+ bool ovr, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct fw_head *head = rtnl_dereference(tp->root);
struct fw_filter *f = *arg;
@@ -354,7 +356,8 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static void fw_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void fw_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct fw_head *head = rtnl_dereference(tp->root);
int h;
@@ -384,7 +387,7 @@ static void fw_walk(struct tcf_proto *tp, struct tcf_walker *arg)
}
static int fw_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct fw_head *head = rtnl_dereference(tp->root);
struct fw_filter *f = fh;
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 5621c38a303a..4e6d1139434a 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -106,7 +106,8 @@ static int mall_replace_hw_filter(struct tcf_proto *tp,
return 0;
}
-static void mall_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void mall_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct cls_mall_head *head = rtnl_dereference(tp->root);
@@ -157,7 +158,8 @@ static int mall_set_parms(struct net *net, struct tcf_proto *tp,
static int mall_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base,
u32 handle, struct nlattr **tca,
- void **arg, bool ovr, struct netlink_ext_ack *extack)
+ void **arg, bool ovr, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct cls_mall_head *head = rtnl_dereference(tp->root);
struct nlattr *tb[TCA_MATCHALL_MAX + 1];
@@ -223,12 +225,13 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
}
static int mall_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
return -EOPNOTSUPP;
}
-static void mall_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void mall_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct cls_mall_head *head = rtnl_dereference(tp->root);
@@ -270,7 +273,7 @@ static int mall_reoffload(struct tcf_proto *tp, bool add, tc_setup_cb_t *cb,
}
static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct cls_mall_head *head = fh;
struct nlattr *nest;
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 44b26038c4c4..e590c3a2999d 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -276,7 +276,8 @@ static void route4_queue_work(struct route4_filter *f)
tcf_queue_work(&f->rwork, route4_delete_filter_work);
}
-static void route4_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void route4_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct route4_head *head = rtnl_dereference(tp->root);
int h1, h2;
@@ -312,7 +313,7 @@ static void route4_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
}
static int route4_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct route4_head *head = rtnl_dereference(tp->root);
struct route4_filter *f = arg;
@@ -468,7 +469,7 @@ static int route4_set_parms(struct net *net, struct tcf_proto *tp,
static int route4_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base, u32 handle,
struct nlattr **tca, void **arg, bool ovr,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct route4_head *head = rtnl_dereference(tp->root);
struct route4_filter __rcu **fp;
@@ -560,7 +561,8 @@ static int route4_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static void route4_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void route4_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct route4_head *head = rtnl_dereference(tp->root);
unsigned int h, h1;
@@ -597,7 +599,7 @@ static void route4_walk(struct tcf_proto *tp, struct tcf_walker *arg)
}
static int route4_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct route4_filter *f = fh;
struct nlattr *nest;
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index 9dd9530e6a52..4d3836178fa5 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -312,7 +312,8 @@ static void rsvp_delete_filter(struct tcf_proto *tp, struct rsvp_filter *f)
__rsvp_delete_filter(f);
}
-static void rsvp_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void rsvp_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct rsvp_head *data = rtnl_dereference(tp->root);
int h1, h2;
@@ -341,7 +342,7 @@ static void rsvp_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
}
static int rsvp_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct rsvp_head *head = rtnl_dereference(tp->root);
struct rsvp_filter *nfp, *f = arg;
@@ -477,7 +478,8 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base,
u32 handle,
struct nlattr **tca,
- void **arg, bool ovr, struct netlink_ext_ack *extack)
+ void **arg, bool ovr, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct rsvp_head *data = rtnl_dereference(tp->root);
struct rsvp_filter *f, *nfp;
@@ -655,7 +657,8 @@ static int rsvp_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static void rsvp_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void rsvp_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct rsvp_head *head = rtnl_dereference(tp->root);
unsigned int h, h1;
@@ -689,7 +692,7 @@ static void rsvp_walk(struct tcf_proto *tp, struct tcf_walker *arg)
}
static int rsvp_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct rsvp_filter *f = fh;
struct rsvp_session *s;
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index b7dc667b6ec0..14d6b4058045 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -173,7 +173,7 @@ static void tcindex_destroy_fexts_work(struct work_struct *work)
}
static int tcindex_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct tcindex_data *p = rtnl_dereference(tp->root);
struct tcindex_filter_result *r = arg;
@@ -226,7 +226,7 @@ static int tcindex_destroy_element(struct tcf_proto *tp,
{
bool last;
- return tcindex_delete(tp, arg, &last, NULL);
+ return tcindex_delete(tp, arg, &last, false, NULL);
}
static void __tcindex_destroy(struct rcu_head *head)
@@ -499,7 +499,7 @@ static int
tcindex_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base, u32 handle,
struct nlattr **tca, void **arg, bool ovr,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct nlattr *opt = tca[TCA_OPTIONS];
struct nlattr *tb[TCA_TCINDEX_MAX + 1];
@@ -522,7 +522,8 @@ tcindex_change(struct net *net, struct sk_buff *in_skb,
tca[TCA_RATE], ovr, extack);
}
-static void tcindex_walk(struct tcf_proto *tp, struct tcf_walker *walker)
+static void tcindex_walk(struct tcf_proto *tp, struct tcf_walker *walker,
+ bool rtnl_held)
{
struct tcindex_data *p = rtnl_dereference(tp->root);
struct tcindex_filter *f, *next;
@@ -558,7 +559,7 @@ static void tcindex_walk(struct tcf_proto *tp, struct tcf_walker *walker)
}
}
-static void tcindex_destroy(struct tcf_proto *tp,
+static void tcindex_destroy(struct tcf_proto *tp, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tcindex_data *p = rtnl_dereference(tp->root);
@@ -568,14 +569,14 @@ static void tcindex_destroy(struct tcf_proto *tp,
walker.count = 0;
walker.skip = 0;
walker.fn = tcindex_destroy_element;
- tcindex_walk(tp, &walker);
+ tcindex_walk(tp, &walker, true);
call_rcu(&p->rcu, __tcindex_destroy);
}
static int tcindex_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct tcindex_data *p = rtnl_dereference(tp->root);
struct tcindex_filter_result *r = fh;
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 6305b6758b98..edb64ed94692 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -629,7 +629,8 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
return -ENOENT;
}
-static void u32_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
+static void u32_destroy(struct tcf_proto *tp, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_u_common *tp_c = tp->data;
struct tc_u_hnode *root_ht = rtnl_dereference(tp->root);
@@ -663,7 +664,7 @@ static void u32_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
}
static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct tc_u_hnode *ht = arg;
struct tc_u_common *tp_c = tp->data;
@@ -858,7 +859,7 @@ static struct tc_u_knode *u32_init_knode(struct tcf_proto *tp,
static int u32_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base, u32 handle,
- struct nlattr **tca, void **arg, bool ovr,
+ struct nlattr **tca, void **arg, bool ovr, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tc_u_common *tp_c = tp->data;
@@ -1123,7 +1124,8 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
return err;
}
-static void u32_walk(struct tcf_proto *tp, struct tcf_walker *arg)
+static void u32_walk(struct tcf_proto *tp, struct tcf_walker *arg,
+ bool rtnl_held)
{
struct tc_u_common *tp_c = tp->data;
struct tc_u_hnode *ht;
@@ -1281,7 +1283,7 @@ static void u32_bind_class(void *fh, u32 classid, unsigned long cl)
}
static int u32_dump(struct net *net, struct tcf_proto *tp, void *fh,
- struct sk_buff *skb, struct tcmsg *t)
+ struct sk_buff *skb, struct tcmsg *t, bool rtnl_held)
{
struct tc_u_knode *n = fh;
struct tc_u_hnode *ht_up, *ht_down;
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f472d3cf1ccb..06481071fc33 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1924,7 +1924,7 @@ static void tc_bind_tclass(struct Qdisc *q, u32 portid, u32 clid,
arg.w.fn = tcf_node_bind;
arg.classid = clid;
arg.cl = new_cl;
- tp->ops->walk(tp, &arg.w);
+ tp->ops->walk(tp, &arg.w, true);
}
}
}
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 15/17] net: sched: add flags to Qdisc class ops struct
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (13 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 14/17] net: sched: extend proto ops to support unlocked classifiers Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:11 ` [PATCH net-next v2 16/17] net: sched: refactor tcf_block_find() into standalone functions Vlad Buslov
2018-12-11 10:12 ` [PATCH net-next v2 17/17] net: sched: unlock rules update API Vlad Buslov
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Extend Qdisc_class_ops with flags. Create enum to hold possible class ops
flag values. Add first class ops flags value QDISC_CLASS_OPS_DOIT_UNLOCKED
to indicate that class ops functions can be called without taking rtnl
lock.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/sch_generic.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 0bb69fefb188..8541d6a036d9 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -184,6 +184,7 @@ static inline int qdisc_avail_bulklimit(const struct netdev_queue *txq)
}
struct Qdisc_class_ops {
+ unsigned int flags;
/* Child qdisc manipulation */
struct netdev_queue * (*select_queue)(struct Qdisc *, struct tcmsg *);
int (*graft)(struct Qdisc *, unsigned long cl,
@@ -215,6 +216,13 @@ struct Qdisc_class_ops {
struct gnet_dump *);
};
+/* Qdisc_class_ops flag values */
+
+/* Implements API that doesn't require rtnl lock */
+enum qdisc_class_ops_flags {
+ QDISC_CLASS_OPS_DOIT_UNLOCKED = 1,
+};
+
struct Qdisc_ops {
struct Qdisc_ops *next;
const struct Qdisc_class_ops *cl_ops;
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 16/17] net: sched: refactor tcf_block_find() into standalone functions
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (14 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 15/17] net: sched: add flags to Qdisc class ops struct Vlad Buslov
@ 2018-12-11 10:11 ` Vlad Buslov
2018-12-11 10:12 ` [PATCH net-next v2 17/17] net: sched: unlock rules update API Vlad Buslov
16 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:11 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Refactor tcf_block_find() code into three standalone functions:
- __tcf_qdisc_find() to lookup Qdisc and increment its reference counter.
- __tcf_qdisc_cl_find() to lookup class.
- __tcf_block_find() to lookup block and increment its reference counter.
This change is necessary to allow netlink tc rule update handlers to call
these functions directly in order to conditionally take rtnl lock
according to Qdisc class ops flags before calling any of class ops
functions.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/cls_api.c | 241 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 149 insertions(+), 92 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index c63673b7500c..0a841f65e518 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1105,6 +1105,142 @@ static void tcf_block_flush_all_chains(struct tcf_block *block)
}
}
+/* Lookup Qdisc and increments its reference counter.
+ * Set parent, if necessary.
+ */
+
+static int __tcf_qdisc_find(struct net *net, struct Qdisc **q,
+ u32 *parent, int ifindex, bool rtnl_held,
+ struct netlink_ext_ack *extack)
+{
+ const struct Qdisc_class_ops *cops;
+ struct net_device *dev;
+ int err = 0;
+
+ if (ifindex == TCM_IFINDEX_MAGIC_BLOCK)
+ return 0;
+
+ rcu_read_lock();
+
+ /* Find link */
+ dev = dev_get_by_index_rcu(net, ifindex);
+ if (!dev) {
+ rcu_read_unlock();
+ return -ENODEV;
+ }
+
+ /* Find qdisc */
+ if (!*parent) {
+ *q = dev->qdisc;
+ *parent = (*q)->handle;
+ } else {
+ *q = qdisc_lookup_rcu(dev, TC_H_MAJ(*parent));
+ if (!*q) {
+ NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
+ err = -EINVAL;
+ goto errout_rcu;
+ }
+ }
+
+ *q = qdisc_refcount_inc_nz(*q);
+ if (!*q) {
+ NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
+ err = -EINVAL;
+ goto errout_rcu;
+ }
+
+ /* Is it classful? */
+ cops = (*q)->ops->cl_ops;
+ if (!cops) {
+ NL_SET_ERR_MSG(extack, "Qdisc not classful");
+ err = -EINVAL;
+ goto errout_qdisc;
+ }
+
+ if (!cops->tcf_block) {
+ NL_SET_ERR_MSG(extack, "Class doesn't support blocks");
+ err = -EOPNOTSUPP;
+ goto errout_qdisc;
+ }
+
+errout_rcu:
+ /* At this point we know that qdisc is not noop_qdisc,
+ * which means that qdisc holds a reference to net_device
+ * and we hold a reference to qdisc, so it is safe to release
+ * rcu read lock.
+ */
+ rcu_read_unlock();
+ return err;
+
+errout_qdisc:
+ rcu_read_unlock();
+
+ if (rtnl_held)
+ qdisc_put(*q);
+ else
+ qdisc_put_unlocked(*q);
+ *q = NULL;
+
+ return err;
+}
+
+static int __tcf_qdisc_cl_find(struct Qdisc *q, u32 parent, unsigned long *cl,
+ int ifindex, struct netlink_ext_ack *extack)
+{
+ if (ifindex == TCM_IFINDEX_MAGIC_BLOCK)
+ return 0;
+
+ /* Do we search for filter, attached to class? */
+ if (TC_H_MIN(parent)) {
+ const struct Qdisc_class_ops *cops = q->ops->cl_ops;
+
+ *cl = cops->find(q, parent);
+ if (*cl == 0) {
+ NL_SET_ERR_MSG(extack, "Specified class doesn't exist");
+ return -ENOENT;
+ }
+ }
+
+ return 0;
+}
+
+static struct tcf_block *__tcf_block_find(struct net *net, struct Qdisc *q,
+ unsigned long cl, int ifindex,
+ u32 block_index,
+ struct netlink_ext_ack *extack)
+{
+ struct tcf_block *block;
+
+ if (ifindex == TCM_IFINDEX_MAGIC_BLOCK) {
+ block = tcf_block_refcnt_get(net, block_index);
+ if (!block) {
+ NL_SET_ERR_MSG(extack, "Block of given index was not found");
+ return ERR_PTR(-EINVAL);
+ }
+ } else {
+ const struct Qdisc_class_ops *cops = q->ops->cl_ops;
+
+ block = cops->tcf_block(q, cl, extack);
+ if (!block)
+ return ERR_PTR(-EINVAL);
+
+ if (tcf_block_shared(block)) {
+ NL_SET_ERR_MSG(extack, "This filter block is shared. Please use the block index to manipulate the filters");
+ return ERR_PTR(-EOPNOTSUPP);
+ }
+
+ /* Always take reference to block in order to support execution
+ * of rules update path of cls API without rtnl lock. Caller
+ * must release block when it is finished using it. 'if' block
+ * of this conditional obtain reference to block by calling
+ * tcf_block_refcnt_get().
+ */
+ refcount_inc(&block->refcnt);
+ }
+
+ return block;
+}
+
static void __tcf_block_put(struct tcf_block *block, struct Qdisc *q,
struct tcf_block_ext_info *ei)
{
@@ -1150,106 +1286,27 @@ static struct tcf_block *tcf_block_find(struct net *net, struct Qdisc **q,
struct tcf_block *block;
int err = 0;
- if (ifindex == TCM_IFINDEX_MAGIC_BLOCK) {
- block = tcf_block_refcnt_get(net, block_index);
- if (!block) {
- NL_SET_ERR_MSG(extack, "Block of given index was not found");
- return ERR_PTR(-EINVAL);
- }
- } else {
- const struct Qdisc_class_ops *cops;
- struct net_device *dev;
-
- rcu_read_lock();
-
- /* Find link */
- dev = dev_get_by_index_rcu(net, ifindex);
- if (!dev) {
- rcu_read_unlock();
- return ERR_PTR(-ENODEV);
- }
-
- /* Find qdisc */
- if (!*parent) {
- *q = dev->qdisc;
- *parent = (*q)->handle;
- } else {
- *q = qdisc_lookup_rcu(dev, TC_H_MAJ(*parent));
- if (!*q) {
- NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
- err = -EINVAL;
- goto errout_rcu;
- }
- }
-
- *q = qdisc_refcount_inc_nz(*q);
- if (!*q) {
- NL_SET_ERR_MSG(extack, "Parent Qdisc doesn't exists");
- err = -EINVAL;
- goto errout_rcu;
- }
-
- /* Is it classful? */
- cops = (*q)->ops->cl_ops;
- if (!cops) {
- NL_SET_ERR_MSG(extack, "Qdisc not classful");
- err = -EINVAL;
- goto errout_rcu;
- }
-
- if (!cops->tcf_block) {
- NL_SET_ERR_MSG(extack, "Class doesn't support blocks");
- err = -EOPNOTSUPP;
- goto errout_rcu;
- }
-
- /* At this point we know that qdisc is not noop_qdisc,
- * which means that qdisc holds a reference to net_device
- * and we hold a reference to qdisc, so it is safe to release
- * rcu read lock.
- */
- rcu_read_unlock();
+ ASSERT_RTNL();
- /* Do we search for filter, attached to class? */
- if (TC_H_MIN(*parent)) {
- *cl = cops->find(*q, *parent);
- if (*cl == 0) {
- NL_SET_ERR_MSG(extack, "Specified class doesn't exist");
- err = -ENOENT;
- goto errout_qdisc;
- }
- }
+ err = __tcf_qdisc_find(net, q, parent, ifindex, true, extack);
+ if (err)
+ goto errout;
- /* And the last stroke */
- block = cops->tcf_block(*q, *cl, extack);
- if (!block) {
- err = -EINVAL;
- goto errout_qdisc;
- }
- if (tcf_block_shared(block)) {
- NL_SET_ERR_MSG(extack, "This filter block is shared. Please use the block index to manipulate the filters");
- err = -EOPNOTSUPP;
- goto errout_qdisc;
- }
+ err = __tcf_qdisc_cl_find(*q, *parent, cl, ifindex, extack);
+ if (err)
+ goto errout_qdisc;
- /* Always take reference to block in order to support execution
- * of rules update path of cls API without rtnl lock. Caller
- * must release block when it is finished using it. 'if' block
- * of this conditional obtain reference to block by calling
- * tcf_block_refcnt_get().
- */
- refcount_inc(&block->refcnt);
- }
+ block = __tcf_block_find(net, *q, *cl, ifindex, block_index, extack);
+ if (IS_ERR(block))
+ goto errout_qdisc;
return block;
-errout_rcu:
- rcu_read_unlock();
errout_qdisc:
- if (*q) {
+ if (*q)
qdisc_put(*q);
- *q = NULL;
- }
+errout:
+ *q = NULL;
return ERR_PTR(err);
}
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH net-next v2 17/17] net: sched: unlock rules update API
2018-12-11 10:11 [PATCH net-next v2 00/17] Refactor classifier API to work with chain/classifiers without rtnl lock Vlad Buslov
` (15 preceding siblings ...)
2018-12-11 10:11 ` [PATCH net-next v2 16/17] net: sched: refactor tcf_block_find() into standalone functions Vlad Buslov
@ 2018-12-11 10:12 ` Vlad Buslov
2018-12-20 0:15 ` kbuild test robot
16 siblings, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2018-12-11 10:12 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, ast, daniel, Vlad Buslov
Register netlink protocol handlers for message types RTM_NEWTFILTER,
RTM_DELTFILTER, RTM_GETTFILTER as unlocked. Set rtnl_held variable that
tracks rtnl mutex state to be false by default.
Introduce tcf_proto_is_unlocked() helper that is used to check
tcf_proto_ops->flag to determine if ops can be called without taking rtnl
lock. Manually lookup Qdisc, class and block in rule update handlers.
Verify that both Qdisc ops and proto ops are unlocked before using any of
their callbacks and obtain rtnl lock otherwise.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Refactor to only lock take rtnl lock once (at the beginning of rule
update handlers).
- Always release rtnl mutex in the same function that locked it.
Remove unlock code from tcf_block_release().
net/sched/cls_api.c | 142 ++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 120 insertions(+), 22 deletions(-)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 0a841f65e518..33992cc1326f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -180,6 +180,23 @@ static void tcf_proto_destroy_work(struct work_struct *work)
rtnl_unlock();
}
+static bool tcf_proto_is_unlocked(const char *kind)
+{
+ const struct tcf_proto_ops *ops;
+ bool ret;
+
+ ops = tcf_proto_lookup_ops(kind, false, NULL);
+ /* On error return false to take rtnl lock. Proto lookup/create
+ * functions will perform lookup again and properly handle errors.
+ */
+ if (IS_ERR(ops))
+ return false;
+
+ ret = !!(ops->flags & TCF_PROTO_OPS_DOIT_UNLOCKED);
+ module_put(ops->owner);
+ return ret;
+}
+
static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
u32 prio, struct tcf_chain *chain,
bool rtnl_held,
@@ -1310,13 +1327,18 @@ static struct tcf_block *tcf_block_find(struct net *net, struct Qdisc **q,
return ERR_PTR(err);
}
-static void tcf_block_release(struct Qdisc *q, struct tcf_block *block)
+static void tcf_block_release(struct Qdisc *q, struct tcf_block *block,
+ bool rtnl_held)
{
if (!IS_ERR_OR_NULL(block))
tcf_block_refcnt_put(block);
- if (q)
- qdisc_put(q);
+ if (q) {
+ if (rtnl_held)
+ qdisc_put(q);
+ else
+ qdisc_put_unlocked(q);
+ }
}
struct tcf_block_owner_item {
@@ -1992,7 +2014,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
void *fh;
int err;
int tp_created;
- bool rtnl_held = true;
+ bool rtnl_held = false;
if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
return -EPERM;
@@ -2011,6 +2033,7 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
parent = t->tcm_parent;
tp = NULL;
cl = 0;
+ block = NULL;
if (prio == 0) {
/* If no priority is provided by the user,
@@ -2027,8 +2050,27 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
/* Find head of filter chain. */
- block = tcf_block_find(net, &q, &parent, &cl,
- t->tcm_ifindex, t->tcm_block_index, extack);
+ err = __tcf_qdisc_find(net, &q, &parent, t->tcm_ifindex, false, extack);
+ if (err)
+ return err;
+
+ /* Take rtnl mutex if rtnl_held was set to true on previous iteration,
+ * block is shared (no qdisc found), qdisc is not unlocked, classifier
+ * type is not specified, classifier is not unlocked.
+ */
+ if (rtnl_held ||
+ (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) ||
+ !tca[TCA_KIND] || !tcf_proto_is_unlocked(nla_data(tca[TCA_KIND]))) {
+ rtnl_held = true;
+ rtnl_lock();
+ }
+
+ err = __tcf_qdisc_cl_find(q, parent, &cl, t->tcm_ifindex, extack);
+ if (err)
+ goto errout;
+
+ block = __tcf_block_find(net, q, cl, t->tcm_ifindex, t->tcm_block_index,
+ extack);
if (IS_ERR(block)) {
err = PTR_ERR(block);
goto errout;
@@ -2147,10 +2189,19 @@ static int tc_new_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
if (!tp_created)
tcf_chain_put(chain);
}
- tcf_block_release(q, block);
- if (err == -EAGAIN)
+ tcf_block_release(q, block, rtnl_held);
+
+ if (rtnl_held)
+ rtnl_unlock();
+
+ if (err == -EAGAIN) {
+ /* Take rtnl lock in case EAGAIN is caused by concurrent flush
+ * of target chain.
+ */
+ rtnl_held = true;
/* Replay the request. */
goto replay;
+ }
return err;
errout_locked:
@@ -2171,12 +2222,12 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
struct Qdisc *q = NULL;
struct tcf_chain_info chain_info;
struct tcf_chain *chain = NULL;
- struct tcf_block *block;
+ struct tcf_block *block = NULL;
struct tcf_proto *tp = NULL;
unsigned long cl = 0;
void *fh = NULL;
int err;
- bool rtnl_held = true;
+ bool rtnl_held = false;
if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN))
return -EPERM;
@@ -2197,8 +2248,27 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
/* Find head of filter chain. */
- block = tcf_block_find(net, &q, &parent, &cl,
- t->tcm_ifindex, t->tcm_block_index, extack);
+ err = __tcf_qdisc_find(net, &q, &parent, t->tcm_ifindex, false, extack);
+ if (err)
+ return err;
+
+ /* Take rtnl mutex if flushing whole chain, block is shared (no qdisc
+ * found), qdisc is not unlocked, classifier type is not specified,
+ * classifier is not unlocked.
+ */
+ if (!prio ||
+ (q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) ||
+ !tca[TCA_KIND] || !tcf_proto_is_unlocked(nla_data(tca[TCA_KIND]))) {
+ rtnl_held = true;
+ rtnl_lock();
+ }
+
+ err = __tcf_qdisc_cl_find(q, parent, &cl, t->tcm_ifindex, extack);
+ if (err)
+ goto errout;
+
+ block = __tcf_block_find(net, q, cl, t->tcm_ifindex, t->tcm_block_index,
+ extack);
if (IS_ERR(block)) {
err = PTR_ERR(block);
goto errout;
@@ -2279,7 +2349,11 @@ static int tc_del_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tcf_proto_put(tp, NULL);
tcf_chain_put(chain);
}
- tcf_block_release(q, block);
+ tcf_block_release(q, block, rtnl_held);
+
+ if (rtnl_held)
+ rtnl_unlock();
+
return err;
errout_locked:
@@ -2300,12 +2374,12 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
struct Qdisc *q = NULL;
struct tcf_chain_info chain_info;
struct tcf_chain *chain = NULL;
- struct tcf_block *block;
+ struct tcf_block *block = NULL;
struct tcf_proto *tp = NULL;
unsigned long cl = 0;
void *fh = NULL;
int err;
- bool rtnl_held = true;
+ bool rtnl_held = false;
err = nlmsg_parse(n, sizeof(*t), tca, TCA_MAX, rtm_tca_policy, extack);
if (err < 0)
@@ -2323,8 +2397,26 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
/* Find head of filter chain. */
- block = tcf_block_find(net, &q, &parent, &cl,
- t->tcm_ifindex, t->tcm_block_index, extack);
+ err = __tcf_qdisc_find(net, &q, &parent, t->tcm_ifindex, false, extack);
+ if (err)
+ return err;
+
+ /* Take rtnl mutex if block is shared (no qdisc found), qdisc is not
+ * unlocked, classifier type is not specified, classifier is not
+ * unlocked.
+ */
+ if ((q && !(q->ops->cl_ops->flags & QDISC_CLASS_OPS_DOIT_UNLOCKED)) ||
+ !tca[TCA_KIND] || !tcf_proto_is_unlocked(nla_data(tca[TCA_KIND]))) {
+ rtnl_held = true;
+ rtnl_lock();
+ }
+
+ err = __tcf_qdisc_cl_find(q, parent, &cl, t->tcm_ifindex, extack);
+ if (err)
+ goto errout;
+
+ block = __tcf_block_find(net, q, cl, t->tcm_ifindex, t->tcm_block_index,
+ extack);
if (IS_ERR(block)) {
err = PTR_ERR(block);
goto errout;
@@ -2376,7 +2468,11 @@ static int tc_get_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
tcf_proto_put(tp, NULL);
tcf_chain_put(chain);
}
- tcf_block_release(q, block);
+ tcf_block_release(q, block, rtnl_held);
+
+ if (rtnl_held)
+ rtnl_unlock();
+
return err;
}
@@ -2823,7 +2919,7 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n,
errout:
tcf_chain_put(chain);
errout_block:
- tcf_block_release(q, block);
+ tcf_block_release(q, block, true);
if (err == -EAGAIN)
/* Replay the request. */
goto replay;
@@ -3166,10 +3262,12 @@ static int __init tc_filter_init(void)
if (err)
goto err_rhash_setup_block_ht;
- rtnl_register(PF_UNSPEC, RTM_NEWTFILTER, tc_new_tfilter, NULL, 0);
- rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_del_tfilter, NULL, 0);
+ rtnl_register(PF_UNSPEC, RTM_NEWTFILTER, tc_new_tfilter, NULL,
+ RTNL_FLAG_DOIT_UNLOCKED);
+ rtnl_register(PF_UNSPEC, RTM_DELTFILTER, tc_del_tfilter, NULL,
+ RTNL_FLAG_DOIT_UNLOCKED);
rtnl_register(PF_UNSPEC, RTM_GETTFILTER, tc_get_tfilter,
- tc_dump_tfilter, 0);
+ tc_dump_tfilter, RTNL_FLAG_DOIT_UNLOCKED);
rtnl_register(PF_UNSPEC, RTM_NEWCHAIN, tc_ctl_chain, NULL, 0);
rtnl_register(PF_UNSPEC, RTM_DELCHAIN, tc_ctl_chain, NULL, 0);
rtnl_register(PF_UNSPEC, RTM_GETCHAIN, tc_ctl_chain,
--
2.7.5
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH net-next v2 17/17] net: sched: unlock rules update API
2018-12-11 10:12 ` [PATCH net-next v2 17/17] net: sched: unlock rules update API Vlad Buslov
@ 2018-12-20 0:15 ` kbuild test robot
0 siblings, 0 replies; 30+ messages in thread
From: kbuild test robot @ 2018-12-20 0:15 UTC (permalink / raw)
To: Vlad Buslov
Cc: kbuild-all, netdev, jhs, xiyou.wangcong, jiri, davem, ast, daniel,
Vlad Buslov
[-- Attachment #1: Type: text/plain, Size: 4807 bytes --]
Hi Vlad,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Vlad-Buslov/Refactor-classifier-API-to-work-with-chain-classifiers-without-rtnl-lock/20181211-231158
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
net/sched/sch_generic.c:563:34: warning: incorrect type in initializer (different address spaces)
net/sched/sch_generic.c:563:34: expected struct Qdisc [noderef] <asn:4> *qdisc
net/sched/sch_generic.c:563:34: got struct Qdisc *<noident>
include/linux/slab.h:332:43: warning: dubious: x & !y
include/linux/slab.h:332:43: warning: dubious: x & !y
include/linux/slab.h:332:43: warning: dubious: x & !y
>> net/sched/sch_generic.c:1376:6: warning: symbol '__mini_qdisc_pair_swap' was not declared. Should it be static?
>> net/sched/sch_generic.c:1412:6: warning: symbol 'mini_qdisc_pair_swap_work' was not declared. Should it be static?
net/sched/sch_generic.c:207:50: warning: context imbalance in 'try_bulk_dequeue_skb_slow' - different lock contexts for basic block
net/sched/sch_generic.c:266:17: warning: context imbalance in 'dequeue_skb' - different lock contexts for basic block
net/sched/sch_generic.c:309:28: warning: context imbalance in 'sch_direct_xmit' - unexpected unlock
net/sched/sch_generic.c:1151:9: warning: context imbalance in 'dev_deactivate_queue' - different lock contexts for basic block
--
include/linux/slab.h:332:43: warning: dubious: x & !y
net/sched/cls_api.c:218:22: warning: incorrect type in assignment (different base types)
net/sched/cls_api.c:218:22: expected restricted __be16 [usertype] protocol
net/sched/cls_api.c:218:22: got unsigned int [unsigned] [usertype] protocol
include/linux/slab.h:332:43: warning: dubious: x & !y
>> net/sched/cls_api.c:549:20: warning: incorrect type in assignment (different address spaces)
net/sched/cls_api.c:549:20: expected struct tcf_proto *[assigned] tp
net/sched/cls_api.c:549:20: got struct tcf_proto [noderef] <asn:4> *next
include/linux/slab.h:332:43: warning: dubious: x & !y
include/linux/slab.h:332:43: warning: dubious: x & !y
include/linux/slab.h:332:43: warning: dubious: x & !y
include/linux/slab.h:332:43: warning: dubious: x & !y
include/linux/slab.h:332:43: warning: dubious: x & !y
include/linux/slab.h:332:43: warning: dubious: x & !y
net/sched/cls_api.c:1752:16: error: incompatible types in comparison expression (different address spaces)
net/sched/cls_api.c:1844:20: error: incompatible types in comparison expression (different address spaces)
net/sched/cls_api.c:1807:25: error: incompatible types in comparison expression (different address spaces)
net/sched/cls_api.c:1826:16: error: incompatible types in comparison expression (different address spaces)
net/sched/cls_api.c:1891:25: warning: restricted __be16 degrades to integer
net/sched/cls_api.c:2521:50: warning: restricted __be16 degrades to integer
>> net/sched/cls_api.c:1271:45: warning: context imbalance in '__tcf_block_put' - unexpected unlock
vim +549 net/sched/cls_api.c
32a4f5ec Jiri Pirko 2018-07-23 535
290b1c8b Jiri Pirko 2018-08-01 536 static void tcf_chain_flush(struct tcf_chain *chain)
290b1c8b Jiri Pirko 2018-08-01 537 {
11ccabb2 Vlad Buslov 2018-12-11 538 struct tcf_proto *tp;
290b1c8b Jiri Pirko 2018-08-01 539
11ccabb2 Vlad Buslov 2018-12-11 540 spin_lock(&chain->filter_chain_lock);
11ccabb2 Vlad Buslov 2018-12-11 541 tp = tcf_chain_dereference(chain->filter_chain, chain);
eb733349 Vlad Buslov 2018-12-11 542 RCU_INIT_POINTER(chain->filter_chain, NULL);
290b1c8b Jiri Pirko 2018-08-01 543 tcf_chain0_head_change(chain, NULL);
dc5255f2 Vlad Buslov 2018-12-11 544 chain->flushing = true;
11ccabb2 Vlad Buslov 2018-12-11 545 spin_unlock(&chain->filter_chain_lock);
11ccabb2 Vlad Buslov 2018-12-11 546
290b1c8b Jiri Pirko 2018-08-01 547 while (tp) {
eb733349 Vlad Buslov 2018-12-11 548 tcf_proto_put(tp, NULL);
eb733349 Vlad Buslov 2018-12-11 @549 tp = tp->next;
290b1c8b Jiri Pirko 2018-08-01 550 }
290b1c8b Jiri Pirko 2018-08-01 551 }
290b1c8b Jiri Pirko 2018-08-01 552
:::::: The code at line 549 was first introduced by commit
:::::: eb733349a2b09f803d821c70862067ca1a632bbb net: sched: introduce reference counting for tcf proto
:::::: TO: Vlad Buslov <vladbu@mellanox.com>
:::::: CC: 0day robot <lkp@intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66632 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread