On Wed 16 May 2018 at 08:56, Jiri Pirko <jiri@xxxxxxxxxxx> wrote:
> Wed, May 16, 2018 at 10:16:13AM CEST, vladbu@xxxxxxxxxxxx wrote:
>>
>>On Wed 16 May 2018 at 07:50, Jiri Pirko <jiri@xxxxxxxxxxx> wrote:
>>> Mon, May 14, 2018 at 04:27:11PM CEST, vladbu@xxxxxxxxxxxx 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@xxxxxxxxxxxx>
>>>>---
>>>> 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?
>>
>>Original idr remove function used _bh version so I used it here as well.
>>As I already replied to your previous question about idrinfo lock usage,
>>I don't see any particular reason for locking with _bh at this point.
>>I've contacted the author(Chris Mi) and he said that he just preserved
>>locking the same way as it was before he changed hash table to idr for
>>action lookup.
>>
>>You want me to do standalone patch that cleans up idrinfo locking?
>
> Yes please. You can send it separately, not as a part of this
> patchset.
Okay.
>
>
>
>>
>>>
>>>
>>>>+ 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...
>>
>>This is what original code returned so I decided to preserve
>>compatibility.
>
> Okay.
>
>
>>
>>>
>>>
>>>>+ }
>>>>+
>>>>+ 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?
>>
>>When idr replace returns non-NULL pointer, which means that somehow
>>concurrent insertion of action with same index has happened and we are
>>leaking memory.
>
> Is that possible to happen? Meaning, can I as a user cause this by doing
> something in a wrong/unexpected way?
No, it shouldn't be possible unless there is a race condition.
Otherwise I would put some proper error handling code there.
>
>
>>
>>By the way I'm still not sure if having this insert unique function is
>>warranted or I should just add WARN to regular idr insert. What is your
>>opinion on this?
>
> I have to check where you use this.
Every action init function uses this.
>
>
>>
>>>
>>>
>>>>+ 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
>>>>
>>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
