Re: Is this code right in zram?

[Borislav Petkov <bp@alien8.de>]

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.