RE: [patch]fastboot: remove duplicate unpack_to_rootfs()

[Li, Shaohua]

>-----Original Message-----
>From: Ingo Molnar [mailto:mingo@xxxxxxx]
>Sent: Wednesday, August 13, 2008 3:45 PM
>To: Li, Shaohua
>Cc: lkml; Andrew Morton; Arjan van de Ven
>Subject: Re: [patch]fastboot: remove duplicate unpack_to_rootfs()
>
>
>* Shaohua Li <shaohua.li@xxxxxxxxx> 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?
I thought there will be a panic if it fails, so I didn't explicitly add a check here. I can add one.

>> +     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?
Oops, I use 4096 first and found it's too big, so changed to 1024, but forgot change all. My bad.

>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.
Actually I did the test already, just forgot change all size.

>> +     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().
I'll add check.

>> +                     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.
I'm not quite sure here. Do you think the .d_reclen can be a incorrect value?

>>  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.
Ok, I can cleanup this.

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