| 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: <87k01ad01q.fsf@nvidia.com>
Date: Wed, 25 Jan 2023 18:48:57 +0200
From: Vlad Buslov <vladbu@...dia.com>
To: Jamal Hadi Salim <jhs@...atatu.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
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