[PATCH 5/5] erofs-utils: mount: stop checking `/sys/block/nbdX/pid`

[zhaoyifan (H)]

On 2025/12/26 14:51, Gao Xiang wrote:
> Hi Yifan,
>
> On 2025/12/23 18:04, Yifan Zhao wrote:
>> The `current erofsmount_nbd()` implementation verifies that the value in
>> `/sys/block/nbdX/pid`` matches the PID of the process executing
>> `erofsmount_nbd_loopfn()`, using this as an indicator that the NBD
>> device is ready. This check is incorrect, as the PID recorded in the
>> aforementioned sysfs file may belong to a kernel thread rather than a
>> userspace process (see [1]).
>
> Do you have a way to reproduce that?

This issue is consistently reproducible in my WSL2 environment (kernel 
version 6.6.87.2-microsoft-standard-WSL2),

but works correctly in other environments (e.g., openEuler 24.03 SP2). 
The finer-grained difference

between these 2 environments remains unclear.

>
>>
>> Moreover, since this verification only occurs after the child process
>> has successfully issued and returned from the NBD connect request,
>> removing it introduces no risk of NBD device hijacking by malicious
>> actors. This patch removes the erroneous check.
>
> It's not used to avoid device hijacking by malicious actors but
> detecting if the NBD device is reused by another daemon.
>
ok. I believe under the ioctl interface, there is indeed no reliable way 
to determine exactly whether the current daemon

is the one holding the NBD device open (unlike with netlink, where 
|/sys/block/nbdX/backend|can be used for such verification).

This is indeed a genuine limitation.

Thanks,

Yifan

> Thanks,
> Gao Xiang
>
>>
>> [1] 
>> https://elixir.bootlin.com/linux/latest/source/drivers/block/nbd.c#L1501
>>
>> Signed-off-by: Yifan Zhao <zhaoyifan28 at huawei.com>
>> ---
>>   lib/backends/nbd.c | 16 +++++-----------
>>   lib/liberofs_nbd.h |  2 +-
>>   mount/main.c       |  5 ++---
>>   3 files changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/lib/backends/nbd.c b/lib/backends/nbd.c
>> index 46e75cd..2d941a9 100644
>> --- a/lib/backends/nbd.c
>> +++ b/lib/backends/nbd.c
>> @@ -52,7 +52,8 @@ struct nbd_reply {
>>       };
>>   } __packed;
>>   -long erofs_nbd_in_service(int nbdnum)
>> +/* Return: 0 while nbd is in service, <0 otherwise */
>> +int erofs_nbd_in_service(int nbdnum)
>>   {
>>       int fd, err;
>>       char s[32];
>> @@ -72,17 +73,10 @@ long erofs_nbd_in_service(int nbdnum)
>>           return -ENOTCONN;
>>         (void)snprintf(s, sizeof(s), "/sys/block/nbd%d/pid", nbdnum);
>> -    fd = open(s, O_RDONLY);
>> -    if (fd < 0)
>> +    if (access(s, F_OK) < 0)
>>           return -errno;
>> -    err = read(fd, s, sizeof(s));
>> -    if (err < 0) {
>> -        err = -errno;
>> -        close(fd);
>> -        return err;
>> -    }
>> -    close(fd);
>> -    return strtol(s, NULL, 10);
>> +
>> +    return 0;
>>   }
>>     int erofs_nbd_devscan(void)
>> diff --git a/lib/liberofs_nbd.h b/lib/liberofs_nbd.h
>> index 78c8af5..b719d80 100644
>> --- a/lib/liberofs_nbd.h
>> +++ b/lib/liberofs_nbd.h
>> @@ -34,7 +34,7 @@ struct erofs_nbd_request {
>>   /* 30-day timeout for NBD recovery */
>>   #define EROFS_NBD_DEAD_CONN_TIMEOUT    (3600 * 24 * 30)
>>   -long erofs_nbd_in_service(int nbdnum);
>> +int erofs_nbd_in_service(int nbdnum);
>>   int erofs_nbd_devscan(void);
>>   int erofs_nbd_connect(int nbdfd, int blkbits, u64 blocks);
>>   char *erofs_nbd_get_identifier(int nbdnum);
>> diff --git a/mount/main.c b/mount/main.c
>> index d2d4815..f6cba33 100644
>> --- a/mount/main.c
>> +++ b/mount/main.c
>> @@ -1270,6 +1270,8 @@ static int erofsmount_nbd(struct 
>> erofs_nbd_source *source,
>>         while (1) {
>>           err = erofs_nbd_in_service(msg.nbdnum);
>> +        if (!err)
>> +            break;
>>           if (err == -ENOENT || err == -ENOTCONN) {
>>               err = waitpid(child, &child_status, WNOHANG);
>>               if (err < 0)
>> @@ -1280,9 +1282,6 @@ static int erofsmount_nbd(struct 
>> erofs_nbd_source *source,
>>               usleep(50000);
>>               continue;
>>           }
>> -        if (err >= 0)
>> -            err = (err != child ? -EBUSY : 0);
>> -        break;
>>       }
>>         if (!err) {
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20251226/4c3258dc/attachment.htm>