Re: [PATCH] initramfs: clean old path before creating a hardlink

[Li Zhijian]

On 11/27/2018 07:34 AM, Andrew Morton wrote:
> On Fri, 16 Nov 2018 15:12:48 +0800 Li Zhijian <lizhijian@xxxxxxxxxxxxxx> wrote:
>
> > Previously, sys_link() will fail due to the new path is already existed.
> > this case ofen appears when we use a concated initrd, below is an
> > sample:
> >
> > 1) prepare a basic rootfs, it contains a regular files rc.local
> > lizhijian@:~/yocto-tiny-i386-2016-04-22$ cat etc/rc.local
> >   #!/bin/sh
> >   echo "Running /etc/rc.local..."
> > yocto-tiny-i386-2016-04-22$ find . | sed 's,^\./,,' | cpio -o -H newc | gzip -n -9 >../rootfs.cgz
> >
> > 2) create a extra initrd which also includes a etc/rc.local
> > lizhijian@:~/lkp-x86_64/etc$ echo "append initrd" >rc.local
> > lizhijian@:~/lkp/lkp-x86_64/etc$ cat rc.local
> > append initrd
> > lizhijian@:~/lkp/lkp-x86_64/etc$ ln rc.local rc.local.hardlink
> > append initrd
> > lizhijian@:~/lkp/lkp-x86_64/etc$ stat rc.local rc.local.hardlink
> >    File: 'rc.local'
> >    Size: 14        	Blocks: 8          IO Block: 4096   regular file
> > Device: 801h/2049d	Inode: 11296086    Links: 2
> > Access: (0664/-rw-rw-r--)  Uid: ( 1002/lizhijian)   Gid: ( 1002/lizhijian)
> > Access: 2018-11-15 16:08:28.654464815 +0800
> > Modify: 2018-11-15 16:07:57.514903210 +0800
> > Change: 2018-11-15 16:08:24.180228872 +0800
> >   Birth: -
> >    File: 'rc.local.hardlink'
> >    Size: 14        	Blocks: 8          IO Block: 4096   regular file
> > Device: 801h/2049d	Inode: 11296086    Links: 2
> > Access: (0664/-rw-rw-r--)  Uid: ( 1002/lizhijian)   Gid: ( 1002/lizhijian)
> > Access: 2018-11-15 16:08:28.654464815 +0800
> > Modify: 2018-11-15 16:07:57.514903210 +0800
> > Change: 2018-11-15 16:08:24.180228872 +0800
> >   Birth: -
> >
> > lizhijian@:~/lkp/lkp-x86_64$ find . | sed 's,^\./,,' | cpio -o -H newc | gzip -n -9 >../rc-local.cgz
> > lizhijian@:~/lkp/lkp-x86_64$ gzip -dc ../rc-local.cgz | cpio -t
> > .
> > etc
> > etc/rc.local.hardlink <<< it will be extracted first at this initrd
> > etc/rc.local
> >
> > 3) concate 2 initrds and boot
> > lizhijian@:~/lkp$ cat rootfs.cgz rc-local.cgz >concate-initrd.cgz
> > lizhijian@:~/lkp$ qemu-system-x86_64 -nographic -enable-kvm -cpu host -smp 1 -m 1024 -kernel ~/lkp/linux/arch/x86/boot/bzImage -append "console=ttyS0 earlyprint=ttyS0 ignore_loglevel" -initrd ./concate-initr.cgz -serial stdio -nodefaults
> >
> > In this case, sys_link(2) will fail and return -EEXIST, so we can only
> > get the rc.local at rootfs.cgz instead of rc-local.cgz
> I can't claim to understand this, but I believe you ;)

thank you.

IMO, people who use concatenated initrd(cat rootfs.cgz rc-local.cgz >concatenated-initrd.cgz),
could expect that the later initrd(rc-local.cgz) can overwrite the previous initrd(rootfs.cgz).

However the previous kernel can not ensure this.

> How serious is this problem?  Do you think a 4.20 merge is justified?

It's critical for our scenario where it always concatenates multiple initrds.
and our scenario often use the latest kernel, so 4.20+ is good to us.



> Or a -stable backport?  If so, why?
>
> The forward declaration is unpleasing.  Why not simply move the function?

Got it, i will update in V2


Thanks

> --- a/init/initramfs.c~initramfs-clean-old-path-before-creating-a-hardlink-fix
> +++ a/init/initramfs.c
> @@ -291,7 +291,17 @@ static int __init do_reset(void)
>   	return 1;
>   }
>   
> -static void __init clean_path(char *path, umode_t fmode);
> +static void __init clean_path(char *path, umode_t fmode)
> +{
> +	struct kstat st;
> +
> +	if (!vfs_lstat(path, &st) && (st.mode ^ fmode) & S_IFMT) {
> +		if (S_ISDIR(st.mode))
> +			ksys_rmdir(path);
> +		else
> +			ksys_unlink(path);
> +	}
> +}
>   
>   static int __init maybe_link(void)
>   {
> @@ -305,18 +315,6 @@ static int __init maybe_link(void)
>   	return 0;
>   }
>   
> -static void __init clean_path(char *path, umode_t fmode)
> -{
> -	struct kstat st;
> -
> -	if (!vfs_lstat(path, &st) && (st.mode ^ fmode) & S_IFMT) {
> -		if (S_ISDIR(st.mode))
> -			ksys_rmdir(path);
> -		else
> -			ksys_unlink(path);
> -	}
> -}
> -
>   static __initdata int wfd;
>   
>   static int __init do_name(void)
> _
>
>
>
> .