| Message ID | 1526308035-12484-6-git-send-email-vladbu@mellanox.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Modify action API for implementing lockless actions | expand |
Mon, May 14, 2018 at 04:27:06PM CEST, vladbu@mellanox.com wrote: >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. > >Signed-off-by: Vlad Buslov <vladbu@mellanox.com> >--- > net/sched/act_api.c | 38 ++++++++++++++++---------------------- > 1 file changed, 16 insertions(+), 22 deletions(-) > >diff --git a/net/sched/act_api.c b/net/sched/act_api.c >index 1331beb..9459cce 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) >+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_bh(&idrinfo->lock); Why "_bh" variant is necessary here? > p = idr_find(&idrinfo->action_idr, index); >+ if (p) { >+ refcount_inc(&p->tcfa_refcnt); >+ if (bind) >+ atomic_inc(&p->tcfa_bindcnt); >+ } > spin_unlock_bh(&idrinfo->lock); [...] -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 14 May 2018 at 16:23, Jiri Pirko <jiri@resnulli.us> wrote: > Mon, May 14, 2018 at 04:27:06PM CEST, vladbu@mellanox.com wrote: >>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. >> >>Signed-off-by: Vlad Buslov <vladbu@mellanox.com> >>--- >> net/sched/act_api.c | 38 ++++++++++++++++---------------------- >> 1 file changed, 16 insertions(+), 22 deletions(-) >> >>diff --git a/net/sched/act_api.c b/net/sched/act_api.c >>index 1331beb..9459cce 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) >>+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_bh(&idrinfo->lock); > > Why "_bh" variant is necessary here? It is not my code. > >> p = idr_find(&idrinfo->action_idr, index); >>+ if (p) { >>+ refcount_inc(&p->tcfa_refcnt); >>+ if (bind) >>+ atomic_inc(&p->tcfa_bindcnt); >>+ } >> spin_unlock_bh(&idrinfo->lock); > > [...] -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vlad,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on net/master]
[also build test WARNING on v4.17-rc5 next-20180514]
[cannot apply to net-next/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Vlad-Buslov/Modify-action-API-for-implementing-lockless-actions/20180515-025420
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
net/sched/act_api.c:71:15: sparse: incorrect type in initializer (different address spaces) @@ expected struct tc_cookie [noderef] <asn:4>*__ret @@ got [noderef] <asn:4>*__ret @@
net/sched/act_api.c:71:15: expected struct tc_cookie [noderef] <asn:4>*__ret
net/sched/act_api.c:71:15: got struct tc_cookie *new_cookie
net/sched/act_api.c:71:13: sparse: incorrect type in assignment (different address spaces) @@ expected struct tc_cookie *old @@ got struct tc_cookie [noderef] <struct tc_cookie *old @@
net/sched/act_api.c:71:13: expected struct tc_cookie *old
net/sched/act_api.c:71:13: got struct tc_cookie [noderef] <asn:4>*[assigned] __ret
>> net/sched/act_api.c:287:6: sparse: symbol '__tcf_idr_check' was not declared. Should it be static?
net/sched/act_api.c:144:48: sparse: dereference of noderef expression
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mon, May 14, 2018 at 08:49:07PM CEST, vladbu@mellanox.com wrote: > >On Mon 14 May 2018 at 16:23, Jiri Pirko <jiri@resnulli.us> wrote: >> Mon, May 14, 2018 at 04:27:06PM CEST, vladbu@mellanox.com wrote: >>>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. >>> >>>Signed-off-by: Vlad Buslov <vladbu@mellanox.com> >>>--- >>> net/sched/act_api.c | 38 ++++++++++++++++---------------------- >>> 1 file changed, 16 insertions(+), 22 deletions(-) >>> >>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c >>>index 1331beb..9459cce 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) >>>+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_bh(&idrinfo->lock); >> >> Why "_bh" variant is necessary here? > >It is not my code. Yeah, yet still I wonder :) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 15 May 2018 at 08:58, Jiri Pirko <jiri@resnulli.us> wrote: > Mon, May 14, 2018 at 08:49:07PM CEST, vladbu@mellanox.com wrote: >> >>On Mon 14 May 2018 at 16:23, Jiri Pirko <jiri@resnulli.us> wrote: >>> Mon, May 14, 2018 at 04:27:06PM CEST, vladbu@mellanox.com wrote: >>>>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. >>>> >>>>Signed-off-by: Vlad Buslov <vladbu@mellanox.com> >>>>--- >>>> net/sched/act_api.c | 38 ++++++++++++++++---------------------- >>>> 1 file changed, 16 insertions(+), 22 deletions(-) >>>> >>>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c >>>>index 1331beb..9459cce 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) >>>>+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_bh(&idrinfo->lock); >>> >>> Why "_bh" variant is necessary here? >> >>It is not my code. > > Yeah, yet still I wonder :) I've been looking into it and I don't understand why it is used either. Looking at the commit history I see that long time ago rw_lock was used instead of spinlock, and some actions implemented they own lookup methods. So it seems that it might have been used to protect lookups running in bh context. However, in current architecture classifiers hold direct reference to action so action lookup is not used when processing packets. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 1331beb..9459cce 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) +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_bh(&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_bh(&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);
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. Signed-off-by: Vlad Buslov <vladbu@mellanox.com> --- net/sched/act_api.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-)