[dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[Pablo de Lara]

When freeing memory, pointers should be set to NULL,
to avoid memory corruption/segmentation faults.

Fixes: 31439ee72b2c ("crypto/scheduler: add API implementations")
Fixes: 50e14527b9d1 ("crypto/scheduler: improve parameters parsing")
Fixes: 57523e682bb7 ("crypto/scheduler: register operation functions")
Fixes: a783aa634410 ("crypto/scheduler: add packet size based mode")
Fixes: 4c07e0552f0a ("crypto/scheduler: add multicore scheduling mode")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 drivers/crypto/scheduler/rte_cryptodev_scheduler.c  | 8 ++++++--
 drivers/crypto/scheduler/scheduler_multicore.c      | 6 ++++--
 drivers/crypto/scheduler/scheduler_pkt_size_distr.c | 4 +++-
 drivers/crypto/scheduler/scheduler_pmd_ops.c        | 9 +++++++--
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/scheduler/rte_cryptodev_scheduler.c b/drivers/crypto/scheduler/rte_cryptodev_scheduler.c
index 140c8b418..ed574cc18 100644
--- a/drivers/crypto/scheduler/rte_cryptodev_scheduler.c
+++ b/drivers/crypto/scheduler/rte_cryptodev_scheduler.c
@@ -91,8 +91,10 @@ update_scheduler_capability(struct scheduler_ctx *sched_ctx)
 	struct rte_cryptodev_capabilities tmp_caps[256] = { {0} };
 	uint32_t nb_caps = 0, i;
 
-	if (sched_ctx->capabilities)
+	if (sched_ctx->capabilities) {
 		rte_free(sched_ctx->capabilities);
+		sched_ctx->capabilities = NULL;
+	}
 
 	for (i = 0; i < sched_ctx->nb_slaves; i++) {
 		struct rte_cryptodev_info dev_info;
@@ -462,8 +464,10 @@ rte_cryptodev_scheduler_load_user_scheduler(uint8_t scheduler_id,
 	sched_ctx->ops.option_set = scheduler->ops->option_set;
 	sched_ctx->ops.option_get = scheduler->ops->option_get;
 
-	if (sched_ctx->private_ctx)
+	if (sched_ctx->private_ctx) {
 		rte_free(sched_ctx->private_ctx);
+		sched_ctx->private_ctx = NULL;
+	}
 
 	if (sched_ctx->ops.create_private_ctx) {
 		int ret = (*sched_ctx->ops.create_private_ctx)(dev);
diff --git a/drivers/crypto/scheduler/scheduler_multicore.c b/drivers/crypto/scheduler/scheduler_multicore.c
index b2ce44ceb..15a57c3db 100644
--- a/drivers/crypto/scheduler/scheduler_multicore.c
+++ b/drivers/crypto/scheduler/scheduler_multicore.c
@@ -328,11 +328,13 @@ static int
 scheduler_create_private_ctx(struct rte_cryptodev *dev)
 {
 	struct scheduler_ctx *sched_ctx = dev->data->dev_private;
-	struct mc_scheduler_ctx *mc_ctx;
+	struct mc_scheduler_ctx *mc_ctx = NULL;
 	uint16_t i;
 
-	if (sched_ctx->private_ctx)
+	if (sched_ctx->private_ctx) {
 		rte_free(sched_ctx->private_ctx);
+		sched_ctx->private_ctx = NULL;
+	}
 
 	mc_ctx = rte_zmalloc_socket(NULL, sizeof(struct mc_scheduler_ctx), 0,
 			rte_socket_id());
diff --git a/drivers/crypto/scheduler/scheduler_pkt_size_distr.c b/drivers/crypto/scheduler/scheduler_pkt_size_distr.c
index 96bf01614..d09e849ae 100644
--- a/drivers/crypto/scheduler/scheduler_pkt_size_distr.c
+++ b/drivers/crypto/scheduler/scheduler_pkt_size_distr.c
@@ -334,8 +334,10 @@ scheduler_create_private_ctx(struct rte_cryptodev *dev)
 	struct scheduler_ctx *sched_ctx = dev->data->dev_private;
 	struct psd_scheduler_ctx *psd_ctx;
 
-	if (sched_ctx->private_ctx)
+	if (sched_ctx->private_ctx) {
 		rte_free(sched_ctx->private_ctx);
+		sched_ctx->private_ctx = NULL;
+	}
 
 	psd_ctx = rte_zmalloc_socket(NULL, sizeof(struct psd_scheduler_ctx), 0,
 			rte_socket_id());
diff --git a/drivers/crypto/scheduler/scheduler_pmd_ops.c b/drivers/crypto/scheduler/scheduler_pmd_ops.c
index 680c2afbe..147dc51e9 100644
--- a/drivers/crypto/scheduler/scheduler_pmd_ops.c
+++ b/drivers/crypto/scheduler/scheduler_pmd_ops.c
@@ -46,6 +46,7 @@ scheduler_attach_init_slave(struct rte_cryptodev *dev)
 				sched_ctx->init_slave_names[i]);
 
 		rte_free(sched_ctx->init_slave_names[i]);
+		sched_ctx->init_slave_names[i] = NULL;
 
 		sched_ctx->nb_init_slaves -= 1;
 	}
@@ -261,11 +262,15 @@ scheduler_pmd_close(struct rte_cryptodev *dev)
 		}
 	}
 
-	if (sched_ctx->private_ctx)
+	if (sched_ctx->private_ctx) {
 		rte_free(sched_ctx->private_ctx);
+		sched_ctx->private_ctx = NULL;
+	}
 
-	if (sched_ctx->capabilities)
+	if (sched_ctx->capabilities) {
 		rte_free(sched_ctx->capabilities);
+		sched_ctx->capabilities = NULL;
+	}
 
 	return 0;
 }
-- 
2.14.3

[dpdk-dev] [PATCH 2/2] crypto/scheduler: fix memory leak

[Pablo de Lara]

If private context creation fails, the memory
allocated for it and for some of the rings created
was being leaked.
In case of failure, it must be freed.

Fixes: 4c07e0552f0a ("crypto/scheduler: add multicore scheduling mode")
Cc: stable@dpdk.org

Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
 drivers/crypto/scheduler/scheduler_multicore.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/scheduler/scheduler_multicore.c b/drivers/crypto/scheduler/scheduler_multicore.c
index 15a57c3db..644426e93 100644
--- a/drivers/crypto/scheduler/scheduler_multicore.c
+++ b/drivers/crypto/scheduler/scheduler_multicore.c
@@ -352,20 +352,29 @@ scheduler_create_private_ctx(struct rte_cryptodev *dev)
 					rte_socket_id(), RING_F_SC_DEQ | RING_F_SP_ENQ);
 		if (!mc_ctx->sched_enq_ring[i]) {
 			CS_LOG_ERR("Cannot create ring for worker %u", i);
-			return -1;
+			goto exit;
 		}
 		snprintf(r_name, sizeof(r_name), MC_SCHED_DEQ_RING_NAME_PREFIX "%u", i);
 		mc_ctx->sched_deq_ring[i] = rte_ring_create(r_name, PER_SLAVE_BUFF_SIZE,
 					rte_socket_id(), RING_F_SC_DEQ | RING_F_SP_ENQ);
 		if (!mc_ctx->sched_deq_ring[i]) {
 			CS_LOG_ERR("Cannot create ring for worker %u", i);
-			return -1;
+			goto exit;
 		}
 	}
 
 	sched_ctx->private_ctx = (void *)mc_ctx;
 
 	return 0;
+
+exit:
+	for (i = 0; i < sched_ctx->nb_wc; i++) {
+		rte_ring_free(mc_ctx->sched_enq_ring[i]);
+		rte_ring_free(mc_ctx->sched_deq_ring[i]);
+	}
+	rte_free(mc_ctx);
+
+	return -1;
 }
 
 struct rte_cryptodev_scheduler_ops scheduler_mc_ops = {
-- 
2.14.3

Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[Akhil Goyal]

Hi Pablo,

On 4/26/2018 8:39 PM, Pablo de Lara wrote:
> When freeing memory, pointers should be set to NULL,
> to avoid memory corruption/segmentation faults.

Shouldn't this be handled in the rte_free itself. A lot of other driver 
are also not setting null after rte_free.
This would require change at a lot of places if this is not handled in 
rte_free.

Thanks,
Akhil

Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[De Lara Guarch, Pablo]

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Friday, April 27, 2018 9:47 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after
> freeing
> 
> Hi Pablo,
> 
> On 4/26/2018 8:39 PM, Pablo de Lara wrote:
> > When freeing memory, pointers should be set to NULL, to avoid memory
> > corruption/segmentation faults.
> 
> Shouldn't this be handled in the rte_free itself. A lot of other driver are also not
> setting null after rte_free.
> This would require change at a lot of places if this is not handled in rte_free.
> 

The glibc function "free" works the same way. Users are responsible for
setting to NULL these pointers (because sometimes, it is not necessary to do such thing).

Anyway, in case we still wanted to change it, we would need to pass a pointer
to a pointer in rte_free, which would imply an API breakage.

Thanks,
Pablo

> Thanks,
> Akhil

Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[Akhil Goyal]

Hi Pablo,

On 4/27/2018 5:06 PM, De Lara Guarch, Pablo wrote:
> Hi Akhil,
>
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Friday, April 27, 2018 9:47 AM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Zhang, Roy Fan
>> <roy.fan.zhang@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after
>> freeing
>>
>> Hi Pablo,
>>
>> On 4/26/2018 8:39 PM, Pablo de Lara wrote:
>>> When freeing memory, pointers should be set to NULL, to avoid memory
>>> corruption/segmentation faults.
>>
>> Shouldn't this be handled in the rte_free itself. A lot of other driver are also not
>> setting null after rte_free.
>> This would require change at a lot of places if this is not handled in rte_free.
>>
>
> The glibc function "free" works the same way. Users are responsible for
> setting to NULL these pointers (because sometimes, it is not necessary to do such thing).
Yes it is correct but rte_free is custom free API in DPDK which can be 
modified or we can have a safer API rte_free_safe which can set the 
pointer to null.
>
> Anyway, in case we still wanted to change it, we would need to pass a pointer
> to a pointer in rte_free, which would imply an API breakage.
>
I think if the community agrees, we can add this change may be in next 
releases.

> Thanks,
> Pablo
>
>> Thanks,
>> Akhil

Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
> Sent: Friday, April 27, 2018 12:59 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after
> freeing
> 
> Hi Pablo,
> 
> On 4/27/2018 5:06 PM, De Lara Guarch, Pablo wrote:
> > Hi Akhil,
> >
> >> -----Original Message-----
> >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> >> Sent: Friday, April 27, 2018 9:47 AM
> >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Zhang, Roy Fan
> >> <roy.fan.zhang@intel.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer
> after
> >> freeing
> >>
> >> Hi Pablo,
> >>
> >> On 4/26/2018 8:39 PM, Pablo de Lara wrote:
> >>> When freeing memory, pointers should be set to NULL, to avoid memory
> >>> corruption/segmentation faults.
> >>
> >> Shouldn't this be handled in the rte_free itself. A lot of other driver are
> also not
> >> setting null after rte_free.
> >> This would require change at a lot of places if this is not handled in
> rte_free.
> >>
> >
> > The glibc function "free" works the same way. Users are responsible for
> > setting to NULL these pointers (because sometimes, it is not necessary to do
> such thing).
> Yes it is correct but rte_free is custom free API in DPDK which can be
> modified or we can have a safer API rte_free_safe which can set the
> pointer to null.
> >
> > Anyway, in case we still wanted to change it, we would need to pass a
> pointer
> > to a pointer in rte_free, which would imply an API breakage.


Actually there is an alternative solution, by creating a macro like so;

#define rte_free(x) do {
     rte_free_(x); /* call the real implementation, now with _ */
     x = NULL;
} while (0)

This is not an ABI break if symbol versioning is used for rte_free().

It is an API change however - not that the calling code has to change,
but rather that the effect of rte_free() changes transparently.

I'm not sure what the correct thing to do is in this case - just pointing
out that this is another possible solution.


> I think if the community agrees, we can add this change may be in next
> releases.

+1 to discuss this with the community, regardless of the implementation :)


> > Thanks,
> > Pablo
> >
> >> Thanks,
> >> Akhil

Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[Bruce Richardson]

On Fri, Apr 27, 2018 at 12:37:08PM +0000, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal
> > Sent: Friday, April 27, 2018 12:59 PM
> > To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Akhil Goyal
> > <akhil.goyal@nxp.com>; Zhang, Roy Fan <roy.fan.zhang@intel.com>
> > Cc: dev@dpdk.org; stable@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after
> > freeing
> > 
> > Hi Pablo,
> > 
> > On 4/27/2018 5:06 PM, De Lara Guarch, Pablo wrote:
> > > Hi Akhil,
> > >
> > >> -----Original Message-----
> > >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > >> Sent: Friday, April 27, 2018 9:47 AM
> > >> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Zhang, Roy Fan
> > >> <roy.fan.zhang@intel.com>
> > >> Cc: dev@dpdk.org; stable@dpdk.org
> > >> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer
> > after
> > >> freeing
> > >>
> > >> Hi Pablo,
> > >>
> > >> On 4/26/2018 8:39 PM, Pablo de Lara wrote:
> > >>> When freeing memory, pointers should be set to NULL, to avoid memory
> > >>> corruption/segmentation faults.
> > >>
> > >> Shouldn't this be handled in the rte_free itself. A lot of other driver are
> > also not
> > >> setting null after rte_free.
> > >> This would require change at a lot of places if this is not handled in
> > rte_free.
> > >>
> > >
> > > The glibc function "free" works the same way. Users are responsible for
> > > setting to NULL these pointers (because sometimes, it is not necessary to do
> > such thing).
> > Yes it is correct but rte_free is custom free API in DPDK which can be
> > modified or we can have a safer API rte_free_safe which can set the
> > pointer to null.
> > >
> > > Anyway, in case we still wanted to change it, we would need to pass a
> > pointer
> > > to a pointer in rte_free, which would imply an API breakage.
> 
> 
> Actually there is an alternative solution, by creating a macro like so;
> 
> #define rte_free(x) do {
>      rte_free_(x); /* call the real implementation, now with _ */
>      x = NULL;
> } while (0)
> 
> This is not an ABI break if symbol versioning is used for rte_free().
> 
> It is an API change however - not that the calling code has to change,
> but rather that the effect of rte_free() changes transparently.
> 
> I'm not sure what the correct thing to do is in this case - just pointing
> out that this is another possible solution.
> 
> 
> > I think if the community agrees, we can add this change may be in next
> > releases.
> 
> +1 to discuss this with the community, regardless of the implementation :)
> 
> 
I really don't think this change is necessary. I think having rte_free
behave as libc free is fine.

However, if we want to add a new API called rte_free_and_null(void **x),
I could be ok with that, though I'd be somewhat dubious of its necessity.
Static analysis tools should be able to pick up use-after-free errors,
though we may need to provide metadata to the tools in some form indicating
that rte_free is a free-ing function.

/Bruce

Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[Zhang, Roy Fan]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Thursday, April 26, 2018 4:10 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> stable@dpdk.org
> Subject: [PATCH 1/2] crypto/scheduler: set null pointer after freeing
> 
> When freeing memory, pointers should be set to NULL, to avoid memory
> corruption/segmentation faults.
> 
> Fixes: 31439ee72b2c ("crypto/scheduler: add API implementations")
> Fixes: 50e14527b9d1 ("crypto/scheduler: improve parameters parsing")
> Fixes: 57523e682bb7 ("crypto/scheduler: register operation functions")
> Fixes: a783aa634410 ("crypto/scheduler: add packet size based mode")
> Fixes: 4c07e0552f0a ("crypto/scheduler: add multicore scheduling mode")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

Re: [dpdk-dev] [PATCH 2/2] crypto/scheduler: fix memory leak

[Zhang, Roy Fan]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Thursday, April 26, 2018 4:10 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> stable@dpdk.org
> Subject: [PATCH 2/2] crypto/scheduler: fix memory leak
> 
> If private context creation fails, the memory allocated for it and for some of
> the rings created was being leaked.
> In case of failure, it must be freed.
> 
> Fixes: 4c07e0552f0a ("crypto/scheduler: add multicore scheduling mode")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>

Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Zhang, Roy Fan
> Sent: Tuesday, May 8, 2018 12:29 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH 1/2] crypto/scheduler: set null pointer after freeing
> 
> 
> 
> > -----Original Message-----
> > From: De Lara Guarch, Pablo
> > Sent: Thursday, April 26, 2018 4:10 PM
> > To: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; stable@dpdk.org
> > Subject: [PATCH 1/2] crypto/scheduler: set null pointer after freeing
> >
> > When freeing memory, pointers should be set to NULL, to avoid memory
> > corruption/segmentation faults.
> >
> > Fixes: 31439ee72b2c ("crypto/scheduler: add API implementations")
> > Fixes: 50e14527b9d1 ("crypto/scheduler: improve parameters parsing")
> > Fixes: 57523e682bb7 ("crypto/scheduler: register operation functions")
> > Fixes: a783aa634410 ("crypto/scheduler: add packet size based mode")
> > Fixes: 4c07e0552f0a ("crypto/scheduler: add multicore scheduling
> > mode")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> 
> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

Applied to dpdk-next-crypto.

Pablo

Re: [dpdk-dev] [PATCH 2/2] crypto/scheduler: fix memory leak

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Zhang, Roy Fan
> Sent: Tuesday, May 8, 2018 1:45 PM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: RE: [PATCH 2/2] crypto/scheduler: fix memory leak
> 
> 
> 
> > -----Original Message-----
> > From: De Lara Guarch, Pablo
> > Sent: Thursday, April 26, 2018 4:10 PM
> > To: Zhang, Roy Fan <roy.fan.zhang@intel.com>
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>; stable@dpdk.org
> > Subject: [PATCH 2/2] crypto/scheduler: fix memory leak
> >
> > If private context creation fails, the memory allocated for it and for
> > some of the rings created was being leaked.
> > In case of failure, it must be freed.
> >
> > Fixes: 4c07e0552f0a ("crypto/scheduler: add multicore scheduling
> > mode")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> >
> 
> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

Applied to dpdk-next-crypto.

Pablo