linux-kernel - Re: [PATCH 10/14] net: sched: extend act API for lockless actions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180516075000.GC1972@nanopsycho>
Date:   Wed, 16 May 2018 09:50:00 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jhs@...atatu.com,
        xiyou.wangcong@...il.com, pablo@...filter.org,
        kadlec@...ckhole.kfki.hu, fw@...len.de, ast@...nel.org,
        daniel@...earbox.net, edumazet@...gle.com, keescook@...omium.org,
        linux-kernel@...r.kernel.org, netfilter-devel@...r.kernel.org,
        coreteam@...filter.org, kliteyn@...lanox.com
Subject: Re: [PATCH 10/14] net: sched: extend act API for lockless actions

Mon, May 14, 2018 at 04:27:11PM CEST, vladbu@...lanox.com wrote:
>Implement new action API function to atomically delete action with
>specified index and to atomically insert unique action. These functions are
>required to implement init and delete functions for specific actions that
>do not rely on rtnl lock.
>
>Signed-off-by: Vlad Buslov <vladbu@...lanox.com>
>---
> include/net/act_api.h |  2 ++
> net/sched/act_api.c   | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
>diff --git a/include/net/act_api.h b/include/net/act_api.h
>index a8c8570..bce0cf1 100644
>--- a/include/net/act_api.h
>+++ b/include/net/act_api.h
>@@ -153,7 +153,9 @@ 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);
> void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a);
> 
>+int tcf_idr_find_delete(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 2772276e..a5193dc 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -330,6 +330,41 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, struct tc_action **a,
> }
> EXPORT_SYMBOL(tcf_idr_check);
> 
>+int tcf_idr_find_delete(struct tc_action_net *tn, u32 index)
>+{
>+	struct tcf_idrinfo *idrinfo = tn->idrinfo;
>+	struct tc_action *p;
>+	int ret = 0;
>+
>+	spin_lock_bh(&idrinfo->lock);

Why "_bh" is needed here?


>+	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_bh(&idrinfo->lock);
>+
>+			tcf_action_cleanup(p);
>+			module_put(owner);
>+			return 0;
>+		}
>+		ret = 0;
>+	} else {
>+		ret = -EPERM;

I wonder if "-EPERM" is the best error code for this...


>+	}
>+
>+	spin_unlock_bh(&idrinfo->lock);
>+	return ret;
>+}
>+EXPORT_SYMBOL(tcf_idr_find_delete);
>+
> 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)
>@@ -407,6 +442,16 @@ void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
> }
> EXPORT_SYMBOL(tcf_idr_insert);
> 
>+void tcf_idr_insert_unique(struct tc_action_net *tn, struct tc_action *a)
>+{
>+	struct tcf_idrinfo *idrinfo = tn->idrinfo;
>+
>+	spin_lock_bh(&idrinfo->lock);
>+	WARN_ON(idr_replace(&idrinfo->action_idr, a, a->tcfa_index));

Under which condition this WARN_ON is hit?


>+	spin_unlock_bh(&idrinfo->lock);
>+}
>+EXPORT_SYMBOL(tcf_idr_insert_unique);
>+
> void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
> 			 struct tcf_idrinfo *idrinfo)
> {
>-- 
>2.7.5
>