[PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Zoltan Racz]

Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
which returns the sector size of a partition (or its parent). After more testing 
it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
for them.

Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
---
 common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/common/device-utils.c b/common/device-utils.c
index dd781bc5..a75194bf 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
 	char sysfs[PATH_MAX] = {};
 	char sizebuf[128];
 
-	snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
+	/*
+	 * First we look for hw_sector_size directly directly under 
+	 * /sys/class/block/[partition_name]/queue. In case of loopback and
+	 * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda), 
+	 * and the partition`s sysfs folder itself contains informations regarding 
+	 * the sector size
+	 */
+	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
+	sysfd = open(sysfs, O_RDONLY);
 
-	if (!realpath(link_path, real_path)) {
-		error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
-		return -1;
-	}
+	if (sysfd < 0) {
+		/*
+		 * If we couldn`t find it, it means our partition is created on a real 
+		 * device and we need to find its parent
+		 */
+		snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
 
-	dev_name = basename(real_path);
+		if (!realpath(link_path, real_path)) {
+			error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
+			return -1;
+		}
 
-	if (!dev_name) {
-		error("Failed to determine basename for path %s\n", real_path);
-		return -1;
-	}
+		dev_name = basename(real_path);
 
-	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
+		if (!dev_name) {
+			error("Failed to determine basename for path %s\n", real_path);
+			return -1;
+		}
 
-	sysfd = open(sysfs, O_RDONLY);
-	if (sysfd < 0) {
-		error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
-		return -1;
+		memset(sysfs, 0, PATH_MAX);
+		snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
+
+		sysfd = open(sysfs, O_RDONLY);
+
+		if (sysfd < 0) {
+			error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
+			return -1;
+		}
 	}
 
 	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
-- 
2.48.1

Re: [PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Qu Wenruo]

在 2025/8/1 20:33, Zoltan Racz 写道:
> Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
> which returns the sector size of a partition (or its parent). After more testing
> it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
> for them.

I can fold this change into the original patch if needed.

Although during my test, even unprivileged users can still do regular 
ioctl based size detection, as long as the user have read permission to 
that device.

And if the user can not even read the device, I'd say the environment is 
set up to intentionally prevent user accesses to that block device.

So I'm not convinced about all the fallback method, especially we're 
doing a lot of special handling (partition vs raw devices).

Mind to also provide the test setup you're using and the involved block 
device mode?

> 
> Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
> ---
>   common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
>   1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/common/device-utils.c b/common/device-utils.c
> index dd781bc5..a75194bf 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
>   	char sysfs[PATH_MAX] = {};
>   	char sizebuf[128];
>   
> -	snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
> +	/*
> +	 * First we look for hw_sector_size directly directly under
> +	 * /sys/class/block/[partition_name]/queue. In case of loopback and
> +	 * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda),
> +	 * and the partition`s sysfs folder itself contains informations regarding
> +	 * the sector size
> +	 */
> +	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
> +	sysfd = open(sysfs, O_RDONLY);
>   
> -	if (!realpath(link_path, real_path)) {
> -		error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> -		return -1;
> -	}
> +	if (sysfd < 0) {

Just a small nitpic, it's better to check the errno against ENOENT.

But my question still stands, does it really make sense to use sysfs as 
a fallback?

Thanks,
Qu

> +		/*
> +		 * If we couldn`t find it, it means our partition is created on a real
> +		 * device and we need to find its parent
> +		 */
> +		snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
>   
> -	dev_name = basename(real_path);
> +		if (!realpath(link_path, real_path)) {
> +			error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> +			return -1;
> +		}
>   
> -	if (!dev_name) {
> -		error("Failed to determine basename for path %s\n", real_path);
> -		return -1;
> -	}
> +		dev_name = basename(real_path);
>   
> -	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> +		if (!dev_name) {
> +			error("Failed to determine basename for path %s\n", real_path);
> +			return -1;
> +		}
>   
> -	sysfd = open(sysfs, O_RDONLY);
> -	if (sysfd < 0) {
> -		error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> -		return -1;
> +		memset(sysfs, 0, PATH_MAX);
> +		snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> +
> +		sysfd = open(sysfs, O_RDONLY);
> +
> +		if (sysfd < 0) {
> +			error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> +			return -1;
> +		}
>   	}
>   
>   	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));

Re: [PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Racz Zoli]

I reproduced the bug on three separate distros, Arch, Ubuntu 25.04 and
Fedora 42 and open() fails on all three of them
when device usage was checked as a normal user. For testing I used the
most basic commands I could to be sure it`s
not only a particular usecase it fails.

For real storage device:

sudo mkfs.btrfs /dev/sda1
sudo mount /dev/sda1 /mnt
btrfs device usage /mnt -> run as normal user, and open() fails.

For loopback device:

fallocate -l 5G test_bug.img
sudo losetup --find --show test_bug.img
sudo mkfs.btrfs /dev/loop0
sudo mount /dev/loop0 /mnt/
btrfs device usage /mnt/ -> also fails

Thank you,
Zoltan

On Sat, Aug 2, 2025 at 7:19 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2025/8/1 20:33, Zoltan Racz 写道:
> > Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
> > which returns the sector size of a partition (or its parent). After more testing
> > it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
> > for them.
>
> I can fold this change into the original patch if needed.
>
> Although during my test, even unprivileged users can still do regular
> ioctl based size detection, as long as the user have read permission to
> that device.
>
> And if the user can not even read the device, I'd say the environment is
> set up to intentionally prevent user accesses to that block device.
>
> So I'm not convinced about all the fallback method, especially we're
> doing a lot of special handling (partition vs raw devices).
>
> Mind to also provide the test setup you're using and the involved block
> device mode?
>
> >
> > Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
> > ---
> >   common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
> >   1 file changed, 33 insertions(+), 15 deletions(-)
> >
> > diff --git a/common/device-utils.c b/common/device-utils.c
> > index dd781bc5..a75194bf 100644
> > --- a/common/device-utils.c
> > +++ b/common/device-utils.c
> > @@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
> >       char sysfs[PATH_MAX] = {};
> >       char sizebuf[128];
> >
> > -     snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
> > +     /*
> > +      * First we look for hw_sector_size directly directly under
> > +      * /sys/class/block/[partition_name]/queue. In case of loopback and
> > +      * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda),
> > +      * and the partition`s sysfs folder itself contains informations regarding
> > +      * the sector size
> > +      */
> > +     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
> > +     sysfd = open(sysfs, O_RDONLY);
> >
> > -     if (!realpath(link_path, real_path)) {
> > -             error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> > -             return -1;
> > -     }
> > +     if (sysfd < 0) {
>
> Just a small nitpic, it's better to check the errno against ENOENT.
>
> But my question still stands, does it really make sense to use sysfs as
> a fallback?
>
> Thanks,
> Qu
>
> > +             /*
> > +              * If we couldn`t find it, it means our partition is created on a real
> > +              * device and we need to find its parent
> > +              */
> > +             snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
> >
> > -     dev_name = basename(real_path);
> > +             if (!realpath(link_path, real_path)) {
> > +                     error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> > +                     return -1;
> > +             }
> >
> > -     if (!dev_name) {
> > -             error("Failed to determine basename for path %s\n", real_path);
> > -             return -1;
> > -     }
> > +             dev_name = basename(real_path);
> >
> > -     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> > +             if (!dev_name) {
> > +                     error("Failed to determine basename for path %s\n", real_path);
> > +                     return -1;
> > +             }
> >
> > -     sysfd = open(sysfs, O_RDONLY);
> > -     if (sysfd < 0) {
> > -             error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> > -             return -1;
> > +             memset(sysfs, 0, PATH_MAX);
> > +             snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> > +
> > +             sysfd = open(sysfs, O_RDONLY);
> > +
> > +             if (sysfd < 0) {
> > +                     error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> > +                     return -1;
> > +             }
> >       }
> >
> >       ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
>

Re: [PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Qu Wenruo]

在 2025/8/2 18:59, Racz Zoli 写道:
> I reproduced the bug on three separate distros, Arch, Ubuntu 25.04 and
> Fedora 42 and open() fails on all three of them
> when device usage was checked as a normal user. For testing I used the
> most basic commands I could to be sure it`s
> not only a particular usecase it fails.
> 
> For real storage device:
> 
> sudo mkfs.btrfs /dev/sda1
> sudo mount /dev/sda1 /mnt
> btrfs device usage /mnt -> run as normal user, and open() fails.

Have you checked if you're in the "disk" group?

> 
> For loopback device:
> 
> fallocate -l 5G test_bug.img
> sudo losetup --find --show test_bug.img
> sudo mkfs.btrfs /dev/loop0
> sudo mount /dev/loop0 /mnt/
> btrfs device usage /mnt/ -> also fails
> 
> Thank you,
> Zoltan
> 
> On Sat, Aug 2, 2025 at 7:19 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> 在 2025/8/1 20:33, Zoltan Racz 写道:
>>> Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
>>> which returns the sector size of a partition (or its parent). After more testing
>>> it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
>>> for them.
>>
>> I can fold this change into the original patch if needed.
>>
>> Although during my test, even unprivileged users can still do regular
>> ioctl based size detection, as long as the user have read permission to
>> that device.
>>
>> And if the user can not even read the device, I'd say the environment is
>> set up to intentionally prevent user accesses to that block device.
>>
>> So I'm not convinced about all the fallback method, especially we're
>> doing a lot of special handling (partition vs raw devices).
>>
>> Mind to also provide the test setup you're using and the involved block
>> device mode?
>>
>>>
>>> Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
>>> ---
>>>    common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
>>>    1 file changed, 33 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/common/device-utils.c b/common/device-utils.c
>>> index dd781bc5..a75194bf 100644
>>> --- a/common/device-utils.c
>>> +++ b/common/device-utils.c
>>> @@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
>>>        char sysfs[PATH_MAX] = {};
>>>        char sizebuf[128];
>>>
>>> -     snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
>>> +     /*
>>> +      * First we look for hw_sector_size directly directly under
>>> +      * /sys/class/block/[partition_name]/queue. In case of loopback and
>>> +      * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda),
>>> +      * and the partition`s sysfs folder itself contains informations regarding
>>> +      * the sector size
>>> +      */
>>> +     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
>>> +     sysfd = open(sysfs, O_RDONLY);
>>>
>>> -     if (!realpath(link_path, real_path)) {
>>> -             error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
>>> -             return -1;
>>> -     }
>>> +     if (sysfd < 0) {
>>
>> Just a small nitpic, it's better to check the errno against ENOENT.
>>
>> But my question still stands, does it really make sense to use sysfs as
>> a fallback?
>>
>> Thanks,
>> Qu
>>
>>> +             /*
>>> +              * If we couldn`t find it, it means our partition is created on a real
>>> +              * device and we need to find its parent
>>> +              */
>>> +             snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
>>>
>>> -     dev_name = basename(real_path);
>>> +             if (!realpath(link_path, real_path)) {
>>> +                     error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
>>> +                     return -1;
>>> +             }
>>>
>>> -     if (!dev_name) {
>>> -             error("Failed to determine basename for path %s\n", real_path);
>>> -             return -1;
>>> -     }
>>> +             dev_name = basename(real_path);
>>>
>>> -     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
>>> +             if (!dev_name) {
>>> +                     error("Failed to determine basename for path %s\n", real_path);
>>> +                     return -1;
>>> +             }
>>>
>>> -     sysfd = open(sysfs, O_RDONLY);
>>> -     if (sysfd < 0) {
>>> -             error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
>>> -             return -1;
>>> +             memset(sysfs, 0, PATH_MAX);
>>> +             snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
>>> +
>>> +             sysfd = open(sysfs, O_RDONLY);
>>> +
>>> +             if (sysfd < 0) {
>>> +                     error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
>>> +                     return -1;
>>> +             }
>>>        }
>>>
>>>        ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
>>

Re: [PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Racz Zoli]

If I add my user to the disk group it works, but starting from the
issue reported on github, I think it could be useful to leave the
sysfs functionality there and make btrfs device usage work for normal
users, and also users which are not in the disk group.
One usage I can think of would be for web services doing statistics
running under the apache user or any other random user who are not
part of this group.

But if the policy should be for normal users to not have access to
this functionality then you are right, and it might not be necessary
to have the sysfs functionality.

Thank you,
Zoli

On Sat, Aug 2, 2025 at 12:32 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2025/8/2 18:59, Racz Zoli 写道:
> > I reproduced the bug on three separate distros, Arch, Ubuntu 25.04 and
> > Fedora 42 and open() fails on all three of them
> > when device usage was checked as a normal user. For testing I used the
> > most basic commands I could to be sure it`s
> > not only a particular usecase it fails.
> >
> > For real storage device:
> >
> > sudo mkfs.btrfs /dev/sda1
> > sudo mount /dev/sda1 /mnt
> > btrfs device usage /mnt -> run as normal user, and open() fails.
>
> Have you checked if you're in the "disk" group?
>
> >
> > For loopback device:
> >
> > fallocate -l 5G test_bug.img
> > sudo losetup --find --show test_bug.img
> > sudo mkfs.btrfs /dev/loop0
> > sudo mount /dev/loop0 /mnt/
> > btrfs device usage /mnt/ -> also fails
> >
> > Thank you,
> > Zoltan
> >
> > On Sat, Aug 2, 2025 at 7:19 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >>
> >>
> >>
> >> 在 2025/8/1 20:33, Zoltan Racz 写道:
> >>> Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
> >>> which returns the sector size of a partition (or its parent). After more testing
> >>> it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
> >>> for them.
> >>
> >> I can fold this change into the original patch if needed.
> >>
> >> Although during my test, even unprivileged users can still do regular
> >> ioctl based size detection, as long as the user have read permission to
> >> that device.
> >>
> >> And if the user can not even read the device, I'd say the environment is
> >> set up to intentionally prevent user accesses to that block device.
> >>
> >> So I'm not convinced about all the fallback method, especially we're
> >> doing a lot of special handling (partition vs raw devices).
> >>
> >> Mind to also provide the test setup you're using and the involved block
> >> device mode?
> >>
> >>>
> >>> Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
> >>> ---
> >>>    common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
> >>>    1 file changed, 33 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/common/device-utils.c b/common/device-utils.c
> >>> index dd781bc5..a75194bf 100644
> >>> --- a/common/device-utils.c
> >>> +++ b/common/device-utils.c
> >>> @@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
> >>>        char sysfs[PATH_MAX] = {};
> >>>        char sizebuf[128];
> >>>
> >>> -     snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
> >>> +     /*
> >>> +      * First we look for hw_sector_size directly directly under
> >>> +      * /sys/class/block/[partition_name]/queue. In case of loopback and
> >>> +      * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda),
> >>> +      * and the partition`s sysfs folder itself contains informations regarding
> >>> +      * the sector size
> >>> +      */
> >>> +     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
> >>> +     sysfd = open(sysfs, O_RDONLY);
> >>>
> >>> -     if (!realpath(link_path, real_path)) {
> >>> -             error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> >>> -             return -1;
> >>> -     }
> >>> +     if (sysfd < 0) {
> >>
> >> Just a small nitpic, it's better to check the errno against ENOENT.
> >>
> >> But my question still stands, does it really make sense to use sysfs as
> >> a fallback?
> >>
> >> Thanks,
> >> Qu
> >>
> >>> +             /*
> >>> +              * If we couldn`t find it, it means our partition is created on a real
> >>> +              * device and we need to find its parent
> >>> +              */
> >>> +             snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
> >>>
> >>> -     dev_name = basename(real_path);
> >>> +             if (!realpath(link_path, real_path)) {
> >>> +                     error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> >>> +                     return -1;
> >>> +             }
> >>>
> >>> -     if (!dev_name) {
> >>> -             error("Failed to determine basename for path %s\n", real_path);
> >>> -             return -1;
> >>> -     }
> >>> +             dev_name = basename(real_path);
> >>>
> >>> -     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> >>> +             if (!dev_name) {
> >>> +                     error("Failed to determine basename for path %s\n", real_path);
> >>> +                     return -1;
> >>> +             }
> >>>
> >>> -     sysfd = open(sysfs, O_RDONLY);
> >>> -     if (sysfd < 0) {
> >>> -             error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> >>> -             return -1;
> >>> +             memset(sysfs, 0, PATH_MAX);
> >>> +             snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> >>> +
> >>> +             sysfd = open(sysfs, O_RDONLY);
> >>> +
> >>> +             if (sysfd < 0) {
> >>> +                     error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> >>> +                     return -1;
> >>> +             }
> >>>        }
> >>>
> >>>        ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
> >>
>

Re: [PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Qu Wenruo]

在 2025/8/2 22:15, Racz Zoli 写道:
> If I add my user to the disk group it works, but starting from the
> issue reported on github, I think it could be useful to leave the
> sysfs functionality there and make btrfs device usage work for normal
> users, and also users which are not in the disk group.
> One usage I can think of would be for web services doing statistics
> running under the apache user or any other random user who are not
> part of this group.

For those use cases, they do not really need to access the raw disks, 
regular `df` would be enough.

Although vanilla `df` is not always accurate for btrfs due to the 
dynamic chunk allocation behavior.

> 
> But if the policy should be for normal users to not have access to
> this functionality then you are right, and it might not be necessary
> to have the sysfs functionality.

I think the distro's policy is to minimal privilege by default, thus 
they want to reject such read access for non-disk group users.

And to be honest, if there is really some services want to access `btrfs 
fi usage`, adding them to `disk` group sounds completely valid to me.



But I also understand there are exceptions, like `lsblk` which shows the 
device size no matter if the user is in `disk` group or not, and that is 
utilizing sysfs too.

So I'd prefer David to do the final call.


And no matter what the final call David made, I still believe the error 
handling enhancement series (the first 4 patches from 
https://lore.kernel.org/linux-btrfs/cover.1754116463.git.wqu@suse.com/) 
would be needed before your fix.

Thanks,
Qu

> 
> Thank you,
> Zoli
> 
> On Sat, Aug 2, 2025 at 12:32 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> 在 2025/8/2 18:59, Racz Zoli 写道:
>>> I reproduced the bug on three separate distros, Arch, Ubuntu 25.04 and
>>> Fedora 42 and open() fails on all three of them
>>> when device usage was checked as a normal user. For testing I used the
>>> most basic commands I could to be sure it`s
>>> not only a particular usecase it fails.
>>>
>>> For real storage device:
>>>
>>> sudo mkfs.btrfs /dev/sda1
>>> sudo mount /dev/sda1 /mnt
>>> btrfs device usage /mnt -> run as normal user, and open() fails.
>>
>> Have you checked if you're in the "disk" group?
>>
>>>
>>> For loopback device:
>>>
>>> fallocate -l 5G test_bug.img
>>> sudo losetup --find --show test_bug.img
>>> sudo mkfs.btrfs /dev/loop0
>>> sudo mount /dev/loop0 /mnt/
>>> btrfs device usage /mnt/ -> also fails
>>>
>>> Thank you,
>>> Zoltan
>>>
>>> On Sat, Aug 2, 2025 at 7:19 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>>>
>>>>
>>>>
>>>> 在 2025/8/1 20:33, Zoltan Racz 写道:
>>>>> Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
>>>>> which returns the sector size of a partition (or its parent). After more testing
>>>>> it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
>>>>> for them.
>>>>
>>>> I can fold this change into the original patch if needed.
>>>>
>>>> Although during my test, even unprivileged users can still do regular
>>>> ioctl based size detection, as long as the user have read permission to
>>>> that device.
>>>>
>>>> And if the user can not even read the device, I'd say the environment is
>>>> set up to intentionally prevent user accesses to that block device.
>>>>
>>>> So I'm not convinced about all the fallback method, especially we're
>>>> doing a lot of special handling (partition vs raw devices).
>>>>
>>>> Mind to also provide the test setup you're using and the involved block
>>>> device mode?
>>>>
>>>>>
>>>>> Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
>>>>> ---
>>>>>     common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
>>>>>     1 file changed, 33 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/common/device-utils.c b/common/device-utils.c
>>>>> index dd781bc5..a75194bf 100644
>>>>> --- a/common/device-utils.c
>>>>> +++ b/common/device-utils.c
>>>>> @@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
>>>>>         char sysfs[PATH_MAX] = {};
>>>>>         char sizebuf[128];
>>>>>
>>>>> -     snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
>>>>> +     /*
>>>>> +      * First we look for hw_sector_size directly directly under
>>>>> +      * /sys/class/block/[partition_name]/queue. In case of loopback and
>>>>> +      * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda),
>>>>> +      * and the partition`s sysfs folder itself contains informations regarding
>>>>> +      * the sector size
>>>>> +      */
>>>>> +     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
>>>>> +     sysfd = open(sysfs, O_RDONLY);
>>>>>
>>>>> -     if (!realpath(link_path, real_path)) {
>>>>> -             error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
>>>>> -             return -1;
>>>>> -     }
>>>>> +     if (sysfd < 0) {
>>>>
>>>> Just a small nitpic, it's better to check the errno against ENOENT.
>>>>
>>>> But my question still stands, does it really make sense to use sysfs as
>>>> a fallback?
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>> +             /*
>>>>> +              * If we couldn`t find it, it means our partition is created on a real
>>>>> +              * device and we need to find its parent
>>>>> +              */
>>>>> +             snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
>>>>>
>>>>> -     dev_name = basename(real_path);
>>>>> +             if (!realpath(link_path, real_path)) {
>>>>> +                     error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
>>>>> +                     return -1;
>>>>> +             }
>>>>>
>>>>> -     if (!dev_name) {
>>>>> -             error("Failed to determine basename for path %s\n", real_path);
>>>>> -             return -1;
>>>>> -     }
>>>>> +             dev_name = basename(real_path);
>>>>>
>>>>> -     snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
>>>>> +             if (!dev_name) {
>>>>> +                     error("Failed to determine basename for path %s\n", real_path);
>>>>> +                     return -1;
>>>>> +             }
>>>>>
>>>>> -     sysfd = open(sysfs, O_RDONLY);
>>>>> -     if (sysfd < 0) {
>>>>> -             error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
>>>>> -             return -1;
>>>>> +             memset(sysfs, 0, PATH_MAX);
>>>>> +             snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
>>>>> +
>>>>> +             sysfd = open(sysfs, O_RDONLY);
>>>>> +
>>>>> +             if (sysfd < 0) {
>>>>> +                     error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
>>>>> +                     return -1;
>>>>> +             }
>>>>>         }
>>>>>
>>>>>         ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
>>>>
>>
> 

Re: [PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Qu Wenruo]

在 2025/8/1 20:33, Zoltan Racz 写道:
> Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
> which returns the sector size of a partition (or its parent). After more testing
> it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
> for them.

After more digging into the sysfs implementation and how util-linux is 
handling the sysfs block device size, it turns out that 
/sys/block/<dev>/size is always in sector unit (512 bytes).

It's not related to any block size in the queue directory.


The core code implementing this is from block/genhd.c, function 
part_size_show():

	sysfs_emit(buf, "%llu\n", bdev_nr_sectors(dev_to_bdev(dev));

BTW, to my surprise, this very basic member is not explained in any 
sysfs related docs...

So we don't need all those complex workaround at all, just a simple 
shift with SECTOR_SHIFT will solve the problem.

I'll update your previous fix to use /sys/block/<dev>/size with left 
shift directly.

Thanks,
Qu

> 
> Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
> ---
>   common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
>   1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/common/device-utils.c b/common/device-utils.c
> index dd781bc5..a75194bf 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
>   	char sysfs[PATH_MAX] = {};
>   	char sizebuf[128];
>   
> -	snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
> +	/*
> +	 * First we look for hw_sector_size directly directly under
> +	 * /sys/class/block/[partition_name]/queue. In case of loopback and
> +	 * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda),
> +	 * and the partition`s sysfs folder itself contains informations regarding
> +	 * the sector size
> +	 */
> +	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
> +	sysfd = open(sysfs, O_RDONLY);
>   
> -	if (!realpath(link_path, real_path)) {
> -		error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> -		return -1;
> -	}
> +	if (sysfd < 0) {
> +		/*
> +		 * If we couldn`t find it, it means our partition is created on a real
> +		 * device and we need to find its parent
> +		 */
> +		snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
>   
> -	dev_name = basename(real_path);
> +		if (!realpath(link_path, real_path)) {
> +			error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> +			return -1;
> +		}
>   
> -	if (!dev_name) {
> -		error("Failed to determine basename for path %s\n", real_path);
> -		return -1;
> -	}
> +		dev_name = basename(real_path);
>   
> -	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> +		if (!dev_name) {
> +			error("Failed to determine basename for path %s\n", real_path);
> +			return -1;
> +		}
>   
> -	sysfd = open(sysfs, O_RDONLY);
> -	if (sysfd < 0) {
> -		error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> -		return -1;
> +		memset(sysfs, 0, PATH_MAX);
> +		snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> +
> +		sysfd = open(sysfs, O_RDONLY);
> +
> +		if (sysfd < 0) {
> +			error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> +			return -1;
> +		}
>   	}
>   
>   	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));