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;
