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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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