| 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: <20180531172327.GU3697@mtr-leonro.mtl.com>
Date: Thu, 31 May 2018 20:23:27 +0300
From: Leon Romanovsky <leon@...nel.org>
To: "Ruhl, Michael J" <michael.j.ruhl@...el.com>
Cc: Doug Ledford <dledford@...hat.com>,
Jason Gunthorpe <jgg@...lanox.com>,
RDMA mailing list <linux-rdma@...r.kernel.org>,
Boris Pismenny <borisp@...lanox.com>,
Matan Barak <matanb@...lanox.com>,
Or Gerlitz <ogerlitz@...lanox.com>,
Raed Salem <raeds@...lanox.com>,
Yishai Hadas <yishaih@...lanox.com>,
Saeed Mahameed <saeedm@...lanox.com>,
linux-netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH rdma-next v3 10/14] IB/uverbs: Add support for flow
counters
On Thu, May 31, 2018 at 02:49:44PM +0000, Ruhl, Michael J wrote:
> >-----Original Message-----
> >From: Leon Romanovsky [mailto:leon@...nel.org]
> >Sent: Thursday, May 31, 2018 9:44 AM
> >To: Doug Ledford <dledford@...hat.com>; Jason Gunthorpe
> ><jgg@...lanox.com>
> >Cc: Leon Romanovsky <leonro@...lanox.com>; RDMA mailing list <linux-
> >rdma@...r.kernel.org>; Boris Pismenny <borisp@...lanox.com>; Matan
> >Barak <matanb@...lanox.com>; Ruhl, Michael J <michael.j.ruhl@...el.com>;
> >Or Gerlitz <ogerlitz@...lanox.com>; Raed Salem <raeds@...lanox.com>;
> >Yishai Hadas <yishaih@...lanox.com>; Saeed Mahameed
> ><saeedm@...lanox.com>; linux-netdev <netdev@...r.kernel.org>
> >Subject: [PATCH rdma-next v3 10/14] IB/uverbs: Add support for flow
> >counters
> >
> >From: Raed Salem <raeds@...lanox.com>
> >
> >The struct ib_uverbs_flow_spec_action_count associates
> >a counters object with the flow.
> >
> >Post this association the flow counters can be read via
> >the counters object.
> >
> >Reviewed-by: Yishai Hadas <yishaih@...lanox.com>
> >Signed-off-by: Raed Salem <raeds@...lanox.com>
> >Signed-off-by: Leon Romanovsky <leonro@...lanox.com>
> >---
> > drivers/infiniband/core/uverbs.h | 1 +
> > drivers/infiniband/core/uverbs_cmd.c | 81
> >+++++++++++++++++++++++++++++++-----
> > include/uapi/rdma/ib_user_verbs.h | 13 ++++++
> > 3 files changed, 84 insertions(+), 11 deletions(-)
> >
> >diff --git a/drivers/infiniband/core/uverbs.h
> >b/drivers/infiniband/core/uverbs.h
> >index 5b2461fa634d..c0d40fc3a53a 100644
> >--- a/drivers/infiniband/core/uverbs.h
> >+++ b/drivers/infiniband/core/uverbs.h
> >@@ -263,6 +263,7 @@ struct ib_uverbs_flow_spec {
> > struct ib_uverbs_flow_spec_action_tag flow_tag;
> > struct ib_uverbs_flow_spec_action_drop drop;
> > struct ib_uverbs_flow_spec_action_handle action;
> >+ struct ib_uverbs_flow_spec_action_count flow_count;
> > };
> > };
> >
> >diff --git a/drivers/infiniband/core/uverbs_cmd.c
> >b/drivers/infiniband/core/uverbs_cmd.c
> >index ddb9d79691be..3179a95c6f5e 100644
> >--- a/drivers/infiniband/core/uverbs_cmd.c
> >+++ b/drivers/infiniband/core/uverbs_cmd.c
> >@@ -2748,43 +2748,82 @@ ssize_t ib_uverbs_detach_mcast(struct
> >ib_uverbs_file *file,
> > struct ib_uflow_resources {
> > size_t max;
> > size_t num;
> >- struct ib_flow_action *collection[0];
> >+ size_t collection_num;
> >+ size_t counters_num;
> >+ struct ib_counters **counters;
> >+ struct ib_flow_action **collection;
> > };
> >
> > static struct ib_uflow_resources *flow_resources_alloc(size_t num_specs)
> > {
> > struct ib_uflow_resources *resources;
> >
> >- resources =
> >- kmalloc(sizeof(*resources) +
> >- num_specs * sizeof(*resources->collection),
> >GFP_KERNEL);
> >+ resources = kzalloc(sizeof(*resources), GFP_KERNEL);
> >
> > if (!resources)
> >- return NULL;
> >+ goto err_res;
>
> Why the new goto?
No real reason :)
>
> >+
> >+ resources->counters =
> >+ kcalloc(num_specs, sizeof(*resources->counters),
> >GFP_KERNEL);
> >+
> >+ if (!resources->counters)
> >+ goto err_cnt;
>
> kcalloc() zeros stuff. Could you just have a single common goto for the
> cleanup?
I have mixed feelings regarding such approach, technically you are
right, but I think that it will hurt readability.
I can send followup patch, will it work for you?
Thanks for review.
>
> Mike
>
> >+
> >+ resources->collection =
> >+ kcalloc(num_specs, sizeof(*resources->collection),
> >GFP_KERNEL);
> >+
> >+ if (!resources->collection)
> >+ goto err_collection;
> >
> >- resources->num = 0;
> > resources->max = num_specs;
> >
> > return resources;
> >+
> >+err_collection:
> >+ kfree(resources->counters);
> >+err_cnt:
> >+ kfree(resources);
> >+err_res:
> >+ return NULL;
> > }
> >
> > void ib_uverbs_flow_resources_free(struct ib_uflow_resources *uflow_res)
> > {
> > unsigned int i;
> >
> >- for (i = 0; i < uflow_res->num; i++)
> >+ for (i = 0; i < uflow_res->collection_num; i++)
> > atomic_dec(&uflow_res->collection[i]->usecnt);
> >
> >+ for (i = 0; i < uflow_res->counters_num; i++)
> >+ atomic_dec(&uflow_res->counters[i]->usecnt);
> >+
> >+ kfree(uflow_res->collection);
> >+ kfree(uflow_res->counters);
> > kfree(uflow_res);
> > }
> >
> > static void flow_resources_add(struct ib_uflow_resources *uflow_res,
> >- struct ib_flow_action *action)
> >+ enum ib_flow_spec_type type,
> >+ void *ibobj)
> > {
> > WARN_ON(uflow_res->num >= uflow_res->max);
> >
> >- atomic_inc(&action->usecnt);
> >- uflow_res->collection[uflow_res->num++] = action;
> >+ switch (type) {
> >+ case IB_FLOW_SPEC_ACTION_HANDLE:
> >+ atomic_inc(&((struct ib_flow_action *)ibobj)->usecnt);
> >+ uflow_res->collection[uflow_res->collection_num++] =
> >+ (struct ib_flow_action *)ibobj;
> >+ break;
> >+ case IB_FLOW_SPEC_ACTION_COUNT:
> >+ atomic_inc(&((struct ib_counters *)ibobj)->usecnt);
> >+ uflow_res->counters[uflow_res->counters_num++] =
> >+ (struct ib_counters *)ibobj;
> >+ break;
> >+ default:
> >+ WARN_ON(1);
> >+ }
> >+
> >+ uflow_res->num++;
> > }
> >
> > static int kern_spec_to_ib_spec_action(struct ib_ucontext *ucontext,
> >@@ -2821,9 +2860,29 @@ static int kern_spec_to_ib_spec_action(struct
> >ib_ucontext *ucontext,
> > return -EINVAL;
> > ib_spec->action.size =
> > sizeof(struct ib_flow_spec_action_handle);
> >- flow_resources_add(uflow_res, ib_spec->action.act);
> >+ flow_resources_add(uflow_res,
> >+ IB_FLOW_SPEC_ACTION_HANDLE,
> >+ ib_spec->action.act);
> > uobj_put_obj_read(ib_spec->action.act);
> > break;
> >+ case IB_FLOW_SPEC_ACTION_COUNT:
> >+ if (kern_spec->flow_count.size !=
> >+ sizeof(struct ib_uverbs_flow_spec_action_count))
> >+ return -EINVAL;
> >+ ib_spec->flow_count.counters =
> >+ uobj_get_obj_read(counters,
> >+ UVERBS_OBJECT_COUNTERS,
> >+ kern_spec->flow_count.handle,
> >+ ucontext);
> >+ if (!ib_spec->flow_count.counters)
> >+ return -EINVAL;
> >+ ib_spec->flow_count.size =
> >+ sizeof(struct ib_flow_spec_action_count);
> >+ flow_resources_add(uflow_res,
> >+ IB_FLOW_SPEC_ACTION_COUNT,
> >+ ib_spec->flow_count.counters);
> >+ uobj_put_obj_read(ib_spec->flow_count.counters);
> >+ break;
> > default:
> > return -EINVAL;
> > }
> >diff --git a/include/uapi/rdma/ib_user_verbs.h
> >b/include/uapi/rdma/ib_user_verbs.h
> >index 409507f83b91..4f9991de8e3a 100644
> >--- a/include/uapi/rdma/ib_user_verbs.h
> >+++ b/include/uapi/rdma/ib_user_verbs.h
> >@@ -998,6 +998,19 @@ struct ib_uverbs_flow_spec_action_handle {
> > __u32 reserved1;
> > };
> >
> >+struct ib_uverbs_flow_spec_action_count {
> >+ union {
> >+ struct ib_uverbs_flow_spec_hdr hdr;
> >+ struct {
> >+ __u32 type;
> >+ __u16 size;
> >+ __u16 reserved;
> >+ };
> >+ };
> >+ __u32 handle;
> >+ __u32 reserved1;
> >+};
> >+
> > struct ib_uverbs_flow_tunnel_filter {
> > __be32 tunnel_id;
> > };
> >--
> >2.14.3
>
Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)
Powered by blists - more mailing lists