| Message ID | f4345fcac83ba226efdadcd4610861e434f8a73e.1641389199.git.kreijack@inwind.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [TRIVIAL] btrfs-progs: Allow autodetect_object_types() to handle the link. | expand |
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 >
On 05/01/2022 18.50, Boris Burkov wrote: > On Wed, Jan 05, 2022 at 02:32:58PM +0100, Goffredo Baroncelli wrote: >> From: Goffredo Baroncelli <kreijack@inwind.it> [...] > > 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. It is not so simple: think about a case where the symlink points to a subvolume of *another* filesystem. Now, "btrfs prop get" returns the property (e.g. the label) of the *underlining* filesystem. If we change statl to stat, it still return the property of the underlining filesystem, thinking that it is a subvolume (of a foreign filesystem). If fact I added an exception of that rule, because if we pass a block device, we don't consider the underling filesystem, but the filesystem contained in the block device. But there is a precedence in get/set label. Anyway, symlink created some confusion, and I bet that in btrfs there are areas with incoherence around symlink between the pointed object and the underling filesystem. BR G.Baroncelli
On Wed, Jan 05, 2022 at 07:23:33PM +0100, Goffredo Baroncelli wrote: > On 05/01/2022 18.50, Boris Burkov wrote: > > On Wed, Jan 05, 2022 at 02:32:58PM +0100, Goffredo Baroncelli wrote: > > > From: Goffredo Baroncelli <kreijack@inwind.it> > [...] > > > > 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. > > It is not so simple: think about a case where the symlink points to a > subvolume of *another* filesystem. > > Now, "btrfs prop get" returns the property (e.g. the label) of the *underlining* > filesystem. If we change statl to stat, it still return the property of the > underlining filesystem, thinking that it is a subvolume (of a foreign filesystem). > > If fact I added an exception of that rule, because if we pass a block device, we > don't consider the underling filesystem, but the filesystem contained in the block > device. But there is a precedence in get/set label. > > Anyway, symlink created some confusion, and I bet that in btrfs there are areas > with incoherence around symlink between the pointed object and the underling > filesystem. Good point. I agree that btrfs (the tool) is not the most rigorous with symlinks. In this very function, we check if it is a btrfs object by opening the file without O_NOFOLLOW, but then we do this lstat. I'm not exactly sure what you are alluding to regarding the precedent set by label, but I tested links and labels and it seems to exhibit the link following behavior. mkfs.btrfs -f /dev/loop0 mkfs.btrfs -f -L LOOP /dev/loop1 mount /dev/loop0 /mnt/0 mount /dev/loop1 /mnt/1 ln -s /mnt/1 /mnt/0/lnk btrfs property get /mnt/0 label label= btrfs property get /mnt/1 label label=LOOP btrfs property get /mnt/0/lnk label label=LOOP btrfs property get /mnt/0/lnk ro ERROR: object is not compatible with property: ro So it looks like root detection follows links but subvol detection does not. Without testing, but judging by the code, I think inode follows symlinks too. So with all that said, I think the most consistent is to make subvol follow the symlink, unless you have a really confusing unexpected behavior in mind with links out to another btrfs that I am failing to see. Thanks, Boris > > BR > G.Baroncelli > > -- > gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> > Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
On 05/01/2022 20.05, Boris Burkov wrote: > On Wed, Jan 05, 2022 at 07:23:33PM +0100, Goffredo Baroncelli wrote: >> On 05/01/2022 18.50, Boris Burkov wrote: >>> On Wed, Jan 05, 2022 at 02:32:58PM +0100, Goffredo Baroncelli wrote: >>>> From: Goffredo Baroncelli <kreijack@inwind.it> >> [...] >>> >>> 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. >> >> It is not so simple: think about a case where the symlink points to a >> subvolume of *another* filesystem. >> >> Now, "btrfs prop get" returns the property (e.g. the label) of the *underlining* >> filesystem. If we change statl to stat, it still return the property of the >> underlining filesystem, thinking that it is a subvolume (of a foreign filesystem). >> >> If fact I added an exception of that rule, because if we pass a block device, we >> don't consider the underling filesystem, but the filesystem contained in the block >> device. But there is a precedence in get/set label. >> >> Anyway, symlink created some confusion, and I bet that in btrfs there are areas >> with incoherence around symlink between the pointed object and the underling >> filesystem. > > Good point. I agree that btrfs (the tool) is not the most rigorous with > symlinks. In this very function, we check if it is a btrfs object by > opening the file without O_NOFOLLOW, but then we do this lstat. > > I'm not exactly sure what you are alluding to regarding the precedent set > by label, but I tested links and labels and it seems to exhibit the link > following behavior. > > mkfs.btrfs -f /dev/loop0 > mkfs.btrfs -f -L LOOP /dev/loop1 > mount /dev/loop0 /mnt/0 > mount /dev/loop1 /mnt/1 > ln -s /mnt/1 /mnt/0/lnk > btrfs property get /mnt/0 label > label= > btrfs property get /mnt/1 label > label=LOOP > btrfs property get /mnt/0/lnk label > label=LOOP > btrfs property get /mnt/0/lnk ro > ERROR: object is not compatible with property: ro > > So it looks like root detection follows links but subvol detection does > not. Without testing, but judging by the code, I think inode follows > symlinks too. So with all that said, I think the most consistent is to > make subvol follow the symlink, unless you have a really confusing > unexpected behavior in mind with links out to another btrfs that I am > failing to see. Hi Boris, you are right. I didn't consider that open(path) follows the symlink too. So I will update the patch changing statl() to stat() and removing the 2nd stat invocation. BR G.Baroncelli > Thanks, > Boris > >> >> BR >> G.Baroncelli >> >> -- >> gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> >> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
Hi all, On 05/01/2022 20.14, Goffredo Baroncelli wrote: > On 05/01/2022 20.05, Boris Burkov wrote: >> On Wed, Jan 05, 2022 at 07:23:33PM +0100, Goffredo Baroncelli wrote: >>> On 05/01/2022 18.50, Boris Burkov wrote: >>>> On Wed, Jan 05, 2022 at 02:32:58PM +0100, Goffredo Baroncelli wrote: >>>>> From: Goffredo Baroncelli <kreijack@inwind.it> >>> [...] >>>> >>>> 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. >>> >>> It is not so simple: think about a case where the symlink points to a >>> subvolume of *another* filesystem. >>> >>> Now, "btrfs prop get" returns the property (e.g. the label) of the *underlining* >>> filesystem. If we change statl to stat, it still return the property of the >>> underlining filesystem, thinking that it is a subvolume (of a foreign filesystem). >>> >>> If fact I added an exception of that rule, because if we pass a block device, we >>> don't consider the underling filesystem, but the filesystem contained in the block >>> device. But there is a precedence in get/set label. >>> >>> Anyway, symlink created some confusion, and I bet that in btrfs there are areas >>> with incoherence around symlink between the pointed object and the underling >>> filesystem. >> >> Good point. I agree that btrfs (the tool) is not the most rigorous with >> symlinks. In this very function, we check if it is a btrfs object by >> opening the file without O_NOFOLLOW, but then we do this lstat. >> >> I'm not exactly sure what you are alluding to regarding the precedent set >> by label, but I tested links and labels and it seems to exhibit the link >> following behavior. >> >> mkfs.btrfs -f /dev/loop0 >> mkfs.btrfs -f -L LOOP /dev/loop1 >> mount /dev/loop0 /mnt/0 >> mount /dev/loop1 /mnt/1 >> ln -s /mnt/1 /mnt/0/lnk >> btrfs property get /mnt/0 label >> label= >> btrfs property get /mnt/1 label >> label=LOOP >> btrfs property get /mnt/0/lnk label >> label=LOOP >> btrfs property get /mnt/0/lnk ro >> ERROR: object is not compatible with property: ro >> >> So it looks like root detection follows links but subvol detection does >> not. Without testing, but judging by the code, I think inode follows >> symlinks too. So with all that said, I think the most consistent is to >> make subvol follow the symlink, unless you have a really confusing >> unexpected behavior in mind with links out to another btrfs that I am >> failing to see. > > Hi Boris, > > you are right. I didn't consider that open(path) follows the symlink too. > So I will update the patch changing statl() to stat() and removing the > 2nd stat invocation. Only now I noticed that the code behind set/get_label works only with "regular" file or "block" device. This may be a more cleaner solution to avoid this kind of ambiguity. Of course we can add exception where required. BR G.Baroncelli > BR > G.Baroncelli > >> Thanks, >> Boris >> >>> >>> BR >>> G.Baroncelli >>> >>> -- >>> gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> >>> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 > >
On 05/01/2022 21.14, Goffredo Baroncelli wrote: > Hi all, > On 05/01/2022 20.14, Goffredo Baroncelli wrote: >> On 05/01/2022 20.05, Boris Burkov wrote: >[...] >> Hi Boris, >> >> you are right. I didn't consider that open(path) follows the symlink too. >> So I will update the patch changing statl() to stat() and removing the >> 2nd stat invocation. > > Only now I noticed that the code behind set/get_label works only with > "regular" file or "block" device. > This may be a more cleaner solution to avoid this kind of ambiguity. > No, I was wrong again. The set/get_label code doesn't works so. You can pass a link, only it tries to interpreter the link as a link to mount point... which mess... > Of course we can add exception where required. > > BR > G.Baroncelli > >> BR >> G.Baroncelli >> >>> Thanks, >>> Boris >>> >>>> >>>> BR >>>> G.Baroncelli >>>> >>>> -- >>>> gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it> >>>> Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5 >> >> > >
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 + */ + ret = stat(object, &st); + if (ret < 0) { + ret = -errno; + goto out; + } if (S_ISBLK(st.st_mode)) types |= prop_object_dev;