Is this code right in zram?

Is this code right in zram?

[John Moser]

before I go stomping all over other peoples' work and sending idiotic 
patches, I think I'll ask.  Since I have no clue what I'm doing.

in drivers/staging/zram.c out of 3.4 (I just grabbed the source hours 
ago), I see this on lines 810-822:

        /* Allocate the device array and initialize each one */
         pr_info("Creating %u devices ...\n", num_devices);
         zram_devices = kzalloc(num_devices * sizeof(struct zram), 
GFP_KERNEL);
         if (!zram_devices) {
                 ret = -ENOMEM;
                 goto unregister;
         }

         for (dev_id = 0; dev_id < num_devices; dev_id++) {
                 ret = create_device(&zram_devices[dev_id], dev_id);
                 if (ret)
                         goto free_devices;
         }

Curiosity got me to here:

http://lwn.net/Articles/147014/

So assuming this, what I see here is:

  - kmalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
  - memset() that to 0
  - immediately fill in this RAM without reading it

I'm wondering what the immediate need is to fill the area with zeros?  
Also curious as to whether the kzalloc() thing should better be 
kcalloc(num_devices, sizeof(struct zram), GFP_KERNEL) as a matter of 
convention.

Re: Is this code right in zram?

[Borislav Petkov]

On Thu, May 31, 2012 at 05:43:44PM -0400, John Moser wrote:
> before I go stomping all over other peoples' work and sending
> idiotic patches, I think I'll ask.  Since I have no clue what I'm
> doing.
> 
> in drivers/staging/zram.c out of 3.4

There's no such file in 3.4 - I get

tree v3.4:drivers/staging/zram/

Kconfig
Makefile
zram.txt
zram_drv.c
zram_drv.h
zram_sysfs.c

I'm guessing you mean zram_drv.c - there's code like that below?

> (I just grabbed the source
> hours ago), I see this on lines 810-822:
> 
>        /* Allocate the device array and initialize each one */
>         pr_info("Creating %u devices ...\n", num_devices);
>         zram_devices = kzalloc(num_devices * sizeof(struct zram),
> GFP_KERNEL);
>         if (!zram_devices) {
>                 ret = -ENOMEM;
>                 goto unregister;
>         }
> 
>         for (dev_id = 0; dev_id < num_devices; dev_id++) {
>                 ret = create_device(&zram_devices[dev_id], dev_id);
>                 if (ret)
>                         goto free_devices;
>         }
> 
> Curiosity got me to here:
> 
> http://lwn.net/Articles/147014/
> 
> So assuming this, what I see here is:
> 
>  - kmalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
>  - memset() that to 0
>  - immediately fill in this RAM without reading it
> 
> I'm wondering what the immediate need is to fill the area with
> zeros?

You don't want to use unitialized memory in those zram_devices thingies,
i.e. touch something in there which you haven't initialized before thus
the convention to zero out the whole struct.

Right, create_device() does that init later but what do you do if
create_device changes and forgets one variable, for example? There's
your bug (and a very subtle one, for that matter).

That's why you can't risk it and have to init the memory to a known good
value just in case.

> Also curious as to whether the kzalloc() thing should better be
> kcalloc(num_devices, sizeof(struct zram), GFP_KERNEL) as a matter of
> convention.

That makes sense, let's ask.

-- 
Regards/Gruss,
    Boris.

Re: Is this code right in zram?

[Jerome Marchand]

On 05/31/2012 11:43 PM, John Moser wrote:
> before I go stomping all over other peoples' work and sending idiotic 
> patches, I think I'll ask.  Since I have no clue what I'm doing.
> 
> in drivers/staging/zram.c out of 3.4 (I just grabbed the source hours 
> ago), I see this on lines 810-822:

I assume that's drivers/staging/zram_drv.c.

> 
>         /* Allocate the device array and initialize each one */
>          pr_info("Creating %u devices ...\n", num_devices);
>          zram_devices = kzalloc(num_devices * sizeof(struct zram), 
> GFP_KERNEL);
>          if (!zram_devices) {
>                  ret = -ENOMEM;
>                  goto unregister;
>          }
> 
>          for (dev_id = 0; dev_id < num_devices; dev_id++) {
>                  ret = create_device(&zram_devices[dev_id], dev_id);
>                  if (ret)
>                          goto free_devices;
>          }
> 
> Curiosity got me to here:
> 
> http://lwn.net/Articles/147014/
> 
> So assuming this, what I see here is:
> 
>   - kmalloc(num_devices * sizeof(struct zram), GFP_KERNEL);
>   - memset() that to 0
>   - immediately fill in this RAM without reading it
> 
> I'm wondering what the immediate need is to fill the area with zeros?

It's to avoid to have undefined values between the time the device is
created and initialized. In that case, it may be superfluous since the
locking mechanism should avoid to access an uninitialized device.
  
> Also curious as to whether the kzalloc() thing should better be 
> kcalloc(num_devices, sizeof(struct zram), GFP_KERNEL) as a matter of 
> convention.

It probably doesn't matter much. Apparently kcalloc() never got  a lot
of success and kzalloc(sizeof(foo) * num_foos) type of allocation are
more popular.

Jerome

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Is this code right in zram?

[Joe Perches]

On Mon, 2012-06-04 at 14:08 +0200, Jerome Marchand wrote:
> It probably doesn't matter much.

Likely true.

> Apparently kcalloc() never got  a lot
> of success and kzalloc(sizeof(foo) * num_foos) type of allocation are
> more popular.

Likely false, especially in new code.

If you did greps, perhaps you are including the
	k.alloc(sizeof(*foo), GFP)
forms in your uses multiply counts.

Re: Is this code right in zram?

[Jerome Marchand]

On 06/04/2012 02:33 PM, Joe Perches wrote:
> On Mon, 2012-06-04 at 14:08 +0200, Jerome Marchand wrote:
>> It probably doesn't matter much.
> 
> Likely true.
> 
>> Apparently kcalloc() never got  a lot
>> of success and kzalloc(sizeof(foo) * num_foos) type of allocation are
>> more popular.
> 
> Likely false, especially in new code.
> 
> If you did greps, perhaps you are including the
> 	k.alloc(sizeof(*foo), GFP)
> forms in your uses multiply counts.

No. Actually I greped it and look quickly at which proportion of those
kzalloc() looked like the above. I have to admit that my sample was
small and certainly not evenly distributed around the source tree.

Jerome

> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/