[dpdk-dev,1/2] crypto/scheduler: set null pointer after freeing
Checks
Commit Message
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(-)
Comments
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
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
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
> 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
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
> -----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>
> -----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
@@ -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);
@@ -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());
@@ -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());
@@ -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;
}