Messages in this thread | | | Date | Wed, 13 Aug 2008 09:45:03 +0200 | From | Ingo Molnar <> | Subject | Re: [patch]fastboot: remove duplicate unpack_to_rootfs() |
| |
* Shaohua Li <shaohua.li@intel.com> wrote:
> we check if initrd is initramfs first and then do real unpack. The > check isn't required, we can directly do unpack. If initrd isn't > initramfs, we can remove garbage. In my laptop, this saves 0.1s boot > time. This penalizes non-initramfs case, but now initramfs is mostly > widely used.
clever concept!
a few observations about the cleanup function:
> +static void __init clean_rootfs(void) > +{ > + int fd = sys_open("/", O_RDONLY, 0);
can this ever fail?
> + void *buf = malloc(1024);
no error checking for buf==NULL.
> + struct linux_dirent64 *dirp = buf; > + int count; > + > + memset(buf, 0, PAGE_SIZE);
overflow: clearly allocating a 1024 bytes buffer and then clearing 4096 bytes isnt that good?
you could introduce a default-off CONFIG_DEBUG_ROOTFS_CLEANUP option that does two runs of unpack_to_rootfs() and inserts an artificial clean_rootfs() inbetween? Even if that debug patch doesnt get integrated its a good test for the cleanup function.
> + count = sys_getdents64(fd, dirp, PAGE_SIZE);
... and then doing an up to 4096 bytes getdents into the buffer. A large enough initramfs will overflow this.
> + while (count > 0) { > + while (count > 0) { > + struct stat st; > + > + sys_newlstat(dirp->d_name, &st);
can this ever fail? If yes we should at least WARN_ON_ONCE().
> + if (S_ISDIR(st.st_mode)) > + sys_rmdir(dirp->d_name); > + else > + sys_unlink(dirp->d_name); > + > + count -= dirp->d_reclen;
can this ever zero-underflow, with a sufficiently corrupted initramfs? We should check for 0 underflow to be sure.
> + dirp = (void *)dirp + dirp->d_reclen;
likewise, we should size-overflow check this pointer. Failure modes of overrunning the buffer are subtle and hard to notice/track down.
> + } > + dirp = buf; > + memset(buf, 0, 1024); > + count = sys_getdents64(fd, dirp, PAGE_SIZE);
overflow: we do a 4096 bytes getdents into a 1K buffer.
> + } > + > + sys_close(fd); > + free(buf); > +} > + > static int __init populate_rootfs(void) > { > char *err = unpack_to_rootfs(__initramfs_start, > @@ -531,13 +563,15 @@ static int __init populate_rootfs(void) > int fd; > printk(KERN_INFO "checking if image is initramfs..."); > err = unpack_to_rootfs((char *)initrd_start, > - initrd_end - initrd_start, 1); > + initrd_end - initrd_start, 0); > if (!err) { > printk(" it is\n"); > - unpack_to_rootfs((char *)initrd_start, > - initrd_end - initrd_start, 0); > free_initrd(); > return 0; > + } else { > + clean_rootfs(); > + unpack_to_rootfs(__initramfs_start, > + __initramfs_end - __initramfs_start, 0); > }
the dry_run variable is now unused in unpack_to_rootfs() and could be eliminated.
> printk("it isn't (%s); looks like an initrd\n", err); > fd = sys_open("/initrd.image", O_WRONLY|O_CREAT, 0700);
Ingo
| |