netdev - Re: [PATCH net-next RFC 15/20] p4tc: add action template create, update, delete, get, flush and dump

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]

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(&params->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