Re: [PATCH 00/12] treewide: Remove unnecessary kmalloc casts

Re: [PATCH 00/12] treewide: Remove unnecessary kmalloc casts

[Milton Miller]

On Mon Jun 07 2010 at around 23:53:18 EST, Joe Perches wrote:
> Trivial cleanups
> 
> arch/cris: Remove unnecessary kmalloc casts
> arch/powerpc: Remove unnecessary kmalloc casts
> arch/x86/kernel/tlb_uv.c: Remove unnecessary kmalloc casts
> drivers/gpu/drm: Remove unnecessary kmalloc casts
> drivers/isdn/capi/capidrv.c: Remove unnecessary kmalloc casts
> drivers/media: Remove unnecesary kmalloc casts
> drivers/message/i2o/i20_config.c: Remove unnecessary kmalloc casts
> drivers/parisc/iosapic.c: Remove unnecessary kzalloc cast
> drivers/s390: Remove unnecessary kmalloc casts
> drivers/serial/icom.c: Remove unnecessary kmalloc casts
> drivers/usb/storage/isd200.c: Remove unnecessary kmalloc casts
> fs/ufs/util.c: Remove unnecessary kmalloc casts
> 

Joe,

Thanks for doing cleanups.

However, in this case you are removing casts that, while not necessary
for C, are indeed there for a reason.

Specifically, they are of the form
   type *p;
   <code>
   p = (type *)kmalloc(sizeof(type), ...);

For example, from the powerpc patch:
> goto out;
> }
> - tmp_part = (struct nvram_partition *)
> - kmalloc(sizeof(struct nvram_partition), GFP_KERNEL);
> + tmp_part = kmalloc(sizeof(struct nvram_partition), GFP_KERNEL);
> err = -ENOMEM;

The reason they casts are present is to guard against someone changing
the type of p at the top of the function and not changing the type at
the kmalloc.

This was discussed some years ago, possibly around the time kcalloc
got its underflow detection.

Should we create a kmalloc wrapper that enforces this type saftey?
While we have added static analysis tools that can check these things,
and we have slab redzoning options, they tend to be run in batch by
"someone else" or in response to debugging a problem.


There may have been discussion of doing the above vs
   p = kmalloc(sizeof(*p), ...);

I don't remember if it was programmer preference or issues when p is
an array type.


I am not subscribed so I don't have the full list that you sent
these to, and I know some maintainers have already taken some of
the series the last time you posted; could you please augment the
CC list.

thanks,
milton

Re: [PATCH 00/12] treewide: Remove unnecessary kmalloc casts

[Joe Perches]

On Wed, 2010-06-09 at 02:20 -0500, Milton Miller wrote:
> On Mon Jun 07 2010 at around 23:53:18 EST, Joe Perches wrote:
> > Trivial cleanups
> The reason they casts are present is to guard against someone changing
> the type of p at the top of the function and not changing the type at
> the kmalloc.

That'd be true for most all of the kernel mallocs.

> I know some maintainers have already taken some of
> the series the last time you posted; could you please augment the
> CC list.

I did not send to trivial any of kmalloc cast removal patches that
were acked.  The patches in this series were not acked, so I think
no CC list needs augmentation.

Original: http://lkml.org/lkml/2010/5/31/471
Newest:   http://lkml.org/lkml/2010/6/7/428

cheers, Joe

Re: [PATCH 00/12] treewide: Remove unnecessary kmalloc casts

[Pekka Enberg]

Hi Milton,

On Wed, Jun 9, 2010 at 10:20 AM, Milton Miller <miltonm@bga.com> wrote:
> However, in this case you are removing casts that, while not necessary
> for C, are indeed there for a reason.
>
> Specifically, they are of the form
> =A0 type *p;
> =A0 <code>
> =A0 p =3D (type *)kmalloc(sizeof(type), ...);
>
> For example, from the powerpc patch:
>> goto out;
>> }
>> - tmp_part =3D (struct nvram_partition *)
>> - kmalloc(sizeof(struct nvram_partition), GFP_KERNEL);
>> + tmp_part =3D kmalloc(sizeof(struct nvram_partition), GFP_KERNEL);
>> err =3D -ENOMEM;
>
> The reason they casts are present is to guard against someone changing
> the type of p at the top of the function and not changing the type at
> the kmalloc.

If you're worried about that...

[snip]

> There may have been discussion of doing the above vs
> =A0 p =3D kmalloc(sizeof(*p), ...);

...it's better to use this form. There's actually a mention of this in
"Chapter 14: Allocating memory" of Documentation/CodingStyle. The
guard is not really a guard as someone can still change the "sizeof"
part to something else and the cast from "void *" will silently ignore
it.

                        Pekka