lkml.org 
[lkml]   [2008]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2008-08-13 09:49    [W:0.048 / U:0.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site