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

[Qu Wenruo <wqu@suse.com>]

在 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));
>>>>
>>
>