* [PATCH net-next v6 01/11] net: sched: use rcu for action cookie update
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Implement functions to atomically update and free action cookie
using rcu mechanism.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/act_api.h | 2 +-
include/net/pkt_cls.h | 1 +
net/sched/act_api.c | 44 ++++++++++++++++++++++++++++++--------------
3 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 5ff11adbe2a6..ffc3ef321776 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -37,7 +37,7 @@ struct tc_action {
spinlock_t tcfa_lock;
struct gnet_stats_basic_cpu __percpu *cpu_bstats;
struct gnet_stats_queue __percpu *cpu_qstats;
- struct tc_cookie *act_cookie;
+ struct tc_cookie __rcu *act_cookie;
struct tcf_chain *goto_chain;
};
#define tcf_index common.tcfa_index
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 6641584b27f1..2081e4219f81 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -781,6 +781,7 @@ struct tc_mqprio_qopt_offload {
struct tc_cookie {
u8 *data;
u32 len;
+ struct rcu_head rcu;
};
struct tc_qopt_offload_stats {
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 3f4cf930f809..02670c7489e3 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -55,6 +55,24 @@ static void tcf_action_goto_chain_exec(const struct tc_action *a,
res->goto_tp = rcu_dereference_bh(chain->filter_chain);
}
+static void tcf_free_cookie_rcu(struct rcu_head *p)
+{
+ struct tc_cookie *cookie = container_of(p, struct tc_cookie, rcu);
+
+ kfree(cookie->data);
+ kfree(cookie);
+}
+
+static void tcf_set_action_cookie(struct tc_cookie __rcu **old_cookie,
+ struct tc_cookie *new_cookie)
+{
+ struct tc_cookie *old;
+
+ old = xchg(old_cookie, new_cookie);
+ if (old)
+ call_rcu(&old->rcu, tcf_free_cookie_rcu);
+}
+
/* XXX: For standalone actions, we don't need a RCU grace period either, because
* actions are always connected to filters and filters are already destroyed in
* RCU callbacks, so after a RCU grace period actions are already disconnected
@@ -65,10 +83,7 @@ static void free_tcf(struct tc_action *p)
free_percpu(p->cpu_bstats);
free_percpu(p->cpu_qstats);
- if (p->act_cookie) {
- kfree(p->act_cookie->data);
- kfree(p->act_cookie);
- }
+ tcf_set_action_cookie(&p->act_cookie, NULL);
if (p->goto_chain)
tcf_action_goto_chain_fini(p);
@@ -567,16 +582,22 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
int err = -EINVAL;
unsigned char *b = skb_tail_pointer(skb);
struct nlattr *nest;
+ struct tc_cookie *cookie;
if (nla_put_string(skb, TCA_KIND, a->ops->kind))
goto nla_put_failure;
if (tcf_action_copy_stats(skb, a, 0))
goto nla_put_failure;
- if (a->act_cookie) {
- if (nla_put(skb, TCA_ACT_COOKIE, a->act_cookie->len,
- a->act_cookie->data))
+
+ rcu_read_lock();
+ cookie = rcu_dereference(a->act_cookie);
+ if (cookie) {
+ if (nla_put(skb, TCA_ACT_COOKIE, cookie->len, cookie->data)) {
+ rcu_read_unlock();
goto nla_put_failure;
+ }
}
+ rcu_read_unlock();
nest = nla_nest_start(skb, TCA_OPTIONS);
if (nest == NULL)
@@ -719,13 +740,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
if (err < 0)
goto err_mod;
- if (name == NULL && tb[TCA_ACT_COOKIE]) {
- if (a->act_cookie) {
- kfree(a->act_cookie->data);
- kfree(a->act_cookie);
- }
- a->act_cookie = cookie;
- }
+ if (!name && tb[TCA_ACT_COOKIE])
+ tcf_set_action_cookie(&a->act_cookie, cookie);
/* module count goes up only when brand new policy is created
* if it exists and is only bound to in a_o->init() then
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 00/11] Modify action API for implementing lockless actions
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov
Currently, all netlink protocol handlers for updating rules, actions and
qdiscs are protected with single global rtnl lock which removes any
possibility for parallelism. This patch set is a first step to remove
rtnl lock dependency from TC rules update path.
Recently, new rtnl registration flag RTNL_FLAG_DOIT_UNLOCKED was added.
Handlers registered with this flag are called without RTNL taken. End
goal is to have rule update handlers(RTM_NEWTFILTER, RTM_DELTFILTER,
etc.) to be registered with UNLOCKED flag to allow parallel execution.
However, there is no intention to completely remove or split rtnl lock
itself. This patch set addresses specific problems in action API that
prevents it from being executed concurrently. This patch set does not
completely unlock rules or actions update path. Additional patch sets
are required to refactor individual actions and filters update for
parallel execution.
As a preparation for executing TC rules update handlers without rtnl
lock, action API code was audited to determine areas that assume
external synchronization with rtnl lock and must be changed to allow
safe concurrent access with following results:
1. Action idr is already protected with spinlock. However, some code
paths assume that idr state is not changes between several
consecutive tcf_idr_* function calls.
2. tc_action reference and bind counters are implemented as plain
integers. They purpose was to allow single actions to be shared
between multiple filters, not to provide means for concurrent
modification.
3. tc_action 'cookie' pointer field is not protected against
modification.
4. Action API functions, that work with set of actions, use intrusive
linked list, which cannot be used concurrently without additional
synchronization.
5. Action API functions don't take reference to actions while using
them, assuming external synchronization with rtnl lock.
Following solutions to these problems are implemented:
1. To remove assumption that idr state doesn't change between tcf_idr_*
calls, implement new functions that atomically perform several
operations on idr without releasing idr spinlock. (function to
atomically lookup and delete action by index, function to atomically
check if action exists and allocate new one if necessary, etc.)
2. Use atomic operations on counters to make them suitable for
concurrent get/put operations.
3. Data that 'cookie' points to is never modified, so it enough to
refactor it to rcu pointer to prevent concurrent de-allocation.
4. Action API doesn't actually use any linked list specific operations
on actions intrusive linked list, so it can be refactored to array in
straightforward manner.
5. Always take reference to action while accessing it in action API.
tcf_idr_search function modified to take reference to action before
returning it, so there is no way to lookup an action without
incrementing its reference counter. All users of this function are
modified to release the reference, after they done using action. With
all users using reference counting, it is now safe to concurrently
delete actions.
Additionally, actions init function signature was expanded with
'rtnl_held' argument, that allows actions that have internal dependency
on rtnl lock to take/release it when necessary.
Since only shared state in action API module are actions themselves and
action idr, these changes are sufficient to not to rely on global rtnl
lock for protection of internal action API data structures.
Changes from V5 to V6:
- Rebase on current net-next
- When action is deleted, set pointer in actions array to NULL to
prevent double freeing.
Changes from V4 to V5:
- Change action delete API to track actions that were deleted, to
prevent releasing them on error.
Changes from V3 to V4:
- Expand cover letter.
- Reduce actions array size in tcf_action_init_1.
- Rebase on latest net-next.
Changes from V2 to V3:
- Re-send with changelog copied to individual patches.
Changes from V1 to V2:
- Removed redundant actions ops lookup during delete.
- Merge action ops delete definition and implementation.
- Assume all actions have delete implemented and don't check for it
explicitly.
- Resplit action lookup/release code to prevent memory leaks in
individual patches.
- Make __tcf_idr_check function static
- Remove unique idr insertion function. Change original idr insert to do
the same thing.
- Merge changes that take reference to action when performing lookup and
changes that account for this additional reference when dumping action
to user space into single patch.
- Change convoluted commit message.
- Rename "unlocked" to "rtnl_held" for clarity.
- Remove estimator lock add patch.
- Refactor action check-alloc code into standalone function.
- Rename tcf_idr_find_delete to tcf_idr_delete_index.
- Rearrange variable definitions in tc_action_delete.
- Add patch that refactors action API code to use array of pointers to
actions instead of intrusive linked list.
- Expand cover letter.
Vlad Buslov (11):
net: sched: use rcu for action cookie update
net: sched: change type of reference and bind counters
net: sched: implement unlocked action init API
net: sched: always take reference to action
net: sched: implement action API that deletes action by index
net: sched: add 'delete' function to action ops
net: sched: implement reference counted action release
net: sched: don't release reference on action overwrite
net: sched: use reference counting action init
net: sched: atomically check-allocate action
net: sched: change action API to use array of pointers to actions
include/net/act_api.h | 25 ++-
include/net/pkt_cls.h | 1 +
net/sched/act_api.c | 415 +++++++++++++++++++++++++++++++--------------
net/sched/act_bpf.c | 34 ++--
net/sched/act_connmark.c | 29 +++-
net/sched/act_csum.c | 34 ++--
net/sched/act_gact.c | 31 +++-
net/sched/act_ife.c | 31 +++-
net/sched/act_ipt.c | 44 ++++-
net/sched/act_mirred.c | 38 +++--
net/sched/act_nat.c | 30 +++-
net/sched/act_pedit.c | 29 +++-
net/sched/act_police.c | 31 ++--
net/sched/act_sample.c | 34 ++--
net/sched/act_simple.c | 31 +++-
net/sched/act_skbedit.c | 31 +++-
net/sched/act_skbmod.c | 34 ++--
net/sched/act_tunnel_key.c | 35 ++--
net/sched/act_vlan.c | 40 +++--
net/sched/cls_api.c | 25 +--
20 files changed, 707 insertions(+), 295 deletions(-)
--
2.7.5
^ permalink raw reply
* [PATCH net-next v6 04/11] net: sched: always take reference to action
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Without rtnl lock protection it is no longer safe to use pointer to tc
action without holding reference to it. (it can be destroyed concurrently)
Remove unsafe action idr lookup function. Instead of it, implement safe tcf
idr check function that atomically looks up action in idr and increments
its reference and bind counters. Implement both action search and check
using new safe function
Reference taken by idr check is temporal and should not be accounted by
userspace clients (both logically and to preserver current API behavior).
Subtract temporal reference when dumping action to userspace using existing
tca_get_fill function arguments.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Make __tcf_idr_check function static
- Merge changes that take reference to action when performing lookup and
changes that account for this additional reference when dumping action
to user space into single patch.
net/sched/act_api.c | 46 ++++++++++++++++++++--------------------------
1 file changed, 20 insertions(+), 26 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 256b0c93916c..aa304d36fee0 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -284,44 +284,38 @@ int tcf_generic_walker(struct tc_action_net *tn, struct sk_buff *skb,
}
EXPORT_SYMBOL(tcf_generic_walker);
-static struct tc_action *tcf_idr_lookup(u32 index, struct tcf_idrinfo *idrinfo)
+static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
+ struct tc_action **a, int bind)
{
- struct tc_action *p = NULL;
+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
+ struct tc_action *p;
spin_lock(&idrinfo->lock);
p = idr_find(&idrinfo->action_idr, index);
+ if (p) {
+ refcount_inc(&p->tcfa_refcnt);
+ if (bind)
+ atomic_inc(&p->tcfa_bindcnt);
+ }
spin_unlock(&idrinfo->lock);
- return p;
+ if (p) {
+ *a = p;
+ return true;
+ }
+ return false;
}
int tcf_idr_search(struct tc_action_net *tn, struct tc_action **a, u32 index)
{
- struct tcf_idrinfo *idrinfo = tn->idrinfo;
- struct tc_action *p = tcf_idr_lookup(index, idrinfo);
-
- if (p) {
- *a = p;
- return 1;
- }
- return 0;
+ return __tcf_idr_check(tn, index, a, 0);
}
EXPORT_SYMBOL(tcf_idr_search);
bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
int bind)
{
- struct tcf_idrinfo *idrinfo = tn->idrinfo;
- struct tc_action *p = tcf_idr_lookup(index, idrinfo);
-
- if (index && p) {
- if (bind)
- atomic_inc(&p->tcfa_bindcnt);
- refcount_inc(&p->tcfa_refcnt);
- *a = p;
- return true;
- }
- return false;
+ return __tcf_idr_check(tn, index, a, bind);
}
EXPORT_SYMBOL(tcf_idr_check);
@@ -932,7 +926,7 @@ tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
if (!skb)
return -ENOBUFS;
if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
- 0, 0) <= 0) {
+ 0, 1) <= 0) {
NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action");
kfree_skb(skb);
return -EINVAL;
@@ -1072,7 +1066,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
return -ENOBUFS;
if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
- 0, 1) <= 0) {
+ 0, 2) <= 0) {
NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action attributes");
kfree_skb(skb);
return -EINVAL;
@@ -1131,14 +1125,14 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
if (event == RTM_GETACTION)
ret = tcf_get_notify(net, portid, n, &actions, event, extack);
else { /* delete */
+ cleanup_a(&actions, 1); /* lookup took reference */
ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
if (ret)
goto err;
return ret;
}
err:
- if (event != RTM_GETACTION)
- tcf_action_destroy(&actions, 0);
+ tcf_action_destroy(&actions, 0);
return ret;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 05/11] net: sched: implement action API that deletes action by index
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Implement new action API function that atomically finds and deletes action
from idr by index. Intended to be used by lockless actions that do not rely
on rtnl lock.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Rename tcf_idr_find_delete to tcf_idr_delete_index.
include/net/act_api.h | 1 +
net/sched/act_api.c | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 27823f4e24c4..a8eaae67c264 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -153,6 +153,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
int bind, bool cpustats);
void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
+int tcf_idr_delete_index(struct tc_action_net *tn, u32 index);
int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
static inline int tcf_idr_release(struct tc_action *a, bool bind)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index aa304d36fee0..0f31f09946ab 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -319,6 +319,45 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
}
EXPORT_SYMBOL(tcf_idr_check);
+int tcf_idr_delete_index(struct tc_action_net *tn, u32 index)
+{
+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
+ struct tc_action *p;
+ int ret = 0;
+
+ spin_lock(&idrinfo->lock);
+ p = idr_find(&idrinfo->action_idr, index);
+ if (!p) {
+ spin_unlock(&idrinfo->lock);
+ return -ENOENT;
+ }
+
+ if (!atomic_read(&p->tcfa_bindcnt)) {
+ if (refcount_dec_and_test(&p->tcfa_refcnt)) {
+ struct module *owner = p->ops->owner;
+
+ WARN_ON(p != idr_remove(&idrinfo->action_idr,
+ p->tcfa_index));
+ spin_unlock(&idrinfo->lock);
+
+ if (p->ops->cleanup)
+ p->ops->cleanup(p);
+
+ gen_kill_estimator(&p->tcfa_rate_est);
+ free_tcf(p);
+ module_put(owner);
+ return 0;
+ }
+ ret = 0;
+ } else {
+ ret = -EPERM;
+ }
+
+ spin_unlock(&idrinfo->lock);
+ return ret;
+}
+EXPORT_SYMBOL(tcf_idr_delete_index);
+
int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
struct tc_action **a, const struct tc_action_ops *ops,
int bind, bool cpustats)
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 03/11] net: sched: implement unlocked action init API
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Add additional 'rtnl_held' argument to act API init functions. It is
required to implement actions that need to release rtnl lock before loading
kernel module and reacquire if afterwards.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Rename "unlocked" to "rtnl_held" for clarity.
include/net/act_api.h | 6 ++++--
net/sched/act_api.c | 18 +++++++++++-------
net/sched/act_bpf.c | 3 ++-
net/sched/act_connmark.c | 2 +-
net/sched/act_csum.c | 3 ++-
net/sched/act_gact.c | 3 ++-
net/sched/act_ife.c | 3 ++-
net/sched/act_ipt.c | 6 ++++--
net/sched/act_mirred.c | 5 +++--
net/sched/act_nat.c | 2 +-
net/sched/act_pedit.c | 3 ++-
net/sched/act_police.c | 2 +-
net/sched/act_sample.c | 3 ++-
net/sched/act_simple.c | 3 ++-
net/sched/act_skbedit.c | 3 ++-
net/sched/act_skbmod.c | 3 ++-
net/sched/act_tunnel_key.c | 3 ++-
net/sched/act_vlan.c | 3 ++-
net/sched/cls_api.c | 5 +++--
19 files changed, 50 insertions(+), 29 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 2759226527a2..27823f4e24c4 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -92,7 +92,8 @@ struct tc_action_ops {
struct netlink_ext_ack *extack);
int (*init)(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **act, int ovr,
- int bind, struct netlink_ext_ack *extack);
+ int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack);
int (*walk)(struct net *, struct sk_buff *,
struct netlink_callback *, int,
const struct tc_action_ops *,
@@ -168,10 +169,11 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
struct nlattr *est, char *name, int ovr, int bind,
struct list_head *actions, size_t *attr_size,
- struct netlink_ext_ack *extack);
+ bool rtnl_held, struct netlink_ext_ack *extack);
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
+ bool rtnl_held,
struct netlink_ext_ack *extack);
int tcf_action_dump(struct sk_buff *skb, struct list_head *, int, int);
int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 4f064ecab882..256b0c93916c 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -671,6 +671,7 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
+ bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tc_action *a;
@@ -721,9 +722,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
a_o = tc_lookup_action_n(act_name);
if (a_o == NULL) {
#ifdef CONFIG_MODULES
- rtnl_unlock();
+ if (rtnl_held)
+ rtnl_unlock();
request_module("act_%s", act_name);
- rtnl_lock();
+ if (rtnl_held)
+ rtnl_lock();
a_o = tc_lookup_action_n(act_name);
@@ -746,9 +749,10 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
/* backward compatibility for policer */
if (name == NULL)
err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, ovr, bind,
- extack);
+ rtnl_held, extack);
else
- err = a_o->init(net, nla, est, &a, ovr, bind, extack);
+ err = a_o->init(net, nla, est, &a, ovr, bind, rtnl_held,
+ extack);
if (err < 0)
goto err_mod;
@@ -800,7 +804,7 @@ static void cleanup_a(struct list_head *actions, int ovr)
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
struct nlattr *est, char *name, int ovr, int bind,
struct list_head *actions, size_t *attr_size,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
struct tc_action *act;
@@ -814,7 +818,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
act = tcf_action_init_1(net, tp, tb[i], est, name, ovr, bind,
- extack);
+ rtnl_held, extack);
if (IS_ERR(act)) {
err = PTR_ERR(act);
goto err;
@@ -1173,7 +1177,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
LIST_HEAD(actions);
ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0, &actions,
- &attr_size, extack);
+ &attr_size, true, extack);
if (ret)
return ret;
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 15a2a53cbde1..8ebf40a3506c 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -276,7 +276,8 @@ static void tcf_bpf_prog_fill_cfg(const struct tcf_bpf *prog,
static int tcf_bpf_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **act,
- int replace, int bind, struct netlink_ext_ack *extack)
+ int replace, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, bpf_net_id);
struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 188865034f9a..e3787aa0025a 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -96,7 +96,7 @@ static const struct nla_policy connmark_policy[TCA_CONNMARK_MAX + 1] = {
static int tcf_connmark_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind,
+ int ovr, int bind, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, connmark_net_id);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index da865f7b390a..334261943f9f 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -46,7 +46,8 @@ static struct tc_action_ops act_csum_ops;
static int tcf_csum_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, struct netlink_ext_ack *extack)
+ int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, csum_net_id);
struct tcf_csum_params *params_old, *params_new;
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index ca83debd5a70..b4dfb2b4addc 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -56,7 +56,8 @@ static const struct nla_policy gact_policy[TCA_GACT_MAX + 1] = {
static int tcf_gact_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, gact_net_id);
struct nlattr *tb[TCA_GACT_MAX + 1];
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 3536a23f46b5..576ffbba61c3 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -448,7 +448,8 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
static int tcf_ife_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, ife_net_id);
struct nlattr *tb[TCA_IFE_MAX + 1];
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 7bce88dc11c9..9c21663a86a6 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -196,7 +196,8 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
static int tcf_ipt_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, struct netlink_ext_ack *extack)
+ int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
return __tcf_ipt_init(net, ipt_net_id, nla, est, a, &act_ipt_ops, ovr,
bind);
@@ -204,7 +205,8 @@ static int tcf_ipt_init(struct net *net, struct nlattr *nla,
static int tcf_xt_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, struct netlink_ext_ack *extack)
+ int bind, bool unlocked,
+ struct netlink_ext_ack *extack)
{
return __tcf_ipt_init(net, xt_net_id, nla, est, a, &act_xt_ops, ovr,
bind);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 82a8bdd67c47..5434f08f2eb7 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -68,8 +68,9 @@ static unsigned int mirred_net_id;
static struct tc_action_ops act_mirred_ops;
static int tcf_mirred_init(struct net *net, struct nlattr *nla,
- struct nlattr *est, struct tc_action **a, int ovr,
- int bind, struct netlink_ext_ack *extack)
+ struct nlattr *est, struct tc_action **a,
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, mirred_net_id);
struct nlattr *tb[TCA_MIRRED_MAX + 1];
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 457c2ae3de46..e6487ad1e4a8 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -38,7 +38,7 @@ static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
struct tc_action **a, int ovr, int bind,
- struct netlink_ext_ack *extack)
+ bool rtnl_held, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, nat_net_id);
struct nlattr *tb[TCA_NAT_MAX + 1];
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 889690e0ec39..f7965f35585b 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -132,7 +132,8 @@ static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
static int tcf_pedit_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, pedit_net_id);
struct nlattr *tb[TCA_PEDIT_MAX + 1];
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index a789b8060968..0e1c2fb0ebea 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -75,7 +75,7 @@ static const struct nla_policy police_policy[TCA_POLICE_MAX + 1] = {
static int tcf_act_police_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind,
+ int ovr, int bind, bool rtnl_held,
struct netlink_ext_ack *extack)
{
int ret = 0, err;
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 4a46978db092..316fc645595d 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -37,7 +37,8 @@ static const struct nla_policy sample_policy[TCA_SAMPLE_MAX + 1] = {
static int tcf_sample_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, struct netlink_ext_ack *extack)
+ int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, sample_net_id);
struct nlattr *tb[TCA_SAMPLE_MAX + 1];
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index c3a761097b01..dc591cc87f4a 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -79,7 +79,8 @@ static const struct nla_policy simple_policy[TCA_DEF_MAX + 1] = {
static int tcf_simp_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, simp_net_id);
struct nlattr *tb[TCA_DEF_MAX + 1];
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index cfd20d3d2ca9..c4ae4bd830aa 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -94,7 +94,8 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, skbedit_net_id);
struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ff90d720eda3..026d6f58eda1 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -84,7 +84,8 @@ static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {
static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, skbmod_net_id);
struct nlattr *tb[TCA_SKBMOD_MAX + 1];
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 2354f07eba15..15ea5ce0f9ed 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -201,7 +201,8 @@ static const struct nla_policy tunnel_key_policy[TCA_TUNNEL_KEY_MAX + 1] = {
static int tunnel_key_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
struct nlattr *tb[TCA_TUNNEL_KEY_MAX + 1];
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 799e3deb44ac..c61775250722 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -109,7 +109,8 @@ static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
static int tcf_vlan_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- int ovr, int bind, struct netlink_ext_ack *extack)
+ int ovr, int bind, bool rtnl_held,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, vlan_net_id);
struct nlattr *tb[TCA_VLAN_MAX + 1];
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index bbf8dda96b0e..ebc2b9dd783f 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1632,7 +1632,7 @@ 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, extack);
+ TCA_ACT_BIND, true, extack);
if (IS_ERR(act))
return PTR_ERR(act);
@@ -1645,7 +1645,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,
- &actions, &attr_size, extack);
+ &actions, &attr_size, true,
+ extack);
if (err)
return err;
list_for_each_entry(act, &actions, list)
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 02/11] net: sched: change type of reference and bind counters
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Change type of action reference counter to refcount_t.
Change type of action bind counter to atomic_t.
This type is used to allow decrementing bind counter without testing
for 0 result.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
include/net/act_api.h | 5 +++--
net/sched/act_api.c | 32 ++++++++++++++++++++++----------
net/sched/act_bpf.c | 4 ++--
net/sched/act_connmark.c | 4 ++--
net/sched/act_csum.c | 4 ++--
net/sched/act_gact.c | 4 ++--
net/sched/act_ife.c | 4 ++--
net/sched/act_ipt.c | 4 ++--
net/sched/act_mirred.c | 4 ++--
net/sched/act_nat.c | 4 ++--
net/sched/act_pedit.c | 4 ++--
net/sched/act_police.c | 4 ++--
net/sched/act_sample.c | 4 ++--
net/sched/act_simple.c | 4 ++--
net/sched/act_skbedit.c | 4 ++--
net/sched/act_skbmod.c | 4 ++--
net/sched/act_tunnel_key.c | 4 ++--
net/sched/act_vlan.c | 4 ++--
18 files changed, 57 insertions(+), 44 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index ffc3ef321776..2759226527a2 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -6,6 +6,7 @@
* Public action API for classifiers/qdiscs
*/
+#include <linux/refcount.h>
#include <net/sch_generic.h>
#include <net/pkt_sched.h>
#include <net/net_namespace.h>
@@ -26,8 +27,8 @@ struct tc_action {
struct tcf_idrinfo *idrinfo;
u32 tcfa_index;
- int tcfa_refcnt;
- int tcfa_bindcnt;
+ refcount_t tcfa_refcnt;
+ atomic_t tcfa_bindcnt;
u32 tcfa_capab;
int tcfa_action;
struct tcf_t tcfa_tm;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 02670c7489e3..4f064ecab882 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -105,14 +105,26 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
ASSERT_RTNL();
+ /* Release with strict==1 and bind==0 is only called through act API
+ * interface (classifiers always bind). Only case when action with
+ * positive reference count and zero bind count can exist is when it was
+ * also created with act API (unbinding last classifier will destroy the
+ * action if it was created by classifier). So only case when bind count
+ * can be changed after initial check is when unbound action is
+ * destroyed by act API while classifier binds to action with same id
+ * concurrently. This result either creation of new action(same behavior
+ * as before), or reusing existing action if concurrent process
+ * increments reference count before action is deleted. Both scenarios
+ * are acceptable.
+ */
if (p) {
if (bind)
- p->tcfa_bindcnt--;
- else if (strict && p->tcfa_bindcnt > 0)
+ atomic_dec(&p->tcfa_bindcnt);
+ else if (strict && atomic_read(&p->tcfa_bindcnt) > 0)
return -EPERM;
- p->tcfa_refcnt--;
- if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) {
+ if (atomic_read(&p->tcfa_bindcnt) <= 0 &&
+ refcount_dec_and_test(&p->tcfa_refcnt)) {
if (p->ops->cleanup)
p->ops->cleanup(p);
tcf_idr_remove(p->idrinfo, p);
@@ -304,8 +316,8 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
if (index && p) {
if (bind)
- p->tcfa_bindcnt++;
- p->tcfa_refcnt++;
+ atomic_inc(&p->tcfa_bindcnt);
+ refcount_inc(&p->tcfa_refcnt);
*a = p;
return true;
}
@@ -324,9 +336,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
if (unlikely(!p))
return -ENOMEM;
- p->tcfa_refcnt = 1;
+ refcount_set(&p->tcfa_refcnt, 1);
if (bind)
- p->tcfa_bindcnt = 1;
+ atomic_set(&p->tcfa_bindcnt, 1);
if (cpustats) {
p->cpu_bstats = netdev_alloc_pcpu_stats(struct gnet_stats_basic_cpu);
@@ -782,7 +794,7 @@ static void cleanup_a(struct list_head *actions, int ovr)
return;
list_for_each_entry(a, actions, list)
- a->tcfa_refcnt--;
+ refcount_dec(&a->tcfa_refcnt);
}
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -810,7 +822,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
act->order = i;
sz += tcf_action_fill_size(act);
if (ovr)
- act->tcfa_refcnt++;
+ refcount_inc(&act->tcfa_refcnt);
list_add_tail(&act->list, actions);
}
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 18089c02e557..15a2a53cbde1 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -141,8 +141,8 @@ static int tcf_bpf_dump(struct sk_buff *skb, struct tc_action *act,
struct tcf_bpf *prog = to_bpf(act);
struct tc_act_bpf opt = {
.index = prog->tcf_index,
- .refcnt = prog->tcf_refcnt - ref,
- .bindcnt = prog->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&prog->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&prog->tcf_bindcnt) - bind,
.action = prog->tcf_action,
};
struct tcf_t tm;
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index e4b880fa51fe..188865034f9a 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -154,8 +154,8 @@ static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_action *a,
struct tc_connmark opt = {
.index = ci->tcf_index,
- .refcnt = ci->tcf_refcnt - ref,
- .bindcnt = ci->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&ci->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&ci->tcf_bindcnt) - bind,
.action = ci->tcf_action,
.zone = ci->zone,
};
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 526a8e491626..da865f7b390a 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -597,8 +597,8 @@ static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
struct tcf_csum_params *params;
struct tc_csum opt = {
.index = p->tcf_index,
- .refcnt = p->tcf_refcnt - ref,
- .bindcnt = p->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&p->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&p->tcf_bindcnt) - bind,
};
struct tcf_t t;
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 4dc4f153cad8..ca83debd5a70 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -169,8 +169,8 @@ static int tcf_gact_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_gact *gact = to_gact(a);
struct tc_gact opt = {
.index = gact->tcf_index,
- .refcnt = gact->tcf_refcnt - ref,
- .bindcnt = gact->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&gact->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&gact->tcf_bindcnt) - bind,
.action = gact->tcf_action,
};
struct tcf_t t;
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 20d7d36b2fc9..3536a23f46b5 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -596,8 +596,8 @@ static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
struct tcf_ife_params *p = rtnl_dereference(ife->params);
struct tc_ife opt = {
.index = ife->tcf_index,
- .refcnt = ife->tcf_refcnt - ref,
- .bindcnt = ife->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&ife->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&ife->tcf_bindcnt) - bind,
.action = ife->tcf_action,
.flags = p->flags,
};
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 14c312d7908f..7bce88dc11c9 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -280,8 +280,8 @@ static int tcf_ipt_dump(struct sk_buff *skb, struct tc_action *a, int bind,
if (unlikely(!t))
goto nla_put_failure;
- c.bindcnt = ipt->tcf_bindcnt - bind;
- c.refcnt = ipt->tcf_refcnt - ref;
+ c.bindcnt = atomic_read(&ipt->tcf_bindcnt) - bind;
+ c.refcnt = refcount_read(&ipt->tcf_refcnt) - ref;
strcpy(t->u.user.name, ipt->tcfi_t->u.kernel.target->name);
if (nla_put(skb, TCA_IPT_TARG, ipt->tcfi_t->u.user.target_size, t) ||
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index fd34015331ab..82a8bdd67c47 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -250,8 +250,8 @@ static int tcf_mirred_dump(struct sk_buff *skb, struct tc_action *a, int bind,
struct tc_mirred opt = {
.index = m->tcf_index,
.action = m->tcf_action,
- .refcnt = m->tcf_refcnt - ref,
- .bindcnt = m->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&m->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&m->tcf_bindcnt) - bind,
.eaction = m->tcfm_eaction,
.ifindex = dev ? dev->ifindex : 0,
};
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 4b5848b6c252..457c2ae3de46 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -257,8 +257,8 @@ static int tcf_nat_dump(struct sk_buff *skb, struct tc_action *a,
.index = p->tcf_index,
.action = p->tcf_action,
- .refcnt = p->tcf_refcnt - ref,
- .bindcnt = p->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&p->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&p->tcf_bindcnt) - bind,
};
struct tcf_t t;
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index e43aef28fdac..889690e0ec39 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -409,8 +409,8 @@ static int tcf_pedit_dump(struct sk_buff *skb, struct tc_action *a,
opt->nkeys = p->tcfp_nkeys;
opt->flags = p->tcfp_flags;
opt->action = p->tcf_action;
- opt->refcnt = p->tcf_refcnt - ref;
- opt->bindcnt = p->tcf_bindcnt - bind;
+ opt->refcnt = refcount_read(&p->tcf_refcnt) - ref;
+ opt->bindcnt = atomic_read(&p->tcf_bindcnt) - bind;
if (p->tcfp_keys_ex) {
tcf_pedit_key_ex_dump(skb, p->tcfp_keys_ex, p->tcfp_nkeys);
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 4e72bc2a0dfb..a789b8060968 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -274,8 +274,8 @@ static int tcf_act_police_dump(struct sk_buff *skb, struct tc_action *a,
.action = police->tcf_action,
.mtu = police->tcfp_mtu,
.burst = PSCHED_NS2TICKS(police->tcfp_burst),
- .refcnt = police->tcf_refcnt - ref,
- .bindcnt = police->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&police->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&police->tcf_bindcnt) - bind,
};
struct tcf_t t;
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 5db358497c9e..4a46978db092 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -173,8 +173,8 @@ static int tcf_sample_dump(struct sk_buff *skb, struct tc_action *a,
struct tc_sample opt = {
.index = s->tcf_index,
.action = s->tcf_action,
- .refcnt = s->tcf_refcnt - ref,
- .bindcnt = s->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&s->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&s->tcf_bindcnt) - bind,
};
struct tcf_t t;
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 98c4afe7c15b..c3a761097b01 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -145,8 +145,8 @@ static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_defact *d = to_defact(a);
struct tc_defact opt = {
.index = d->tcf_index,
- .refcnt = d->tcf_refcnt - ref,
- .bindcnt = d->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&d->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&d->tcf_bindcnt) - bind,
.action = d->tcf_action,
};
struct tcf_t t;
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index dfaf5d8028dd..cfd20d3d2ca9 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -208,8 +208,8 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_skbedit *d = to_skbedit(a);
struct tc_skbedit opt = {
.index = d->tcf_index,
- .refcnt = d->tcf_refcnt - ref,
- .bindcnt = d->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&d->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&d->tcf_bindcnt) - bind,
.action = d->tcf_action,
};
struct tcf_t t;
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index ad050d7d4b46..ff90d720eda3 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -205,8 +205,8 @@ static int tcf_skbmod_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_skbmod_params *p = rtnl_dereference(d->skbmod_p);
struct tc_skbmod opt = {
.index = d->tcf_index,
- .refcnt = d->tcf_refcnt - ref,
- .bindcnt = d->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&d->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&d->tcf_bindcnt) - bind,
.action = d->tcf_action,
};
struct tcf_t t;
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index ea203e386a92..2354f07eba15 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -474,8 +474,8 @@ static int tunnel_key_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_tunnel_key_params *params;
struct tc_tunnel_key opt = {
.index = t->tcf_index,
- .refcnt = t->tcf_refcnt - ref,
- .bindcnt = t->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&t->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&t->tcf_bindcnt) - bind,
};
struct tcf_t tm;
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 1fb39e1f9d07..799e3deb44ac 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -239,8 +239,8 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
struct tcf_vlan_params *p = rtnl_dereference(v->vlan_p);
struct tc_vlan opt = {
.index = v->tcf_index,
- .refcnt = v->tcf_refcnt - ref,
- .bindcnt = v->tcf_bindcnt - bind,
+ .refcnt = refcount_read(&v->tcf_refcnt) - ref,
+ .bindcnt = atomic_read(&v->tcf_bindcnt) - bind,
.action = v->tcf_action,
.v_action = p->tcfv_action,
};
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 09/11] net: sched: use reference counting action init
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Change action API to assume that action init function always takes
reference to action, even when overwriting existing action. This is
necessary because action API continues to use action pointer after init
function is done. At this point action becomes accessible for concurrent
modifications, so user must always hold reference to it.
Implement helper put list function to atomically release list of actions
after action API init code is done using them.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Resplit action lookup/release code to prevent memory leaks in
individual patches.
net/sched/act_api.c | 35 +++++++++++++++++------------------
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f019f0464cec..eefe8c2fe667 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -627,6 +627,18 @@ static int tcf_action_put(struct tc_action *p)
return __tcf_action_put(p, false);
}
+static void tcf_action_put_lst(struct list_head *actions)
+{
+ struct tc_action *a, *tmp;
+
+ list_for_each_entry_safe(a, tmp, actions, list) {
+ const struct tc_action_ops *ops = a->ops;
+
+ if (tcf_action_put(a))
+ module_put(ops->owner);
+ }
+}
+
int
tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
{
@@ -835,17 +847,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
return ERR_PTR(err);
}
-static void cleanup_a(struct list_head *actions, int ovr)
-{
- struct tc_action *a;
-
- if (!ovr)
- return;
-
- list_for_each_entry(a, actions, list)
- refcount_dec(&a->tcfa_refcnt);
-}
-
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
struct nlattr *est, char *name, int ovr, int bind,
struct list_head *actions, size_t *attr_size,
@@ -874,11 +875,6 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
}
*attr_size = tcf_action_full_attrs_size(sz);
-
- /* Remove the temp refcnt which was necessary to protect against
- * destroying an existing action which was being replaced
- */
- cleanup_a(actions, ovr);
return 0;
err:
@@ -1209,7 +1205,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
return ret;
}
err:
- tcf_action_destroy(&actions, 0);
+ tcf_action_put_lst(&actions);
return ret;
}
@@ -1251,8 +1247,11 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
&attr_size, true, extack);
if (ret)
return ret;
+ ret = tcf_add_notify(net, n, &actions, portid, attr_size, extack);
+ if (ovr)
+ tcf_action_put_lst(&actions);
- return tcf_add_notify(net, n, &actions, portid, attr_size, extack);
+ return ret;
}
static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 06/11] net: sched: add 'delete' function to action ops
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Extend action ops with 'delete' function. Each action type to implements
its own delete function that doesn't depend on rtnl lock.
Implement delete function that is required to delete actions without
holding rtnl lock. Use action API function that atomically deletes action
only if it is still in action idr. This implementation prevents concurrent
threads from deleting same action twice.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Merge action ops delete definition and implementation.
include/net/act_api.h | 1 +
net/sched/act_bpf.c | 8 ++++++++
net/sched/act_connmark.c | 8 ++++++++
net/sched/act_csum.c | 8 ++++++++
net/sched/act_gact.c | 8 ++++++++
net/sched/act_ife.c | 8 ++++++++
net/sched/act_ipt.c | 16 ++++++++++++++++
net/sched/act_mirred.c | 8 ++++++++
net/sched/act_nat.c | 8 ++++++++
net/sched/act_pedit.c | 8 ++++++++
net/sched/act_police.c | 8 ++++++++
net/sched/act_sample.c | 8 ++++++++
net/sched/act_simple.c | 8 ++++++++
net/sched/act_skbedit.c | 8 ++++++++
net/sched/act_skbmod.c | 8 ++++++++
net/sched/act_tunnel_key.c | 8 ++++++++
net/sched/act_vlan.c | 8 ++++++++
17 files changed, 137 insertions(+)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index a8eaae67c264..b9ed2b8256a5 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -101,6 +101,7 @@ struct tc_action_ops {
void (*stats_update)(struct tc_action *, u64, u32, u64);
size_t (*get_fill_size)(const struct tc_action *act);
struct net_device *(*get_dev)(const struct tc_action *a);
+ int (*delete)(struct net *net, u32 index);
};
struct tc_action_net {
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 8ebf40a3506c..7941dd66ff83 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -388,6 +388,13 @@ static int tcf_bpf_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_bpf_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, bpf_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_bpf_ops __read_mostly = {
.kind = "bpf",
.type = TCA_ACT_BPF,
@@ -398,6 +405,7 @@ static struct tc_action_ops act_bpf_ops __read_mostly = {
.init = tcf_bpf_init,
.walk = tcf_bpf_walker,
.lookup = tcf_bpf_search,
+ .delete = tcf_bpf_delete,
.size = sizeof(struct tcf_bpf),
};
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index e3787aa0025a..143c2d3de723 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -193,6 +193,13 @@ static int tcf_connmark_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_connmark_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, connmark_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_connmark_ops = {
.kind = "connmark",
.type = TCA_ACT_CONNMARK,
@@ -202,6 +209,7 @@ static struct tc_action_ops act_connmark_ops = {
.init = tcf_connmark_init,
.walk = tcf_connmark_walker,
.lookup = tcf_connmark_search,
+ .delete = tcf_connmark_delete,
.size = sizeof(struct tcf_connmark_info),
};
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 334261943f9f..3768539340e0 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -654,6 +654,13 @@ static size_t tcf_csum_get_fill_size(const struct tc_action *act)
return nla_total_size(sizeof(struct tc_csum));
}
+static int tcf_csum_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, csum_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_csum_ops = {
.kind = "csum",
.type = TCA_ACT_CSUM,
@@ -665,6 +672,7 @@ static struct tc_action_ops act_csum_ops = {
.walk = tcf_csum_walker,
.lookup = tcf_csum_search,
.get_fill_size = tcf_csum_get_fill_size,
+ .delete = tcf_csum_delete,
.size = sizeof(struct tcf_csum),
};
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index b4dfb2b4addc..a431a711f0dd 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -231,6 +231,13 @@ static size_t tcf_gact_get_fill_size(const struct tc_action *act)
return sz;
}
+static int tcf_gact_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, gact_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_gact_ops = {
.kind = "gact",
.type = TCA_ACT_GACT,
@@ -242,6 +249,7 @@ static struct tc_action_ops act_gact_ops = {
.walk = tcf_gact_walker,
.lookup = tcf_gact_search,
.get_fill_size = tcf_gact_get_fill_size,
+ .delete = tcf_gact_delete,
.size = sizeof(struct tcf_gact),
};
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 576ffbba61c3..89a761395c94 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -844,6 +844,13 @@ static int tcf_ife_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_ife_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, ife_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_ife_ops = {
.kind = "ife",
.type = TCA_ACT_IFE,
@@ -854,6 +861,7 @@ static struct tc_action_ops act_ife_ops = {
.init = tcf_ife_init,
.walk = tcf_ife_walker,
.lookup = tcf_ife_search,
+ .delete = tcf_ife_delete,
.size = sizeof(struct tcf_ife_info),
};
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 9c21663a86a6..6c234411c771 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -324,6 +324,13 @@ static int tcf_ipt_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_ipt_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, ipt_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_ipt_ops = {
.kind = "ipt",
.type = TCA_ACT_IPT,
@@ -334,6 +341,7 @@ static struct tc_action_ops act_ipt_ops = {
.init = tcf_ipt_init,
.walk = tcf_ipt_walker,
.lookup = tcf_ipt_search,
+ .delete = tcf_ipt_delete,
.size = sizeof(struct tcf_ipt),
};
@@ -374,6 +382,13 @@ static int tcf_xt_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_xt_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, xt_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_xt_ops = {
.kind = "xt",
.type = TCA_ACT_XT,
@@ -384,6 +399,7 @@ static struct tc_action_ops act_xt_ops = {
.init = tcf_xt_init,
.walk = tcf_xt_walker,
.lookup = tcf_xt_search,
+ .delete = tcf_xt_delete,
.size = sizeof(struct tcf_ipt),
};
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 5434f08f2eb7..3d8300bce7e4 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -322,6 +322,13 @@ static struct net_device *tcf_mirred_get_dev(const struct tc_action *a)
return rtnl_dereference(m->tcfm_dev);
}
+static int tcf_mirred_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, mirred_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_mirred_ops = {
.kind = "mirred",
.type = TCA_ACT_MIRRED,
@@ -335,6 +342,7 @@ static struct tc_action_ops act_mirred_ops = {
.lookup = tcf_mirred_search,
.size = sizeof(struct tcf_mirred),
.get_dev = tcf_mirred_get_dev,
+ .delete = tcf_mirred_delete,
};
static __net_init int mirred_init_net(struct net *net)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index e6487ad1e4a8..9eb27c89dc46 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -294,6 +294,13 @@ static int tcf_nat_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_nat_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, nat_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_nat_ops = {
.kind = "nat",
.type = TCA_ACT_NAT,
@@ -303,6 +310,7 @@ static struct tc_action_ops act_nat_ops = {
.init = tcf_nat_init,
.walk = tcf_nat_walker,
.lookup = tcf_nat_search,
+ .delete = tcf_nat_delete,
.size = sizeof(struct tcf_nat),
};
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index f7965f35585b..45871052840f 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -454,6 +454,13 @@ static int tcf_pedit_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_pedit_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, pedit_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_pedit_ops = {
.kind = "pedit",
.type = TCA_ACT_PEDIT,
@@ -464,6 +471,7 @@ static struct tc_action_ops act_pedit_ops = {
.init = tcf_pedit_init,
.walk = tcf_pedit_walker,
.lookup = tcf_pedit_search,
+ .delete = tcf_pedit_delete,
.size = sizeof(struct tcf_pedit),
};
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 0e1c2fb0ebea..c955fb0d4f3f 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -314,6 +314,13 @@ static int tcf_police_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_police_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, police_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
MODULE_AUTHOR("Alexey Kuznetsov");
MODULE_DESCRIPTION("Policing actions");
MODULE_LICENSE("GPL");
@@ -327,6 +334,7 @@ static struct tc_action_ops act_police_ops = {
.init = tcf_act_police_init,
.walk = tcf_act_police_walker,
.lookup = tcf_police_search,
+ .delete = tcf_police_delete,
.size = sizeof(struct tcf_police),
};
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 316fc645595d..6f79d2afcba2 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -220,6 +220,13 @@ static int tcf_sample_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_sample_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, sample_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_sample_ops = {
.kind = "sample",
.type = TCA_ACT_SAMPLE,
@@ -230,6 +237,7 @@ static struct tc_action_ops act_sample_ops = {
.cleanup = tcf_sample_cleanup,
.walk = tcf_sample_walker,
.lookup = tcf_sample_search,
+ .delete = tcf_sample_delete,
.size = sizeof(struct tcf_sample),
};
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index dc591cc87f4a..446c750f3d3c 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -184,6 +184,13 @@ static int tcf_simp_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_simp_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, simp_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_simp_ops = {
.kind = "simple",
.type = TCA_ACT_SIMP,
@@ -194,6 +201,7 @@ static struct tc_action_ops act_simp_ops = {
.init = tcf_simp_init,
.walk = tcf_simp_walker,
.lookup = tcf_simp_search,
+ .delete = tcf_simp_delete,
.size = sizeof(struct tcf_defact),
};
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index c4ae4bd830aa..b3eaa120c7f4 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -267,6 +267,13 @@ static int tcf_skbedit_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_skbedit_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, skbedit_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_skbedit_ops = {
.kind = "skbedit",
.type = TCA_ACT_SKBEDIT,
@@ -276,6 +283,7 @@ static struct tc_action_ops act_skbedit_ops = {
.init = tcf_skbedit_init,
.walk = tcf_skbedit_walker,
.lookup = tcf_skbedit_search,
+ .delete = tcf_skbedit_delete,
.size = sizeof(struct tcf_skbedit),
};
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 026d6f58eda1..30be3f767495 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -253,6 +253,13 @@ static int tcf_skbmod_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_skbmod_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, skbmod_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_skbmod_ops = {
.kind = "skbmod",
.type = TCA_ACT_SKBMOD,
@@ -263,6 +270,7 @@ static struct tc_action_ops act_skbmod_ops = {
.cleanup = tcf_skbmod_cleanup,
.walk = tcf_skbmod_walker,
.lookup = tcf_skbmod_search,
+ .delete = tcf_skbmod_delete,
.size = sizeof(struct tcf_skbmod),
};
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 15ea5ce0f9ed..655ed0b3fc67 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -534,6 +534,13 @@ static int tunnel_key_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tunnel_key_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_tunnel_key_ops = {
.kind = "tunnel_key",
.type = TCA_ACT_TUNNEL_KEY,
@@ -544,6 +551,7 @@ static struct tc_action_ops act_tunnel_key_ops = {
.cleanup = tunnel_key_release,
.walk = tunnel_key_walker,
.lookup = tunnel_key_search,
+ .delete = tunnel_key_delete,
.size = sizeof(struct tcf_tunnel_key),
};
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index c61775250722..e334d2751784 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -287,6 +287,13 @@ static int tcf_vlan_search(struct net *net, struct tc_action **a, u32 index,
return tcf_idr_search(tn, a, index);
}
+static int tcf_vlan_delete(struct net *net, u32 index)
+{
+ struct tc_action_net *tn = net_generic(net, vlan_net_id);
+
+ return tcf_idr_delete_index(tn, index);
+}
+
static struct tc_action_ops act_vlan_ops = {
.kind = "vlan",
.type = TCA_ACT_VLAN,
@@ -297,6 +304,7 @@ static struct tc_action_ops act_vlan_ops = {
.cleanup = tcf_vlan_cleanup,
.walk = tcf_vlan_walker,
.lookup = tcf_vlan_search,
+ .delete = tcf_vlan_delete,
.size = sizeof(struct tcf_vlan),
};
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 07/11] net: sched: implement reference counted action release
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Implement helper delete function that uses new action ops 'delete', instead
of destroying action directly. This is required so act API could delete
actions by index, without holding any references to action that is being
deleted.
Implement function __tcf_action_put() that releases reference to action and
frees it, if necessary. Refactor action deletion code to use new put
function and not to rely on rtnl lock. Remove rtnl lock assertions that are
no longer needed.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Removed redundant actions ops lookup during delete.
- Assume all actions have delete implemented and don't check for it
explicitly.
- Rearrange variable definitions in tcf_action_delete.
net/sched/act_api.c | 84 +++++++++++++++++++++++++++++++++++++++--------------
net/sched/cls_api.c | 1 -
2 files changed, 62 insertions(+), 23 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 0f31f09946ab..a023873db713 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -90,21 +90,39 @@ static void free_tcf(struct tc_action *p)
kfree(p);
}
-static void tcf_idr_remove(struct tcf_idrinfo *idrinfo, struct tc_action *p)
+static void tcf_action_cleanup(struct tc_action *p)
{
- spin_lock(&idrinfo->lock);
- idr_remove(&idrinfo->action_idr, p->tcfa_index);
- spin_unlock(&idrinfo->lock);
+ if (p->ops->cleanup)
+ p->ops->cleanup(p);
+
gen_kill_estimator(&p->tcfa_rate_est);
free_tcf(p);
}
+static int __tcf_action_put(struct tc_action *p, bool bind)
+{
+ struct tcf_idrinfo *idrinfo = p->idrinfo;
+
+ if (refcount_dec_and_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
+ if (bind)
+ atomic_dec(&p->tcfa_bindcnt);
+ idr_remove(&idrinfo->action_idr, p->tcfa_index);
+ spin_unlock(&idrinfo->lock);
+
+ tcf_action_cleanup(p);
+ return 1;
+ }
+
+ if (bind)
+ atomic_dec(&p->tcfa_bindcnt);
+
+ return 0;
+}
+
int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
{
int ret = 0;
- ASSERT_RTNL();
-
/* Release with strict==1 and bind==0 is only called through act API
* interface (classifiers always bind). Only case when action with
* positive reference count and zero bind count can exist is when it was
@@ -118,18 +136,11 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
* are acceptable.
*/
if (p) {
- if (bind)
- atomic_dec(&p->tcfa_bindcnt);
- else if (strict && atomic_read(&p->tcfa_bindcnt) > 0)
+ if (!bind && strict && atomic_read(&p->tcfa_bindcnt) > 0)
return -EPERM;
- if (atomic_read(&p->tcfa_bindcnt) <= 0 &&
- refcount_dec_and_test(&p->tcfa_refcnt)) {
- if (p->ops->cleanup)
- p->ops->cleanup(p);
- tcf_idr_remove(p->idrinfo, p);
+ if (__tcf_action_put(p, bind))
ret = ACT_P_DELETED;
- }
}
return ret;
@@ -340,11 +351,7 @@ int tcf_idr_delete_index(struct tc_action_net *tn, u32 index)
p->tcfa_index));
spin_unlock(&idrinfo->lock);
- if (p->ops->cleanup)
- p->ops->cleanup(p);
-
- gen_kill_estimator(&p->tcfa_rate_est);
- free_tcf(p);
+ tcf_action_cleanup(p);
module_put(owner);
return 0;
}
@@ -615,6 +622,11 @@ int tcf_action_destroy(struct list_head *actions, int bind)
return ret;
}
+static int tcf_action_put(struct tc_action *p)
+{
+ return __tcf_action_put(p, false);
+}
+
int
tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
{
@@ -1092,6 +1104,35 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
return err;
}
+static int tcf_action_delete(struct net *net, struct list_head *actions,
+ struct netlink_ext_ack *extack)
+{
+ struct tc_action *a, *tmp;
+ u32 act_index;
+ int ret;
+
+ list_for_each_entry_safe(a, tmp, actions, list) {
+ const struct tc_action_ops *ops = a->ops;
+
+ /* Actions can be deleted concurrently so we must save their
+ * type and id to search again after reference is released.
+ */
+ act_index = a->tcfa_index;
+
+ list_del(&a->list);
+ if (tcf_action_put(a)) {
+ /* last reference, action was deleted concurrently */
+ module_put(ops->owner);
+ } else {
+ /* now do the delete */
+ ret = ops->delete(net, act_index);
+ if (ret < 0)
+ return ret;
+ }
+ }
+ return 0;
+}
+
static int
tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
@@ -1112,7 +1153,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
}
/* now do the delete */
- ret = tcf_action_destroy(actions, 0);
+ ret = tcf_action_delete(net, actions, extack);
if (ret < 0) {
NL_SET_ERR_MSG(extack, "Failed to delete TC action");
kfree_skb(skb);
@@ -1164,7 +1205,6 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
if (event == RTM_GETACTION)
ret = tcf_get_notify(net, portid, n, &actions, event, extack);
else { /* delete */
- cleanup_a(&actions, 1); /* lookup took reference */
ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
if (ret)
goto err;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index ebc2b9dd783f..9041f0e43e9a 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1611,7 +1611,6 @@ void tcf_exts_destroy(struct tcf_exts *exts)
#ifdef CONFIG_NET_CLS_ACT
LIST_HEAD(actions);
- ASSERT_RTNL();
tcf_exts_to_list(exts, &actions);
tcf_action_destroy(&actions, TCA_ACT_UNBIND);
kfree(exts->actions);
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 08/11] net: sched: don't release reference on action overwrite
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Return from action init function with reference to action taken,
even when overwriting existing action.
Action init API initializes its fourth argument (pointer to pointer to tc
action) to either existing action with same index or newly created action.
In case of existing index(and bind argument is zero), init function returns
without incrementing action reference counter. Caller of action init then
proceeds working with action, without actually holding reference to it.
This means that action could be deleted concurrently.
Change action init behavior to always take reference to action before
returning successfully, in order to protect from concurrent deletion.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Resplit action lookup/release code to prevent memory leaks in
individual patches.
- Change convoluted commit message.
net/sched/act_api.c | 2 --
net/sched/act_bpf.c | 8 ++++----
net/sched/act_connmark.c | 5 +++--
net/sched/act_csum.c | 8 ++++----
net/sched/act_gact.c | 5 +++--
net/sched/act_ife.c | 10 +++++-----
net/sched/act_ipt.c | 5 +++--
net/sched/act_mirred.c | 5 ++---
net/sched/act_nat.c | 5 +++--
net/sched/act_pedit.c | 2 +-
net/sched/act_police.c | 8 +++-----
net/sched/act_sample.c | 8 +++-----
net/sched/act_simple.c | 5 +++--
net/sched/act_skbedit.c | 5 +++--
net/sched/act_skbmod.c | 8 +++-----
net/sched/act_tunnel_key.c | 11 ++++-------
net/sched/act_vlan.c | 8 +++-----
17 files changed, 50 insertions(+), 58 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index a023873db713..f019f0464cec 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -870,8 +870,6 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
}
act->order = i;
sz += tcf_action_fill_size(act);
- if (ovr)
- refcount_inc(&act->tcfa_refcnt);
list_add_tail(&act->list, actions);
}
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 7941dd66ff83..d3f4ac6f2c4b 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -311,9 +311,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
if (bind)
return 0;
- tcf_idr_release(*act, bind);
- if (!replace)
+ if (!replace) {
+ tcf_idr_release(*act, bind);
return -EEXIST;
+ }
}
is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
@@ -356,8 +357,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
return res;
out:
- if (res == ACT_P_CREATED)
- tcf_idr_release(*act, bind);
+ tcf_idr_release(*act, bind);
return ret;
}
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 143c2d3de723..701e90244eff 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -135,9 +135,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
ci = to_connmark(*a);
if (bind)
return 0;
- tcf_idr_release(*a, bind);
- if (!ovr)
+ if (!ovr) {
+ tcf_idr_release(*a, bind);
return -EEXIST;
+ }
/* replacing action and zone */
ci->tcf_action = parm->action;
ci->zone = parm->zone;
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 3768539340e0..5dbee136b0a1 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -76,9 +76,10 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
} else {
if (bind)/* dont override defaults */
return 0;
- tcf_idr_release(*a, bind);
- if (!ovr)
+ if (!ovr) {
+ tcf_idr_release(*a, bind);
return -EEXIST;
+ }
}
p = to_tcf_csum(*a);
@@ -86,8 +87,7 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
return -ENOMEM;
}
params_old = rtnl_dereference(p->params);
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index a431a711f0dd..11c4de3f344e 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -100,9 +100,10 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
} else {
if (bind)/* dont override defaults */
return 0;
- tcf_idr_release(*a, bind);
- if (!ovr)
+ if (!ovr) {
+ tcf_idr_release(*a, bind);
return -EEXIST;
+ }
}
gact = to_gact(*a);
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 89a761395c94..acea3feae762 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -498,12 +498,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
return ret;
}
ret = ACT_P_CREATED;
- } else {
+ } else if (!ovr) {
tcf_idr_release(*a, bind);
- if (!ovr) {
- kfree(p);
- return -EEXIST;
- }
+ kfree(p);
+ return -EEXIST;
}
ife = to_ife(*a);
@@ -548,6 +546,8 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
if (exists)
spin_unlock_bh(&ife->tcf_lock);
+ tcf_idr_release(*a, bind);
+
kfree(p);
return err;
}
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 6c234411c771..85e85dfba401 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -145,10 +145,11 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
} else {
if (bind)/* dont override defaults */
return 0;
- tcf_idr_release(*a, bind);
- if (!ovr)
+ if (!ovr) {
+ tcf_idr_release(*a, bind);
return -EEXIST;
+ }
}
hook = nla_get_u32(tb[TCA_IPT_HOOK]);
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 3d8300bce7e4..e08aed06d7f8 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -132,10 +132,9 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
if (ret)
return ret;
ret = ACT_P_CREATED;
- } else {
+ } else if (!ovr) {
tcf_idr_release(*a, bind);
- if (!ovr)
- return -EEXIST;
+ return -EEXIST;
}
m = to_mirred(*a);
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 9eb27c89dc46..1f91e8e66c0f 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -66,9 +66,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
} else {
if (bind)
return 0;
- tcf_idr_release(*a, bind);
- if (!ovr)
+ if (!ovr) {
+ tcf_idr_release(*a, bind);
return -EEXIST;
+ }
}
p = to_tcf_nat(*a);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 45871052840f..3a0e2f762f4e 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -194,8 +194,8 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
} else {
if (bind)
goto out_free;
- tcf_idr_release(*a, bind);
if (!ovr) {
+ tcf_idr_release(*a, bind);
ret = -EEXIST;
goto out_free;
}
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index c955fb0d4f3f..99335cca739e 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -111,10 +111,9 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
if (ret)
return ret;
ret = ACT_P_CREATED;
- } else {
+ } else if (!ovr) {
tcf_idr_release(*a, bind);
- if (!ovr)
- return -EEXIST;
+ return -EEXIST;
}
police = to_police(*a);
@@ -195,8 +194,7 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
failure:
qdisc_put_rtab(P_tab);
qdisc_put_rtab(R_tab);
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
return err;
}
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 6f79d2afcba2..a8582e1347db 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -69,10 +69,9 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
if (ret)
return ret;
ret = ACT_P_CREATED;
- } else {
+ } else if (!ovr) {
tcf_idr_release(*a, bind);
- if (!ovr)
- return -EEXIST;
+ return -EEXIST;
}
s = to_sample(*a);
@@ -81,8 +80,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
s->psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
psample_group = psample_group_get(net, s->psample_group_num);
if (!psample_group) {
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
return -ENOMEM;
}
RCU_INIT_POINTER(s->psample_group, psample_group);
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 446c750f3d3c..2da47c682a30 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -127,9 +127,10 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
} else {
d = to_defact(*a);
- tcf_idr_release(*a, bind);
- if (!ovr)
+ if (!ovr) {
+ tcf_idr_release(*a, bind);
return -EEXIST;
+ }
reset_policy(d, tb[TCA_DEF_DATA], parm);
}
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index b3eaa120c7f4..4616a2c1821f 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -172,9 +172,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
ret = ACT_P_CREATED;
} else {
d = to_skbedit(*a);
- tcf_idr_release(*a, bind);
- if (!ovr)
+ if (!ovr) {
+ tcf_idr_release(*a, bind);
return -EEXIST;
+ }
}
spin_lock_bh(&d->tcf_lock);
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 30be3f767495..e844381af066 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -145,10 +145,9 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
return ret;
ret = ACT_P_CREATED;
- } else {
+ } else if (!ovr) {
tcf_idr_release(*a, bind);
- if (!ovr)
- return -EEXIST;
+ return -EEXIST;
}
d = to_skbmod(*a);
@@ -156,8 +155,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
ASSERT_RTNL();
p = kzalloc(sizeof(struct tcf_skbmod_params), GFP_KERNEL);
if (unlikely(!p)) {
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
return -ENOMEM;
}
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 655ed0b3fc67..ab5bf5c13f87 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -329,12 +329,10 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
}
ret = ACT_P_CREATED;
- } else {
+ } else if (!ovr) {
tcf_idr_release(*a, bind);
- if (!ovr) {
- NL_SET_ERR_MSG(extack, "TC IDR already exists");
- return -EEXIST;
- }
+ NL_SET_ERR_MSG(extack, "TC IDR already exists");
+ return -EEXIST;
}
t = to_tunnel_key(*a);
@@ -342,8 +340,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
ASSERT_RTNL();
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
NL_SET_ERR_MSG(extack, "Cannot allocate tunnel key parameters");
return -ENOMEM;
}
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index e334d2751784..9b600faaccbb 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -187,10 +187,9 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
return ret;
ret = ACT_P_CREATED;
- } else {
+ } else if (!ovr) {
tcf_idr_release(*a, bind);
- if (!ovr)
- return -EEXIST;
+ return -EEXIST;
}
v = to_vlan(*a);
@@ -198,8 +197,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
ASSERT_RTNL();
p = kzalloc(sizeof(*p), GFP_KERNEL);
if (!p) {
- if (ret == ACT_P_CREATED)
- tcf_idr_release(*a, bind);
+ tcf_idr_release(*a, bind);
return -ENOMEM;
}
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 11/11] net: sched: change action API to use array of pointers to actions
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Act API used linked list to pass set of actions to functions. It is
intrusive data structure that stores list nodes inside action structure
itself, which means it is not safe to modify such list concurrently.
However, action API doesn't use any linked list specific operations on this
set of actions, so it can be safely refactored into plain pointer array.
Refactor action API to use array of pointers to tc_actions instead of
linked list. Change argument 'actions' type of exported action init,
destroy and dump functions.
Acked-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V5 to V6:
- When action is deleted, set pointer in actions array to NULL to
prevent double freeing.
Changes from V4 to V5:
- Change action delete API to track actions that were deleted, to
prevent releasing them on error.
Changes from V3 to V4:
- Reduce actions array size in tcf_action_init_1.
include/net/act_api.h | 7 ++--
net/sched/act_api.c | 89 +++++++++++++++++++++++++++++----------------------
net/sched/cls_api.c | 21 ++++--------
3 files changed, 60 insertions(+), 57 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 8090de2edab7..683ce41053d9 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -168,19 +168,20 @@ static inline int tcf_idr_release(struct tc_action *a, bool bind)
int tcf_register_action(struct tc_action_ops *a, struct pernet_operations *ops);
int tcf_unregister_action(struct tc_action_ops *a,
struct pernet_operations *ops);
-int tcf_action_destroy(struct list_head *actions, int bind);
+int tcf_action_destroy(struct tc_action *actions[], int bind);
int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
int nr_actions, struct tcf_result *res);
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
struct nlattr *est, char *name, int ovr, int bind,
- struct list_head *actions, size_t *attr_size,
+ struct tc_action *actions[], size_t *attr_size,
bool rtnl_held, struct netlink_ext_ack *extack);
struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
struct nlattr *nla, struct nlattr *est,
char *name, int ovr, int bind,
bool rtnl_held,
struct netlink_ext_ack *extack);
-int tcf_action_dump(struct sk_buff *skb, struct list_head *, int, int);
+int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[], int bind,
+ int ref);
int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 9511502e1cbb..bf1c35f3deb6 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -657,13 +657,15 @@ int tcf_action_exec(struct sk_buff *skb, struct tc_action **actions,
}
EXPORT_SYMBOL(tcf_action_exec);
-int tcf_action_destroy(struct list_head *actions, int bind)
+int tcf_action_destroy(struct tc_action *actions[], int bind)
{
const struct tc_action_ops *ops;
- struct tc_action *a, *tmp;
- int ret = 0;
+ struct tc_action *a;
+ int ret = 0, i;
- list_for_each_entry_safe(a, tmp, actions, list) {
+ for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+ a = actions[i];
+ actions[i] = NULL;
ops = a->ops;
ret = __tcf_idr_release(a, bind, true);
if (ret == ACT_P_DELETED)
@@ -679,11 +681,12 @@ static int tcf_action_put(struct tc_action *p)
return __tcf_action_put(p, false);
}
-static void tcf_action_put_lst(struct list_head *actions)
+static void tcf_action_put_many(struct tc_action *actions[])
{
- struct tc_action *a, *tmp;
+ int i;
- list_for_each_entry_safe(a, tmp, actions, list) {
+ for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+ struct tc_action *a = actions[i];
const struct tc_action_ops *ops = a->ops;
if (tcf_action_put(a))
@@ -735,14 +738,15 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
}
EXPORT_SYMBOL(tcf_action_dump_1);
-int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
+int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[],
int bind, int ref)
{
struct tc_action *a;
- int err = -EINVAL;
+ int err = -EINVAL, i;
struct nlattr *nest;
- list_for_each_entry(a, actions, list) {
+ for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+ a = actions[i];
nest = nla_nest_start(skb, a->order);
if (nest == NULL)
goto nla_put_failure;
@@ -878,10 +882,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
err = tcf_action_goto_chain_init(a, tp);
if (err) {
- LIST_HEAD(actions);
+ struct tc_action *actions[] = { a, NULL };
- list_add_tail(&a->list, &actions);
- tcf_action_destroy(&actions, bind);
+ tcf_action_destroy(actions, bind);
NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
return ERR_PTR(err);
}
@@ -899,9 +902,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
return ERR_PTR(err);
}
+/* Returns numbers of initialized actions or negative error. */
+
int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
struct nlattr *est, char *name, int ovr, int bind,
- struct list_head *actions, size_t *attr_size,
+ struct tc_action *actions[], size_t *attr_size,
bool rtnl_held, struct netlink_ext_ack *extack)
{
struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
@@ -923,11 +928,12 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
}
act->order = i;
sz += tcf_action_fill_size(act);
- list_add_tail(&act->list, actions);
+ /* Start from index 0 */
+ actions[i - 1] = act;
}
*attr_size = tcf_action_full_attrs_size(sz);
- return 0;
+ return i - 1;
err:
tcf_action_destroy(actions, bind);
@@ -978,7 +984,7 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
return -1;
}
-static int tca_get_fill(struct sk_buff *skb, struct list_head *actions,
+static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
u32 portid, u32 seq, u16 flags, int event, int bind,
int ref)
{
@@ -1014,7 +1020,7 @@ static int tca_get_fill(struct sk_buff *skb, struct list_head *actions,
static int
tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
- struct list_head *actions, int event,
+ struct tc_action *actions[], int event,
struct netlink_ext_ack *extack)
{
struct sk_buff *skb;
@@ -1150,14 +1156,14 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
return err;
}
-static int tcf_action_delete(struct net *net, struct list_head *actions,
- struct netlink_ext_ack *extack)
+static int tcf_action_delete(struct net *net, struct tc_action *actions[],
+ int *acts_deleted, struct netlink_ext_ack *extack)
{
- struct tc_action *a, *tmp;
u32 act_index;
- int ret;
+ int ret, i;
- list_for_each_entry_safe(a, tmp, actions, list) {
+ for (i = 0; i < TCA_ACT_MAX_PRIO && actions[i]; i++) {
+ struct tc_action *a = actions[i];
const struct tc_action_ops *ops = a->ops;
/* Actions can be deleted concurrently so we must save their
@@ -1165,23 +1171,26 @@ static int tcf_action_delete(struct net *net, struct list_head *actions,
*/
act_index = a->tcfa_index;
- list_del(&a->list);
if (tcf_action_put(a)) {
/* last reference, action was deleted concurrently */
module_put(ops->owner);
} else {
/* now do the delete */
ret = ops->delete(net, act_index);
- if (ret < 0)
+ if (ret < 0) {
+ *acts_deleted = i + 1;
return ret;
+ }
}
}
+ *acts_deleted = i;
return 0;
}
static int
-tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
- u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
+tcf_del_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
+ int *acts_deleted, u32 portid, size_t attr_size,
+ struct netlink_ext_ack *extack)
{
int ret;
struct sk_buff *skb;
@@ -1199,7 +1208,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
}
/* now do the delete */
- ret = tcf_action_delete(net, actions, extack);
+ ret = tcf_action_delete(net, actions, acts_deleted, extack);
if (ret < 0) {
NL_SET_ERR_MSG(extack, "Failed to delete TC action");
kfree_skb(skb);
@@ -1221,7 +1230,8 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
struct tc_action *act;
size_t attr_size = 0;
- LIST_HEAD(actions);
+ struct tc_action *actions[TCA_ACT_MAX_PRIO + 1] = {};
+ int acts_deleted = 0;
ret = nla_parse_nested(tb, TCA_ACT_MAX_PRIO, nla, NULL, extack);
if (ret < 0)
@@ -1243,26 +1253,27 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
}
act->order = i;
attr_size += tcf_action_fill_size(act);
- list_add_tail(&act->list, &actions);
+ actions[i - 1] = act;
}
attr_size = tcf_action_full_attrs_size(attr_size);
if (event == RTM_GETACTION)
- ret = tcf_get_notify(net, portid, n, &actions, event, extack);
+ ret = tcf_get_notify(net, portid, n, actions, event, extack);
else { /* delete */
- ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
+ ret = tcf_del_notify(net, n, actions, &acts_deleted, portid,
+ attr_size, extack);
if (ret)
goto err;
return ret;
}
err:
- tcf_action_put_lst(&actions);
+ tcf_action_put_many(&actions[acts_deleted]);
return ret;
}
static int
-tcf_add_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
+tcf_add_notify(struct net *net, struct nlmsghdr *n, struct tc_action *actions[],
u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
{
struct sk_buff *skb;
@@ -1293,15 +1304,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
{
size_t attr_size = 0;
int ret = 0;
- LIST_HEAD(actions);
+ struct tc_action *actions[TCA_ACT_MAX_PRIO] = {};
- ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0, &actions,
+ ret = tcf_action_init(net, NULL, nla, NULL, NULL, ovr, 0, actions,
&attr_size, true, extack);
- if (ret)
+ if (ret < 0)
return ret;
- ret = tcf_add_notify(net, n, &actions, portid, attr_size, extack);
+ ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
if (ovr)
- tcf_action_put_lst(&actions);
+ tcf_action_put_many(actions);
return ret;
}
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 9041f0e43e9a..73d9967c3739 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1609,10 +1609,7 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
void tcf_exts_destroy(struct tcf_exts *exts)
{
#ifdef CONFIG_NET_CLS_ACT
- LIST_HEAD(actions);
-
- tcf_exts_to_list(exts, &actions);
- tcf_action_destroy(&actions, TCA_ACT_UNBIND);
+ tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
kfree(exts->actions);
exts->nr_actions = 0;
#endif
@@ -1639,18 +1636,15 @@ int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
exts->actions[0] = act;
exts->nr_actions = 1;
} else if (exts->action && tb[exts->action]) {
- LIST_HEAD(actions);
- int err, i = 0;
+ int err;
err = tcf_action_init(net, tp, tb[exts->action],
rate_tlv, NULL, ovr, TCA_ACT_BIND,
- &actions, &attr_size, true,
+ exts->actions, &attr_size, true,
extack);
- if (err)
+ if (err < 0)
return err;
- list_for_each_entry(act, &actions, list)
- exts->actions[i++] = act;
- exts->nr_actions = i;
+ exts->nr_actions = err;
}
exts->net = net;
}
@@ -1699,14 +1693,11 @@ int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
* tc data even if iproute2 was newer - jhs
*/
if (exts->type != TCA_OLD_COMPAT) {
- LIST_HEAD(actions);
-
nest = nla_nest_start(skb, exts->action);
if (nest == NULL)
goto nla_put_failure;
- tcf_exts_to_list(exts, &actions);
- if (tcf_action_dump(skb, &actions, 0, 0) < 0)
+ if (tcf_action_dump(skb, exts->actions, 0, 0) < 0)
goto nla_put_failure;
nla_nest_end(skb, nest);
} else if (exts->police) {
--
2.7.5
^ permalink raw reply related
* [PATCH net-next v6 10/11] net: sched: atomically check-allocate action
From: Vlad Buslov @ 2018-07-05 14:24 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, xiyou.wangcong, jiri, ast, daniel, kliteyn,
Vlad Buslov, Jiri Pirko
In-Reply-To: <1530800673-12280-1-git-send-email-vladbu@mellanox.com>
Implement function that atomically checks if action exists and either takes
reference to it, or allocates idr slot for action index to prevent
concurrent allocations of actions with same index. Use EBUSY error pointer
to indicate that idr slot is reserved.
Implement cleanup helper function that removes temporary error pointer from
idr. (in case of error between idr allocation and insertion of newly
created action to specified index)
Refactor all action init functions to insert new action to idr using this
API.
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
Changes from V1 to V2:
- Remove unique idr insertion function. Change original idr insert to do
the same thing.
- Refactor action check-alloc code into standalone function.
include/net/act_api.h | 3 ++
net/sched/act_api.c | 92 ++++++++++++++++++++++++++++++++++++----------
net/sched/act_bpf.c | 11 ++++--
net/sched/act_connmark.c | 10 +++--
net/sched/act_csum.c | 11 ++++--
net/sched/act_gact.c | 11 ++++--
net/sched/act_ife.c | 6 ++-
net/sched/act_ipt.c | 13 ++++++-
net/sched/act_mirred.c | 16 ++++++--
net/sched/act_nat.c | 11 ++++--
net/sched/act_pedit.c | 12 ++++--
net/sched/act_police.c | 9 ++++-
net/sched/act_sample.c | 11 ++++--
net/sched/act_simple.c | 11 +++++-
net/sched/act_skbedit.c | 11 +++++-
net/sched/act_skbmod.c | 11 +++++-
net/sched/act_tunnel_key.c | 9 ++++-
net/sched/act_vlan.c | 17 ++++++++-
18 files changed, 216 insertions(+), 59 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index b9ed2b8256a5..8090de2edab7 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -154,6 +154,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
int bind, bool cpustats);
void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
+void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
+int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
+ struct tc_action **a, int bind);
int tcf_idr_delete_index(struct tc_action_net *tn, u32 index);
int __tcf_idr_release(struct tc_action *a, bool bind, bool strict);
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index eefe8c2fe667..9511502e1cbb 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -303,7 +303,9 @@ static bool __tcf_idr_check(struct tc_action_net *tn, u32 index,
spin_lock(&idrinfo->lock);
p = idr_find(&idrinfo->action_idr, index);
- if (p) {
+ if (IS_ERR(p)) {
+ p = NULL;
+ } else if (p) {
refcount_inc(&p->tcfa_refcnt);
if (bind)
atomic_inc(&p->tcfa_bindcnt);
@@ -371,7 +373,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
{
struct tc_action *p = kzalloc(ops->size, GFP_KERNEL);
struct tcf_idrinfo *idrinfo = tn->idrinfo;
- struct idr *idr = &idrinfo->action_idr;
int err = -ENOMEM;
if (unlikely(!p))
@@ -389,20 +390,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
goto err2;
}
spin_lock_init(&p->tcfa_lock);
- idr_preload(GFP_KERNEL);
- spin_lock(&idrinfo->lock);
- /* user doesn't specify an index */
- if (!index) {
- index = 1;
- err = idr_alloc_u32(idr, NULL, &index, UINT_MAX, GFP_ATOMIC);
- } else {
- err = idr_alloc_u32(idr, NULL, &index, index, GFP_ATOMIC);
- }
- spin_unlock(&idrinfo->lock);
- idr_preload_end();
- if (err)
- goto err3;
-
p->tcfa_index = index;
p->tcfa_tm.install = jiffies;
p->tcfa_tm.lastuse = jiffies;
@@ -412,7 +399,7 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
&p->tcfa_rate_est,
&p->tcfa_lock, NULL, est);
if (err)
- goto err4;
+ goto err3;
}
p->idrinfo = idrinfo;
@@ -420,8 +407,6 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, struct nlattr *est,
INIT_LIST_HEAD(&p->list);
*a = p;
return 0;
-err4:
- idr_remove(idr, index);
err3:
free_percpu(p->cpu_qstats);
err2:
@@ -437,11 +422,78 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
struct tcf_idrinfo *idrinfo = tn->idrinfo;
spin_lock(&idrinfo->lock);
- idr_replace(&idrinfo->action_idr, a, a->tcfa_index);
+ /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
+ WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
spin_unlock(&idrinfo->lock);
}
EXPORT_SYMBOL(tcf_idr_insert);
+/* Cleanup idr index that was allocated but not initialized. */
+
+void tcf_idr_cleanup(struct tc_action_net *tn, u32 index)
+{
+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
+
+ spin_lock(&idrinfo->lock);
+ /* Remove ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
+ WARN_ON(!IS_ERR(idr_remove(&idrinfo->action_idr, index)));
+ spin_unlock(&idrinfo->lock);
+}
+EXPORT_SYMBOL(tcf_idr_cleanup);
+
+/* Check if action with specified index exists. If actions is found, increments
+ * its reference and bind counters, and return 1. Otherwise insert temporary
+ * error pointer (to prevent concurrent users from inserting actions with same
+ * index) and return 0.
+ */
+
+int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
+ struct tc_action **a, int bind)
+{
+ struct tcf_idrinfo *idrinfo = tn->idrinfo;
+ struct tc_action *p;
+ int ret;
+
+again:
+ spin_lock(&idrinfo->lock);
+ if (*index) {
+ p = idr_find(&idrinfo->action_idr, *index);
+ if (IS_ERR(p)) {
+ /* This means that another process allocated
+ * index but did not assign the pointer yet.
+ */
+ spin_unlock(&idrinfo->lock);
+ goto again;
+ }
+
+ if (p) {
+ refcount_inc(&p->tcfa_refcnt);
+ if (bind)
+ atomic_inc(&p->tcfa_bindcnt);
+ *a = p;
+ ret = 1;
+ } else {
+ *a = NULL;
+ ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index,
+ *index, GFP_ATOMIC);
+ if (!ret)
+ idr_replace(&idrinfo->action_idr,
+ ERR_PTR(-EBUSY), *index);
+ }
+ } else {
+ *index = 1;
+ *a = NULL;
+ ret = idr_alloc_u32(&idrinfo->action_idr, NULL, index,
+ UINT_MAX, GFP_ATOMIC);
+ if (!ret)
+ idr_replace(&idrinfo->action_idr, ERR_PTR(-EBUSY),
+ *index);
+ }
+ spin_unlock(&idrinfo->lock);
+ return ret;
+}
+EXPORT_SYMBOL(tcf_idr_check_alloc);
+
void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
struct tcf_idrinfo *idrinfo)
{
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index d3f4ac6f2c4b..06f743d8ed41 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -299,14 +299,17 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
parm = nla_data(tb[TCA_ACT_BPF_PARMS]);
- if (!tcf_idr_check(tn, parm->index, act, bind)) {
+ ret = tcf_idr_check_alloc(tn, &parm->index, act, bind);
+ if (!ret) {
ret = tcf_idr_create(tn, parm->index, est, act,
&act_bpf_ops, bind, true);
- if (ret < 0)
+ if (ret < 0) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
res = ACT_P_CREATED;
- } else {
+ } else if (ret > 0) {
/* Don't override defaults. */
if (bind)
return 0;
@@ -315,6 +318,8 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
tcf_idr_release(*act, bind);
return -EEXIST;
}
+ } else {
+ return ret;
}
is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 701e90244eff..1e31f0e448e2 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -118,11 +118,14 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
parm = nla_data(tb[TCA_CONNMARK_PARMS]);
- if (!tcf_idr_check(tn, parm->index, a, bind)) {
+ ret = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (!ret) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_connmark_ops, bind, false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ci = to_connmark(*a);
ci->tcf_action = parm->action;
@@ -131,7 +134,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
tcf_idr_insert(tn, *a);
ret = ACT_P_CREATED;
- } else {
+ } else if (ret > 0) {
ci = to_connmark(*a);
if (bind)
return 0;
@@ -142,6 +145,7 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
/* replacing action and zone */
ci->tcf_action = parm->action;
ci->zone = parm->zone;
+ ret = 0;
}
return ret;
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 5dbee136b0a1..bd232d3bd022 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -67,19 +67,24 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
return -EINVAL;
parm = nla_data(tb[TCA_CSUM_PARMS]);
- if (!tcf_idr_check(tn, parm->index, a, bind)) {
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (!err) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_csum_ops, bind, true);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
- } else {
+ } else if (err > 0) {
if (bind)/* dont override defaults */
return 0;
if (!ovr) {
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ } else {
+ return err;
}
p = to_tcf_csum(*a);
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 11c4de3f344e..661b72b9147d 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -91,19 +91,24 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
}
#endif
- if (!tcf_idr_check(tn, parm->index, a, bind)) {
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (!err) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_gact_ops, bind, true);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
- } else {
+ } else if (err > 0) {
if (bind)/* dont override defaults */
return 0;
if (!ovr) {
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ } else {
+ return err;
}
gact = to_gact(*a);
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index acea3feae762..a3eef00cd711 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -484,7 +484,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
if (!p)
return -ENOMEM;
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind) {
kfree(p);
return 0;
@@ -494,6 +497,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
ret = tcf_idr_create(tn, parm->index, est, a, &act_ife_ops,
bind, true);
if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
kfree(p);
return ret;
}
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 85e85dfba401..0dc787a57798 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -119,13 +119,18 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
if (tb[TCA_IPT_INDEX] != NULL)
index = nla_get_u32(tb[TCA_IPT_INDEX]);
- exists = tcf_idr_check(tn, index, a, bind);
+ err = tcf_idr_check_alloc(tn, &index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
if (tb[TCA_IPT_HOOK] == NULL || tb[TCA_IPT_TARG] == NULL) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, index);
return -EINVAL;
}
@@ -133,14 +138,18 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
if (nla_len(tb[TCA_IPT_TARG]) < td->u.target_size) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, index);
return -EINVAL;
}
if (!exists) {
ret = tcf_idr_create(tn, index, est, a, ops, bind,
false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, index);
return ret;
+ }
ret = ACT_P_CREATED;
} else {
if (bind)/* dont override defaults */
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index e08aed06d7f8..6afd89a36c69 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -79,7 +79,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
struct tcf_mirred *m;
struct net_device *dev;
bool exists = false;
- int ret;
+ int ret, err;
if (!nla) {
NL_SET_ERR_MSG_MOD(extack, "Mirred requires attributes to be passed");
@@ -94,7 +94,10 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
}
parm = nla_data(tb[TCA_MIRRED_PARMS]);
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
@@ -107,6 +110,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
default:
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
NL_SET_ERR_MSG_MOD(extack, "Unknown mirred option");
return -EINVAL;
}
@@ -115,6 +120,8 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
if (dev == NULL) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -ENODEV;
}
mac_header_xmit = dev_is_mac_header_xmit(dev);
@@ -124,13 +131,16 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
if (!exists) {
if (!dev) {
+ tcf_idr_cleanup(tn, parm->index);
NL_SET_ERR_MSG_MOD(extack, "Specified device does not exist");
return -EINVAL;
}
ret = tcf_idr_create(tn, parm->index, est, a,
&act_mirred_ops, bind, true);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
} else if (!ovr) {
tcf_idr_release(*a, bind);
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 1f91e8e66c0f..4dd9188a72fd 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -57,19 +57,24 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
return -EINVAL;
parm = nla_data(tb[TCA_NAT_PARMS]);
- if (!tcf_idr_check(tn, parm->index, a, bind)) {
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (!err) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_nat_ops, bind, false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
- } else {
+ } else if (err > 0) {
if (bind)
return 0;
if (!ovr) {
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ } else {
+ return err;
}
p = to_tcf_nat(*a);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 3a0e2f762f4e..cc8ffcd1ddb5 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -173,16 +173,20 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
if (IS_ERR(keys_ex))
return PTR_ERR(keys_ex);
- if (!tcf_idr_check(tn, parm->index, a, bind)) {
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (!err) {
if (!parm->nkeys) {
+ tcf_idr_cleanup(tn, parm->index);
NL_SET_ERR_MSG_MOD(extack, "Pedit requires keys to be passed");
ret = -EINVAL;
goto out_free;
}
ret = tcf_idr_create(tn, parm->index, est, a,
&act_pedit_ops, bind, false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
goto out_free;
+ }
p = to_pedit(*a);
keys = kmalloc(ksize, GFP_KERNEL);
if (!keys) {
@@ -191,7 +195,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
goto out_free;
}
ret = ACT_P_CREATED;
- } else {
+ } else if (err > 0) {
if (bind)
goto out_free;
if (!ovr) {
@@ -207,6 +211,8 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
goto out_free;
}
}
+ } else {
+ return err;
}
spin_lock_bh(&p->tcf_lock);
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 99335cca739e..1f3192ea8df7 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -101,15 +101,20 @@ static int tcf_act_police_init(struct net *net, struct nlattr *nla,
return -EINVAL;
parm = nla_data(tb[TCA_POLICE_TBF]);
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
if (!exists) {
ret = tcf_idr_create(tn, parm->index, NULL, a,
&act_police_ops, bind, false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
} else if (!ovr) {
tcf_idr_release(*a, bind);
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index a8582e1347db..3079e7be5bde 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -46,7 +46,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
struct tc_sample *parm;
struct tcf_sample *s;
bool exists = false;
- int ret;
+ int ret, err;
if (!nla)
return -EINVAL;
@@ -59,15 +59,20 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
parm = nla_data(tb[TCA_SAMPLE_PARMS]);
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
if (!exists) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_sample_ops, bind, false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
} else if (!ovr) {
tcf_idr_release(*a, bind);
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 2da47c682a30..aa51152e0066 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -100,21 +100,28 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
return -EINVAL;
parm = nla_data(tb[TCA_DEF_PARMS]);
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
if (tb[TCA_DEF_DATA] == NULL) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -EINVAL;
}
if (!exists) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_simp_ops, bind, false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
d = to_defact(*a);
ret = alloc_defdata(d, tb[TCA_DEF_DATA]);
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 4616a2c1821f..86521a74ecdd 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -152,21 +152,28 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
if (!flags) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -EINVAL;
}
if (!exists) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_skbedit_ops, bind, false);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
d = to_skbedit(*a);
ret = ACT_P_CREATED;
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index e844381af066..cdc6bacfb190 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -128,21 +128,28 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
if (parm->flags & SKBMOD_F_SWAPMAC)
lflags = SKBMOD_F_SWAPMAC;
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
if (!lflags) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -EINVAL;
}
if (!exists) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_skbmod_ops, bind, true);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
} else if (!ovr) {
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index ab5bf5c13f87..3ec585d58762 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -237,7 +237,10 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
}
parm = nla_data(tb[TCA_TUNNEL_KEY_PARMS]);
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
@@ -325,7 +328,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
&act_tunnel_key_ops, bind, true);
if (ret) {
NL_SET_ERR_MSG(extack, "Cannot create TC IDR");
- return ret;
+ goto err_out;
}
ret = ACT_P_CREATED;
@@ -364,6 +367,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
err_out:
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return ret;
}
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 9b600faaccbb..ad37f308175a 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -134,7 +134,10 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
if (!tb[TCA_VLAN_PARMS])
return -EINVAL;
parm = nla_data(tb[TCA_VLAN_PARMS]);
- exists = tcf_idr_check(tn, parm->index, a, bind);
+ err = tcf_idr_check_alloc(tn, &parm->index, a, bind);
+ if (err < 0)
+ return err;
+ exists = err;
if (exists && bind)
return 0;
@@ -146,12 +149,16 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
if (!tb[TCA_VLAN_PUSH_VLAN_ID]) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -EINVAL;
}
push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
if (push_vid >= VLAN_VID_MASK) {
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -ERANGE;
}
@@ -164,6 +171,8 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
default:
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -EPROTONOSUPPORT;
}
} else {
@@ -176,6 +185,8 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
default:
if (exists)
tcf_idr_release(*a, bind);
+ else
+ tcf_idr_cleanup(tn, parm->index);
return -EINVAL;
}
action = parm->v_action;
@@ -183,8 +194,10 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
if (!exists) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_vlan_ops, bind, true);
- if (ret)
+ if (ret) {
+ tcf_idr_cleanup(tn, parm->index);
return ret;
+ }
ret = ACT_P_CREATED;
} else if (!ovr) {
--
2.7.5
^ permalink raw reply related
* Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case
From: John Fastabend @ 2018-07-05 14:41 UTC (permalink / raw)
To: Daniel Borkmann, ast; +Cc: netdev, kafai
In-Reply-To: <2a083803-c830-b911-b8fd-d4dd0f560b56@iogearbox.net>
On 07/03/2018 07:40 AM, Daniel Borkmann wrote:
> On 06/30/2018 03:51 PM, John Fastabend wrote:
>> The current code, in the error path of sock_hash_ctx_update_elem,
>> checks if the sock has a psock in the user data and if so decrements
>> the reference count of the psock. However, if the error happens early
>> in the error path we may have never incremented the psock reference
>> count and if the psock exists because the sock is in another map then
>> we may inadvertently decrement the reference count.
>>
>> Fix this by making the error path only call smap_release_sock if the
>> error happens after the increment.
>>
>> Reported-by: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com
>> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> ---
[...]
>> @@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
>> if (err)
>> goto err;
>>
>> - /* bpf_map_update_elem() can be called in_irq() */
>> + psock = smap_psock_sk(sock);
>> + if (unlikely(!psock)) {
>> + err = -EINVAL;
>> + goto err;
>> + }
>
> Is an error even possible at this point? If __sock_map_ctx_update_elem() succeeds,
> we either allocated and linked a new psock to the sock or we inc'ed the existing
> one's refcount. From my reading it seems we should always succeed the subsequent
> smap_psock_sk(). If we would have failed here in between it would mean we'd have
> a refcount imbalance somewhere?
>
Its not possible will replace with a comment. Thanks.
^ permalink raw reply
* [PATCH net-next 0/2] IP listification follow-ups
From: Edward Cree @ 2018-07-05 14:45 UTC (permalink / raw)
To: davem; +Cc: netdev
While working on IPv6 list processing, I found another bug in the IPv4
version. So this patch series has that fix, and the IPv6 version with
both fixes incorporated.
Edward Cree (2):
net: ipv4: fix list processing on L3 slave devices
net: ipv6: listify ipv6_rcv() and ip6_rcv_finish()
include/net/ipv6.h | 2 +
net/ipv4/ip_input.c | 23 +++++----
net/ipv6/af_inet6.c | 1 +
net/ipv6/ip6_input.c | 131 ++++++++++++++++++++++++++++++++++++++++++++-------
4 files changed, 133 insertions(+), 24 deletions(-)
^ permalink raw reply
* [PATCH net-next 1/2] net: ipv4: fix list processing on L3 slave devices
From: Edward Cree @ 2018-07-05 14:47 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <8ab5939a-c34d-1b39-5416-f595bcd86a12@solarflare.com>
If we have an L3 master device, l3mdev_ip_rcv() will steal the skb, but
we were returning NET_RX_SUCCESS from ip_rcv_finish_core() which meant
that ip_list_rcv_finish() would keep it on the list. Instead let's
move the l3mdev_ip_rcv() call into the caller, so that our response to
a steal can be different in the single packet path (return
NET_RX_SUCCESS) and the list path (forget this packet and continue).
Fixes: 5fa12739a53d net: ipv4: listify ip_rcv_finish
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
net/ipv4/ip_input.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 14ba628b2761..1a3b6f32b1c9 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -316,13 +316,6 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
struct rtable *rt;
int err;
- /* if ingress device is enslaved to an L3 master device pass the
- * skb to its handler for processing
- */
- skb = l3mdev_ip_rcv(skb);
- if (!skb)
- return NET_RX_SUCCESS;
-
if (net->ipv4.sysctl_ip_early_demux &&
!skb_dst(skb) &&
!skb->sk &&
@@ -408,8 +401,16 @@ static int ip_rcv_finish_core(struct net *net, struct sock *sk,
static int ip_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
{
- int ret = ip_rcv_finish_core(net, sk, skb);
+ int ret;
+
+ /* if ingress device is enslaved to an L3 master device pass the
+ * skb to its handler for processing
+ */
+ skb = l3mdev_ip_rcv(skb);
+ if (!skb)
+ return NET_RX_SUCCESS;
+ ret = ip_rcv_finish_core(net, sk, skb);
if (ret != NET_RX_DROP)
ret = dst_input(skb);
return ret;
@@ -545,6 +546,12 @@ static void ip_list_rcv_finish(struct net *net, struct sock *sk,
struct dst_entry *dst;
list_del(&skb->list);
+ /* if ingress device is enslaved to an L3 master device pass the
+ * skb to its handler for processing
+ */
+ skb = l3mdev_ip_rcv(skb);
+ if (!skb)
+ continue;
if (ip_rcv_finish_core(net, sk, skb) == NET_RX_DROP)
continue;
^ permalink raw reply related
* [PATCH net-next 2/2] net: ipv6: listify ipv6_rcv() and ip6_rcv_finish()
From: Edward Cree @ 2018-07-05 14:49 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <8ab5939a-c34d-1b39-5416-f595bcd86a12@solarflare.com>
Essentially the same as the ipv4 equivalents.
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
include/net/ipv6.h | 2 +
net/ipv6/af_inet6.c | 1 +
net/ipv6/ip6_input.c | 131 ++++++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 118 insertions(+), 16 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..b7843e0b16ee 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -922,6 +922,8 @@ static inline __be32 flowi6_get_flowlabel(const struct flowi6 *fl6)
int ipv6_rcv(struct sk_buff *skb, struct net_device *dev,
struct packet_type *pt, struct net_device *orig_dev);
+void ipv6_list_rcv(struct list_head *head, struct packet_type *pt,
+ struct net_device *orig_dev);
int ip6_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index 9ed0eae91758..c9535354149f 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -764,6 +764,7 @@ EXPORT_SYMBOL_GPL(ipv6_opt_accepted);
static struct packet_type ipv6_packet_type __read_mostly = {
.type = cpu_to_be16(ETH_P_IPV6),
.func = ipv6_rcv,
+ .list_func = ipv6_list_rcv,
};
static int __init ipv6_packet_init(void)
diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index f08d34491ece..6242682be876 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -47,17 +47,11 @@
#include <net/inet_ecn.h>
#include <net/dst_metadata.h>
-int ip6_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
+static void ip6_rcv_finish_core(struct net *net, struct sock *sk,
+ struct sk_buff *skb)
{
void (*edemux)(struct sk_buff *skb);
- /* if ingress device is enslaved to an L3 master device pass the
- * skb to its handler for processing
- */
- skb = l3mdev_ip6_rcv(skb);
- if (!skb)
- return NET_RX_SUCCESS;
-
if (net->ipv4.sysctl_ip_early_demux && !skb_dst(skb) && skb->sk == NULL) {
const struct inet6_protocol *ipprot;
@@ -67,20 +61,73 @@ int ip6_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
}
if (!skb_valid_dst(skb))
ip6_route_input(skb);
+}
+
+int ip6_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
+{
+ /* if ingress device is enslaved to an L3 master device pass the
+ * skb to its handler for processing
+ */
+ skb = l3mdev_ip6_rcv(skb);
+ if (!skb)
+ return NET_RX_SUCCESS;
+ ip6_rcv_finish_core(net, sk, skb);
return dst_input(skb);
}
-int ipv6_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev)
+static void ip6_sublist_rcv_finish(struct list_head *head)
+{
+ struct sk_buff *skb, *next;
+
+ list_for_each_entry_safe(skb, next, head, list)
+ dst_input(skb);
+}
+
+static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
+ struct list_head *head)
+{
+ struct dst_entry *curr_dst = NULL;
+ struct sk_buff *skb, *next;
+ struct list_head sublist;
+
+ INIT_LIST_HEAD(&sublist);
+ list_for_each_entry_safe(skb, next, head, list) {
+ struct dst_entry *dst;
+
+ list_del(&skb->list);
+ /* if ingress device is enslaved to an L3 master device pass the
+ * skb to its handler for processing
+ */
+ skb = l3mdev_ip6_rcv(skb);
+ if (!skb)
+ continue;
+ ip6_rcv_finish_core(net, sk, skb);
+ dst = skb_dst(skb);
+ if (curr_dst != dst) {
+ /* dispatch old sublist */
+ if (!list_empty(&sublist))
+ ip6_sublist_rcv_finish(&sublist);
+ /* start new sublist */
+ INIT_LIST_HEAD(&sublist);
+ curr_dst = dst;
+ }
+ list_add_tail(&skb->list, &sublist);
+ }
+ /* dispatch final sublist */
+ ip6_sublist_rcv_finish(&sublist);
+}
+
+static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
+ struct net *net)
{
const struct ipv6hdr *hdr;
u32 pkt_len;
struct inet6_dev *idev;
- struct net *net = dev_net(skb->dev);
if (skb->pkt_type == PACKET_OTHERHOST) {
kfree_skb(skb);
- return NET_RX_DROP;
+ return NULL;
}
rcu_read_lock();
@@ -196,7 +243,7 @@ int ipv6_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt
if (ipv6_parse_hopopts(skb) < 0) {
__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
rcu_read_unlock();
- return NET_RX_DROP;
+ return NULL;
}
}
@@ -205,15 +252,67 @@ int ipv6_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt
/* Must drop socket now because of tproxy. */
skb_orphan(skb);
- return NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING,
- net, NULL, skb, dev, NULL,
- ip6_rcv_finish);
+ return skb;
err:
__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
drop:
rcu_read_unlock();
kfree_skb(skb);
- return NET_RX_DROP;
+ return NULL;
+}
+
+int ipv6_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, struct net_device *orig_dev)
+{
+ struct net *net = dev_net(skb->dev);
+
+ skb = ip6_rcv_core(skb, dev, net);
+ if (skb == NULL)
+ return NET_RX_DROP;
+ return NF_HOOK(NFPROTO_IPV6, NF_INET_PRE_ROUTING,
+ net, NULL, skb, dev, NULL,
+ ip6_rcv_finish);
+}
+
+static void ip6_sublist_rcv(struct list_head *head, struct net_device *dev,
+ struct net *net)
+{
+ NF_HOOK_LIST(NFPROTO_IPV6, NF_INET_PRE_ROUTING, net, NULL,
+ head, dev, NULL, ip6_rcv_finish);
+ ip6_list_rcv_finish(net, NULL, head);
+}
+
+/* Receive a list of IPv6 packets */
+void ipv6_list_rcv(struct list_head *head, struct packet_type *pt,
+ struct net_device *orig_dev)
+{
+ struct net_device *curr_dev = NULL;
+ struct net *curr_net = NULL;
+ struct sk_buff *skb, *next;
+ struct list_head sublist;
+
+ INIT_LIST_HEAD(&sublist);
+ list_for_each_entry_safe(skb, next, head, list) {
+ struct net_device *dev = skb->dev;
+ struct net *net = dev_net(dev);
+
+ list_del(&skb->list);
+ skb = ip6_rcv_core(skb, dev, net);
+ if (skb == NULL)
+ continue;
+
+ if (curr_dev != dev || curr_net != net) {
+ /* dispatch old sublist */
+ if (!list_empty(&sublist))
+ ip6_sublist_rcv(&sublist, curr_dev, curr_net);
+ /* start new sublist */
+ INIT_LIST_HEAD(&sublist);
+ curr_dev = dev;
+ curr_net = net;
+ }
+ list_add_tail(&skb->list, &sublist);
+ }
+ /* dispatch final sublist */
+ ip6_sublist_rcv(&sublist, curr_dev, curr_net);
}
/*
^ permalink raw reply related
* [bpf PATCH v2 0/2] sockmap, syzbot fix error path and RCU fix
From: John Fastabend @ 2018-07-05 15:05 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, john.fastabend
I missed fixing the error path in the sockhash code to align with
supporting socks in multiple maps. Simply checking if the psock is
present does not mean we can decrement the reference count because
it could be part of another map. Fix this by cleaning up the error
path so this situation does not happen.
---
John Fastabend (2):
bpf: sockmap, error path can not release psock in multi-map case
bpf: sockmap, hash table is RCU so readers do not need locks
kernel/bpf/sockmap.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
^ permalink raw reply
* [bpf PATCH v2 1/2] bpf: sockmap, error path can not release psock in multi-map case
From: John Fastabend @ 2018-07-05 15:05 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, john.fastabend
In-Reply-To: <20180705150506.9055.81845.stgit@john-Precision-Tower-5810>
The current code, in the error path of sock_hash_ctx_update_elem,
checks if the sock has a psock in the user data and if so decrements
the reference count of the psock. However, if the error happens early
in the error path we may have never incremented the psock reference
count and if the psock exists because the sock is in another map then
we may inadvertently decrement the reference count.
Fix this by making the error path only call smap_release_sock if the
error happens after the increment.
Reported-by: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/sockmap.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index cf7b6a6..3847a7c 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -1896,7 +1896,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
e = kzalloc(sizeof(*e), GFP_ATOMIC | __GFP_NOWARN);
if (!e) {
err = -ENOMEM;
- goto out_progs;
+ goto out_free;
}
}
@@ -2342,7 +2342,10 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
if (err)
goto err;
- /* bpf_map_update_elem() can be called in_irq() */
+ /* psock is valid here because otherwise above *ctx_update_elem would
+ * have thrown an error. It is safe to skip error check.
+ */
+ psock = smap_psock_sk(sock);
raw_spin_lock_bh(&b->lock);
l_old = lookup_elem_raw(head, hash, key, key_size);
if (l_old && map_flags == BPF_NOEXIST) {
@@ -2360,12 +2363,6 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
goto bucket_err;
}
- psock = smap_psock_sk(sock);
- if (unlikely(!psock)) {
- err = -EINVAL;
- goto bucket_err;
- }
-
rcu_assign_pointer(e->hash_link, l_new);
rcu_assign_pointer(e->htab,
container_of(map, struct bpf_htab, map));
@@ -2388,12 +2385,10 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
raw_spin_unlock_bh(&b->lock);
return 0;
bucket_err:
+ smap_release_sock(psock, sock);
raw_spin_unlock_bh(&b->lock);
err:
kfree(e);
- psock = smap_psock_sk(sock);
- if (psock)
- smap_release_sock(psock, sock);
return err;
}
^ permalink raw reply related
* [bpf PATCH v2 2/2] bpf: sockmap, hash table is RCU so readers do not need locks
From: John Fastabend @ 2018-07-05 15:06 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, john.fastabend
In-Reply-To: <20180705150506.9055.81845.stgit@john-Precision-Tower-5810>
This removes locking from readers of RCU hash table. Its not
necessary.
Fixes: 81110384441a ("bpf: sockmap, add hash map support")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
kernel/bpf/sockmap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 3847a7c..00fb2e3 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -2467,10 +2467,8 @@ struct sock *__sock_hash_lookup_elem(struct bpf_map *map, void *key)
b = __select_bucket(htab, hash);
head = &b->head;
- raw_spin_lock_bh(&b->lock);
l = lookup_elem_raw(head, hash, key, key_size);
sk = l ? l->sk : NULL;
- raw_spin_unlock_bh(&b->lock);
return sk;
}
^ permalink raw reply related
* [PATCH v2 iproute2] man: Fix typos on tc-cbs
From: Jesus Sanchez-Palencia @ 2018-07-05 15:20 UTC (permalink / raw)
To: netdev; +Cc: simon.horman, Jesus Sanchez-Palencia
Fix 2 typos on the man page of the CBS qdisc.
Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
man/man8/tc-cbs.8 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/man/man8/tc-cbs.8 b/man/man8/tc-cbs.8
index 32e1e0d4..ad1d8821 100644
--- a/man/man8/tc-cbs.8
+++ b/man/man8/tc-cbs.8
@@ -28,7 +28,7 @@ defined rate limiting method to the traffic.
This queueing discipline is intended to be used by TSN (Time Sensitive
Networking) applications, the CBS parameters are derived directly by
what is described by the Annex L of the IEEE 802.1Q-2014
-Sepcification. The algorithm and how it affects the latency are
+Specification. The algorithm and how it affects the latency are
detailed there.
CBS is meant to be installed under another qdisc that maps packet
@@ -60,7 +60,7 @@ packet size, which is then used for calculating the idleslope.
sendslope
Sendslope is the rate of credits that is depleted (it should be a
negative number of kilobits per second) when a transmission is
-ocurring. It can be calculated as follows, (IEEE 802.1Q-2014 Section
+occurring. It can be calculated as follows, (IEEE 802.1Q-2014 Section
8.6.8.2 item g):
sendslope = idleslope - port_transmit_rate
--
2.18.0
^ permalink raw reply related
* Re: [RFC PATCH 00/11] OVS eBPF datapath.
From: William Tu @ 2018-07-05 15:32 UTC (permalink / raw)
To: Paul Chaignon
Cc: Alexei Starovoitov,
<dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
Tom Herbert via iovisor-dev, Linux Kernel Network Developers
In-Reply-To: <20180705121202.GA27625@Nover>
>
> d71962f ("bpf: allow map helpers access to map values directly") removes
> that limitation from the verifier and should allow you to use map values
> as map keys directly. 4.18-rc1 has it.
>
>> Thanks
>> William
Hi Paul,
Thanks a lot! This is very helpful.
I'm testing it now, works great so far, and saves lots of bpf stack space.
Regards,
William
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#1361): https://lists.iovisor.org/g/iovisor-dev/message/1361
Mute This Topic: https://lists.iovisor.org/mt/22656941/1132507
Group Owner: iovisor-dev+owner-9jONkmmOlFHEE9lA1F8Ukti2O/JbrIOy@public.gmane.org
Unsubscribe: https://lists.iovisor.org/g/iovisor-dev/unsub [glki-iovisor-dev@m.gmane.org]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply
* [bpf PATCH 0/4] sockhash/sockmap fixes
From: John Fastabend @ 2018-07-05 15:36 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, john.fastabend
First three patches resolve issues found while testing sockhash and
reviewing code. Syzbot also found them about the same time as I was
working on fixes. The main issue is in the sockhash path we reduced
the scope of sk_callback lock but this meant we could get update and
close running in parallel so fix that here.
Then testing sk_msg and sk_skb programs together found that skb->dev
is not always assigned and some of the helpers were depending on this
to lookup max mtu. Fix this by using SKB_MAX_ALLOC when no MTU is
available.
Finally, Martin spotted that the sockmap code was still using the
qdisc skb cb structure. But I was sure we had fixed this long ago.
Looks like we missed it in a merge conflict resolution and then by
chance data_end offset was the same in both structures so everything
sort of continued to work even though it could break at any moment
if the structs ever change. So redo the conversion and this time
also convert the helpers.
---
John Fastabend (4):
bpf: fix sk_skb programs without skb->dev assigned
bpf: sockhash, disallow bpf_tcp_close and update in parallel
bpf: sockmap, consume_skb in close path
bpf: sockmap, convert bpf_compute_data_pointers to bpf_*_sk_skb
0 files changed
^ permalink raw reply
* [bpf PATCH 1/4] bpf: fix sk_skb programs without skb->dev assigned
From: John Fastabend @ 2018-07-05 15:36 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, john.fastabend
In-Reply-To: <20180705152803.15312.47795.stgit@john-Precision-Tower-5810>
Multiple BPF helpers in use by sk_skb programs calculate the max
skb length using the __bpf_skb_max_len function. However, this
calculates the max length using the skb->dev pointer which can be
NULL when an sk_skb program is paired with an sk_msg program.
To force this a sk_msg program needs to redirect into the ingress
path of a sock with an attach sk_skb program. Then the the sk_skb
program would need to call one of the helpers that adjust the skb
size.
To fix the null ptr dereference use SKB_MAX_ALLOC size if no dev
is available.
Fixes: 8934ce2fd081 ("bpf: sockmap redirect ingress support")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
0 files changed
diff --git a/net/core/filter.c b/net/core/filter.c
index 0ca6907..3095f1b 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2779,7 +2779,8 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 len_diff)
static u32 __bpf_skb_max_len(const struct sk_buff *skb)
{
- return skb->dev->mtu + skb->dev->hard_header_len;
+ return skb->dev ? skb->dev->mtu + skb->dev->hard_header_len :
+ SKB_MAX_ALLOC;
}
static int bpf_skb_adjust_net(struct sk_buff *skb, s32 len_diff)
^ permalink raw reply related
* [bpf PATCH 2/4] bpf: sockhash, disallow bpf_tcp_close and update in parallel
From: John Fastabend @ 2018-07-05 15:36 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, john.fastabend
In-Reply-To: <20180705152803.15312.47795.stgit@john-Precision-Tower-5810>
After latest lock updates there is no longer anything preventing a
close and recvmsg call running in parallel. Additionally, we can
race update with close if we close a socket and simultaneously update
if via the BPF userspace API (note the cgroup ops are already run
with sock_lock held).
To resolve this take sock_lock in close and update paths.
Reported-by: syzbot+b680e42077a0d7c9a0c4@syzkaller.appspotmail.com
Fixes: e9db4ef6bf4c ("bpf: sockhash fix omitted bucket lock in sock_close")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
0 files changed
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 00fb2e3..9c67e96 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -312,10 +312,12 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
struct smap_psock *psock;
struct sock *osk;
+ lock_sock(sk);
rcu_read_lock();
psock = smap_psock_sk(sk);
if (unlikely(!psock)) {
rcu_read_unlock();
+ release_sock(sk);
return sk->sk_prot->close(sk, timeout);
}
@@ -371,6 +373,7 @@ static void bpf_tcp_close(struct sock *sk, long timeout)
e = psock_map_pop(sk, psock);
}
rcu_read_unlock();
+ release_sock(sk);
close_fun(sk, timeout);
}
@@ -2069,7 +2072,13 @@ static int sock_map_update_elem(struct bpf_map *map,
return -EOPNOTSUPP;
}
+ lock_sock(skops.sk);
+ preempt_disable();
+ rcu_read_lock();
err = sock_map_ctx_update_elem(&skops, map, key, flags);
+ rcu_read_unlock();
+ preempt_enable();
+ release_sock(skops.sk);
fput(socket->file);
return err;
}
@@ -2410,7 +2419,13 @@ static int sock_hash_update_elem(struct bpf_map *map,
return -EINVAL;
}
+ lock_sock(skops.sk);
+ preempt_disable();
+ rcu_read_lock();
err = sock_hash_ctx_update_elem(&skops, map, key, flags);
+ rcu_read_unlock();
+ preempt_enable();
+ release_sock(skops.sk);
fput(socket->file);
return err;
}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index d10ecd7..a31a1ba 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -735,7 +735,9 @@ static int map_update_elem(union bpf_attr *attr)
if (bpf_map_is_dev_bound(map)) {
err = bpf_map_offload_update_elem(map, key, value, attr->flags);
goto out;
- } else if (map->map_type == BPF_MAP_TYPE_CPUMAP) {
+ } else if (map->map_type == BPF_MAP_TYPE_CPUMAP ||
+ map->map_type == BPF_MAP_TYPE_SOCKHASH ||
+ map->map_type == BPF_MAP_TYPE_SOCKMAP) {
err = map->ops->map_update_elem(map, key, value, attr->flags);
goto out;
}
^ permalink raw reply related
* [bpf PATCH 3/4] bpf: sockmap, consume_skb in close path
From: John Fastabend @ 2018-07-05 15:36 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, john.fastabend
In-Reply-To: <20180705152803.15312.47795.stgit@john-Precision-Tower-5810>
Currently, when a sock is closed and the bpf_tcp_close() callback is
used we remove memory but do not free the skb. Call consume_skb() if
the skb is attached to the buffer.
Reported-by: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com
Fixes: 1aa12bdf1bfb ("bpf: sockmap, add sock close() hook to remove socks")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
0 files changed
diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
index 9c67e96..dfc8a8a 100644
--- a/kernel/bpf/sockmap.c
+++ b/kernel/bpf/sockmap.c
@@ -571,7 +571,8 @@ static int free_sg(struct sock *sk, int start, struct sk_msg_buff *md)
while (sg[i].length) {
free += sg[i].length;
sk_mem_uncharge(sk, sg[i].length);
- put_page(sg_page(&sg[i]));
+ if (!md->skb)
+ put_page(sg_page(&sg[i]));
sg[i].length = 0;
sg[i].page_link = 0;
sg[i].offset = 0;
@@ -580,6 +581,8 @@ static int free_sg(struct sock *sk, int start, struct sk_msg_buff *md)
if (i == MAX_SKB_FRAGS)
i = 0;
}
+ if (md->skb)
+ consume_skb(md->skb);
return free;
}
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox