Messages in this thread | | | Date | Fri, 1 Jun 2012 11:53:30 +0200 | From | Borislav Petkov <> | Subject | Re: Is this code right in zram? |
| |
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.
| |