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
Acked-by: Jiri Pirko
---
include/net/sch_generic.h | 4 ++
net/sched/cls_api.c | 93 +++
2 files changed, 81 insertions(+), 16 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 29118bbd528f..5f4fc28fc77a 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -350,6 +350,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 17c1691bf0c0..df3326dd33ef 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -216,6 +216,10 @@ static void tcf_proto_destroy(struct tcf_proto *tp,
tc_queue_proto_work(>work);
}
+#define ASSERT_BLOCK_LOCKED(block) \
+ WARN_ONCE(!spin_is_locked(&(block)->lock), \
+ "BLOCK: assertion failed at %s (%d)\n", __FILE__, __LINE__)
+
struct tcf_filter_chain_list_item {
struct list_head list;
tcf_chain_head_change_t *chain_head_change;
@@ -227,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(>list, >chain_list);
@@ -258,13 +264,29 @@ 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(>list);
if (!chain->index)
block->chain0.chain = NULL;
+
+ if (list_empty(>chain_list) &&
+ refcount_read(>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(>chain_list) && !refcount_read(>refcnt))
kfree_rcu(block, rcu);
@@ -272,11 +294,15 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
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.
*/
@@ -288,6 +314,8 @@ static struct tcf_chain
