| 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
| ||
|
Message-ID: <CAM0EoMkqZmyUR6cRBu5vP46ik66iLaZZhJU-bEj7BaGfRaYbAA@mail.gmail.com>
Date: Thu, 26 Jan 2023 07:37:34 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Vlad Buslov <vladbu@...dia.com>
Cc: 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 01/20] net/sched: act_api: change act_base
into an IDR
Good catch Vlad. Maybe we can pick 256 to be safe.
cheers,
jamal
On Wed, Jan 25, 2023 at 11:57 AM Vlad Buslov <vladbu@...dia.com> wrote:
>
>
> On Tue 24 Jan 2023 at 12:04, Jamal Hadi Salim <jhs@...atatu.com> wrote:
> > Convert act_base from a list to an IDR.
> >
> > With the introduction of P4TC action templates, we introduce the concept of
> > dynamically creating actions on the fly. Dynamic action IDs are not statically
> > defined (as was the case previously) and are therefore harder to manage within
> > existing linked list approach. We convert to IDR because it has built in ID
> > management which we would have to re-invent with linked lists.
> >
> > Co-developed-by: Victor Nogueira <victor@...atatu.com>
> > Signed-off-by: Victor Nogueira <victor@...atatu.com>
> > Co-developed-by: Pedro Tammela <pctammela@...atatu.com>
> > Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> > Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>
> > ---
> > include/uapi/linux/pkt_cls.h | 1 +
> > net/sched/act_api.c | 39 +++++++++++++++++++++---------------
> > 2 files changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> > index 648a82f32..4d716841c 100644
> > --- a/include/uapi/linux/pkt_cls.h
> > +++ b/include/uapi/linux/pkt_cls.h
> > @@ -139,6 +139,7 @@ enum tca_id {
> > TCA_ID_MPLS,
> > TCA_ID_CT,
> > TCA_ID_GATE,
> > + TCA_ID_DYN,
> > /* other actions go here */
> > __TCA_ID_MAX = 255
> > };
> > diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> > index cd09ef49d..811dddc3b 100644
> > --- a/net/sched/act_api.c
> > +++ b/net/sched/act_api.c
> > @@ -890,7 +890,7 @@ void tcf_idrinfo_destroy(const struct tc_action_ops *ops,
> > }
> > EXPORT_SYMBOL(tcf_idrinfo_destroy);
> >
> > -static LIST_HEAD(act_base);
> > +static DEFINE_IDR(act_base);
> > static DEFINE_RWLOCK(act_mod_lock);
> > /* since act ops id is stored in pernet subsystem list,
> > * then there is no way to walk through only all the action
> > @@ -949,7 +949,6 @@ static void tcf_pernet_del_id_list(unsigned int id)
> > int tcf_register_action(struct tc_action_ops *act,
> > struct pernet_operations *ops)
> > {
> > - struct tc_action_ops *a;
> > int ret;
> >
> > if (!act->act || !act->dump || !act->init)
> > @@ -970,13 +969,24 @@ int tcf_register_action(struct tc_action_ops *act,
> > }
> >
> > write_lock(&act_mod_lock);
> > - list_for_each_entry(a, &act_base, head) {
> > - if (act->id == a->id || (strcmp(act->kind, a->kind) == 0)) {
> > + if (act->id) {
> > + if (idr_find(&act_base, act->id)) {
> > ret = -EEXIST;
> > goto err_out;
> > }
> > + ret = idr_alloc_u32(&act_base, act, &act->id, act->id,
> > + GFP_ATOMIC);
> > + if (ret < 0)
> > + goto err_out;
> > + } else {
> > + /* Only dynamic actions will require ID generation */
> > + act->id = TCA_ID_DYN;
>
> Hi Jamal,
>
> Since TCA_ID_DYN is exposed to userspace and this code expects to use
> the whole range of [TCA_ID_DYN, TCA_ID_MAX] for dynamic actions any new
> action added after that will have two choices:
>
> - Insert future TCA_ID_*NEW_ACTION* before TCA_ID_DYN in the enum tca_id
> in order for this code to continue to work (probably breaking userspace
> code compiled for previous kernels).
>
> - Modify this code to allocate dynamic action id from empty range
> following new action enum value, which is not ideal.
>
> Maybe consider defining TCA_ID_DYN=128 in order to leave some space for
> new actions to be added before it without affecting the userspace?
>
> > +
> > + ret = idr_alloc_u32(&act_base, act, &act->id, TCA_ID_MAX,
> > + GFP_ATOMIC);
> > + if (ret < 0)
> > + goto err_out;
> > }
> > - list_add_tail(&act->head, &act_base);
> > write_unlock(&act_mod_lock);
> >
> > return 0;
> > @@ -994,17 +1004,12 @@ EXPORT_SYMBOL(tcf_register_action);
> > int tcf_unregister_action(struct tc_action_ops *act,
> > struct pernet_operations *ops)
> > {
> > - struct tc_action_ops *a;
> > - int err = -ENOENT;
> > + int err = 0;
> >
> > write_lock(&act_mod_lock);
> > - list_for_each_entry(a, &act_base, head) {
> > - if (a == act) {
> > - list_del(&act->head);
> > - err = 0;
> > - break;
> > - }
> > - }
> > + if (!idr_remove(&act_base, act->id))
> > + err = -EINVAL;
> > +
> > write_unlock(&act_mod_lock);
> > if (!err) {
> > unregister_pernet_subsys(ops);
> > @@ -1019,10 +1024,11 @@ EXPORT_SYMBOL(tcf_unregister_action);
> > static struct tc_action_ops *tc_lookup_action_n(char *kind)
> > {
> > struct tc_action_ops *a, *res = NULL;
> > + unsigned long tmp, id;
> >
> > if (kind) {
> > read_lock(&act_mod_lock);
> > - list_for_each_entry(a, &act_base, head) {
> > + idr_for_each_entry_ul(&act_base, a, tmp, id) {
> > if (strcmp(kind, a->kind) == 0) {
> > if (try_module_get(a->owner))
> > res = a;
> > @@ -1038,10 +1044,11 @@ static struct tc_action_ops *tc_lookup_action_n(char *kind)
> > static struct tc_action_ops *tc_lookup_action(struct nlattr *kind)
> > {
> > struct tc_action_ops *a, *res = NULL;
> > + unsigned long tmp, id;
> >
> > if (kind) {
> > read_lock(&act_mod_lock);
> > - list_for_each_entry(a, &act_base, head) {
> > + idr_for_each_entry_ul(&act_base, a, tmp, id) {
> > if (nla_strcmp(kind, a->kind) == 0) {
> > if (try_module_get(a->owner))
> > res = a;
>
Powered by blists - more mailing lists