netdev - Re: [PATCH 33/38] act_api: Convert action

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: <20190821203514.GA21442@bombadil.infradead.org>
Date:   Wed, 21 Aug 2019 13:35:14 -0700
From:   Matthew Wilcox <willy@...radead.org>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 33/38] act_api: Convert action_idr to XArray

On Wed, Aug 21, 2019 at 07:41:19PM +0000, Vlad Buslov wrote:
> > @@ -301,18 +292,18 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
> >  	if (nla_put_string(skb, TCA_KIND, ops->kind))
> >  		goto nla_put_failure;
> >  
> > -	mutex_lock(&idrinfo->lock);
> > -	idr_for_each_entry_ul(idr, p, tmp, id) {
> > +	xa_lock(&idrinfo->actions);
> > +	xa_for_each(&idrinfo->actions, index, p) {
> >  		ret = tcf_idr_release_unsafe(p);
> 
> I like the simplification of reusing builtin xarray spinlock for this,
> but the reason we are using mutex here is because the following call
> chain: tcf_idr_release_unsafe() -> tcf_action_cleanup() -> free_tcf() ->
> tcf_chain_put_by_act() -> __tcf_chain_put() -> mutex_lock(&block->lock);

Ah, this one is buggy anyway because we call xa_erase() inside the
loop, so it'll deadlock.

We could just drop the xa_lock() / xa_unlock() around the loop.  There's
no need to hold the lock unless we're trying to prevent other simultaneous
additions/deletions, which shouldn't be happening?  ie this:

@@ -268,8 +268,10 @@ static int tcf_idr_release_unsafe(struct tc_action *p)
        if (atomic_read(&p->tcfa_bindcnt) > 0)
                return -EPERM;
 
-       if (refcount_dec_and_test(&p->tcfa_refcnt)) {
-               xa_erase(&p->idrinfo->actions, p->tcfa_index);
+       if (refcount_dec_and_lock(&p->tcfa_refcnt,
+                                       &p->idrinfo->actions.xa_lock)) {
+               __xa_erase(&p->idrinfo->actions, p->tcfa_index);
+               xa_unlock(&p->idrinfo->actions);
                tcf_action_cleanup(p);
                return ACT_P_DELETED;
        }
@@ -292,18 +294,15 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
        if (nla_put_string(skb, TCA_KIND, ops->kind))
                goto nla_put_failure;
 
-       xa_lock(&idrinfo->actions);
        xa_for_each(&idrinfo->actions, index, p) {
                ret = tcf_idr_release_unsafe(p);
                if (ret == ACT_P_DELETED) {
                        module_put(ops->owner);
                        n_i++;
                } else if (ret < 0) {
-                       xa_unlock(&idrinfo->actions);
                        goto nla_put_failure;
                }
        }
-       xa_unlock(&idrinfo->actions);
 
        if (nla_put_u32(skb, TCA_FCNT, n_i))
                goto nla_put_failure;