Re: [PATCH][TRIVIAL] btrfs-progs: Allow autodetect_object_types() to handle the link.

[Boris Burkov <boris@bur.io>]

On Wed, Jan 05, 2022 at 02:32:58PM +0100, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
> 
> 
> Hi All,
> 
> Boris Burkov reported a problem when "btrfs prop get" is invoked on a link of a block
> device. This happens when btrfs is invoked on a LVM device (which typically is
> a link with an user friendly name to the real block device). In this case btrfs
> reports 'ERROR: object is not a btrfs object'.
> 
> ------------------
> Steps to reproduce:
> 
> $ sudo losetup -f disk-1.img 
> $ sudo losetup -f disk-2.img 
> $ sudo losetup -O NAME,BACK-FILE
> NAME       BACK-FILE
> /dev/loop1 /home/ghigo/test-allocation-hint/disk-2.img
> /dev/loop0 /home/ghigo/test-allocation-hint/disk-1.img
>                                   
> $ cd /dev/
> $ mv loop1 loop1-tmp
> $ ln -sf loop1-tmp loop1
> $ ls -l /dev/loop[01]*
> brw-rw---- 1 root disk 7, 0 Jan  5 13:42 /dev/loop0
> lrwxrwxrwx 1 root root    9 Jan  5 13:41 /dev/loop1 -> loop1-tmp
> brw-rw---- 1 root disk 7, 1 Jan  5 13:42 /dev/loop1-tmp
> 
> $ sudo mkfs.btrfs /dev/loop[0-1]
> [....]
> $ sudo mount /dev/loop1 mnt/
> 
> $ # this is the error
> $ sudo btrfs prop get /dev/loop1
> ERROR: object is not a btrfs object: /dev/loop1
> 
> $ # this is what should happen
> $ sudo btrfs prop get /dev/loop0
> label=
> 
> ------------------
> 
> The cause is in the function autodetect_object_types() that detects the type of
> btrfs object passed. If the object is an "inode" type (e.g. file, link...) it
> returns the type of object. If the object is a block device (it doesn't matter
> if it is in a btrfs filesystem), it returns it as block device.
> However it doesn't handle well the case where the object passed is a link
> to a block device (which could be a valid btrfs object). For example
> LVM/DM creates link to block devices.
> 
> This patch adds a further call to stat() (instead of reusing the previous lstat()
> returned value) when btrfs-progs checks if the object is a block device.

Thank you very much for investigating and fixing this. I tested this
patch an it works as advertised.

> 
> Reported-by: Boris Burkov <boris@bur.io>
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  cmds/property.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/cmds/property.c b/cmds/property.c
> index 59ef997c..97dc5ae1 100644
> --- a/cmds/property.c
> +++ b/cmds/property.c
> @@ -391,6 +391,14 @@ static int autodetect_object_types(const char *object, int *types_out)
>  			types |= prop_object_root;
>  	}
>  
> +	/*
> +	 * Do a new stat to follow a possible link
> +	 */

I took a look at the original lstat and it doesn't seem super strongly
motivated. It is used to detect a subvolume object (ino==256) so I don't
see why we would hate to have property get/set work on a symlink to a
subvol.

I tested a patch that just changes lstat to stat instead of adding the
second stat, and it handled the subvol case nicely too.

e.g.
ln -s /mnt/real /mnt/lnk
./btrfs property get /mnt/real ro
ro=false
./btrfs property get /mnt/lnk ro
ro=false

> +	ret = stat(object, &st);
> +	if (ret < 0) {
> +		ret = -errno;
> +		goto out;
> +	}
>  	if (S_ISBLK(st.st_mode))
>  		types |= prop_object_dev;
>  
> -- 
> 2.34.1
>