[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAAFAkD8CYey6inOuxv7doWBaro-rpwcLXyDjbte=VMKb-YRKcg@mail.gmail.com>
Date: Thu, 26 Jan 2023 10:28:49 -0500
From: Jamal Hadi Salim <hadi@...atatu.com>
To: Vlad Buslov <vladbu@...dia.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org,
kernel@...atatu.com, deb.chatterjee@...el.com,
anjali.singhai@...el.com, namrata.limaye@...el.com,
khalidm@...dia.com, tom@...anda.io, pratyush@...anda.io,
jiri@...nulli.us, xiyou.wangcong@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
simon.horman@...igine.com
Subject: Re: [PATCH net-next RFC 15/20] p4tc: add action template create,
update, delete, get, flush and dump
On Wed, Jan 25, 2023 at 4:15 PM 'Vlad Buslov' via kernel issues
<kernel@...atatu.com> wrote:
>
> On Tue 24 Jan 2023 at 12:05, Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > index fd012270d..e4a6d7da6 100644
> > + bool miss;
[..]
> > +static int __tcf_p4_dyna_init(struct net *net, struct nlattr *est,
> > + struct p4tc_act *act, struct tc_act_dyna *parm,
> > + struct tc_action **a, struct tcf_proto *tp,
> > + struct tc_action_ops *a_o,
> > + struct tcf_chain **goto_ch, u32 flags,
> > + struct netlink_ext_ack *extack)
> > +{
> > + bool bind = flags & TCA_ACT_FLAGS_BIND;
> > + bool exists = false;
> > + int ret = 0;
> > + struct p4tc_pipeline *pipeline;
> > + u32 index;
> > + int err;
> > +
> > + index = parm->index;
> > +
> > + err = tcf_idr_check_alloc(act->tn, &index, a, bind);
> > + if (err < 0)
> > + return err;
> > +
> > + exists = err;
> > + if (!exists) {
> > + struct tcf_p4act *p;
> > +
> > + ret = tcf_idr_create(act->tn, index, est, a, a_o, bind, false,
> > + flags);
> > + if (ret) {
> > + tcf_idr_cleanup(act->tn, index);
> > + return ret;
> > + }
> > +
> > + /* dyn_ref here should never be 0, because if we are here, it
> > + * means that a template action of this kind was created. Thus
> > + * dyn_ref should be at least 1. Also since this operation and
> > + * others that add or delete action templates run with
> > + * rtnl_lock held, we cannot do this op and a deletion op in
> > + * parallel.
> > + */
>
> I'm not getting why you need atomic refcount here if according to the
> comment it is used with rtnl lock protection anyway...
>
> > + WARN_ON(!refcount_inc_not_zero(&a_o->dyn_ref));
> > +
> > + pipeline = act->pipeline;
> > +
> > + p = to_p4act(*a);
> > + p->p_id = pipeline->common.p_id;
> > + p->act_id = act->a_id;
> > + INIT_LIST_HEAD(&p->cmd_operations);
[..]
> > +
> > + params = rcu_dereference_protected(m->params, 1);
> > +
> > + if (refcount_read(&ops->dyn_ref) > 1)
> > + refcount_dec(&ops->dyn_ref);
>
> ...especially since usage like this is definitely not concurrency-safe
> without some external protection.
>
I think you may be right - we'll take a closer look. Initially our
goal was to avoid
rtnl lock then we decided to use rtnl only for templates and this may have been
a leftover.
> > +
> > + spin_lock_bh(&m->tcf_lock);
> > + if (params)
> > + call_rcu(¶ms->rcu, tcf_p4_act_params_destroy_rcu);
> > + spin_unlock_bh(&m->tcf_lock);
>
> Why take tcf_lock for scheduling a rcu callback?
>
Seems like a leftover.. Thanks for catching this.
cheers,
jamal
Powered by blists - more mailing lists