Re: Libmount bug ?

[Alan Huang <mmpgouride@gmail.com>]

On Oct 14, 2024, at 23:57, Karel Zak <kzak@redhat.com> wrote:
> 
> 
> Hi Alan,
> 
> On Sat, Oct 12, 2024 at 04:57:39PM GMT, Alan Huang wrote:
>> The bcachefs has the helper called mount.bcachefs.
> 
> do you mean the following script?
> https://evilpiepirate.org/git/bcachefs-tools.git/tree/mount.bcachefs.sh

Not this one, but the Rust code:

https://evilpiepirate.org/git/bcachefs-tools.git/tree/src/commands/mount.rs

> 
> I believe that if you call the regular mount(8) from the script, then
> it's probably fine to not worry about the options. mount(8) will be
> able to ignore them.
> 
>> Currently, there are users using fstab with nofail/user fail to mount,
>> we would like to know whether other filesystems using similar helper
>> properly handle this.
> 
> The mount.nfs command uses libmount internally to generate the
> mount(2) syscall flags, so it is not affected by any additional
> options.
> 
> The mount.fuse command has a list of unwanted mount options:
> https://github.com/libfuse/libfuse/blob/master/util/mount.fuse.c#L318-L326

It seems that no other kernel filesystems are using similar helpers.

> 
> Please note that the "EXTERNAL HELPERS" section in the mount(8) man
> page describes which options are ignored.
> 
> Also, if your mount helper is setuid (like mount.nfs), you still need
> to parse fstab to obtain mount options from a safe source. This is
> because options from the command line should be ignored as they are
> considered unsafe.

Good to know.

> 
>> 
>> This is like commit 06e05eb0f78566b68c44328c37d7c28d8655e9df 
>> (“libmount: don't pass option "defaults" to helper")
>> 
>> Or would you like something like this? This might be incomplete though (e.g. owner, noowner etc.)
>> 
>> diff --git a/libmount/src/optmap.c b/libmount/src/optmap.c
>> index d7569a0f0..c13b9ba19 100644
>> --- a/libmount/src/optmap.c
>> +++ b/libmount/src/optmap.c
>> @@ -152,11 +152,11 @@ static const struct libmnt_optmap userspace_opts_map[] =
>>    { "auto",    MNT_MS_NOAUTO, MNT_NOHLPS | MNT_INVERT | MNT_NOMTAB },  /* Can be mounted using -a */
>>    { "noauto",  MNT_MS_NOAUTO, MNT_NOHLPS | MNT_NOMTAB },  /* Can only be mounted explicitly */
>> 
>> -   { "user[=]", MNT_MS_USER },                             /* Allow ordinary user to mount (mtab) */
>> -   { "nouser",  MNT_MS_USER, MNT_INVERT | MNT_NOMTAB },    /* Forbid ordinary user to mount */
>> +   { "user[=]", MNT_MS_USER, MNT_NOHLPS},                             /* Allow ordinary user to mount (mtab) */
> 
> This may cause issues with certain helpers (e.g. cifs) where "user="
> is a standard option. However, this is something that needs to be
> addressed in libmount, as it already handles this use-case for cifs.
> The use of MNT_NOHLPS may override this.

Yeah, I was worried that there might be helpers using these options.

> 
>> +   { "nouser",  MNT_MS_USER, MNT_INVERT | MNT_NOMTAB | MNT_NOHLPS},    /* Forbid ordinary user to mount */
>> 
>> -   { "users",   MNT_MS_USERS, MNT_NOMTAB },                /* Allow ordinary users to mount */
>> -   { "nousers", MNT_MS_USERS, MNT_INVERT | MNT_NOMTAB },   /* Forbid ordinary users to mount */
>> +   { "users",   MNT_MS_USERS, MNT_NOMTAB | MNT_NOHLPS},                /* Allow ordinary users to mount */
>> +   { "nousers", MNT_MS_USERS, MNT_INVERT | MNT_NOMTAB | MNT_NOHLPS},   /* Forbid ordinary users to mount */
>> 
>>    { "owner",   MNT_MS_OWNER, MNT_NOMTAB },                /* Let the owner of the device mount */
>>    { "noowner", MNT_MS_OWNER, MNT_INVERT | MNT_NOMTAB },   /* Device owner has no special privs */
>> @@ -180,7 +180,7 @@ static const struct libmnt_optmap userspace_opts_map[] =
>>    { "sizelimit=", MNT_MS_SIZELIMIT, MNT_NOHLPS | MNT_NOMTAB },   /* loop device size limit */
>>    { "encryption=", MNT_MS_ENCRYPTION, MNT_NOHLPS | MNT_NOMTAB },   /* loop device encryption */
>> 
>> -   { "nofail",  MNT_MS_NOFAIL, MNT_NOMTAB },               /* Do not fail if ENOENT on dev */
>> +   { "nofail",  MNT_MS_NOFAIL, MNT_NOMTAB | MNT_NOHLPS},               /* Do not fail if ENOENT on dev */
> 
> Could this option be usable for some helpers?
> 
> I believe the best solution is to follow the Fuse way and define a
> list of options to ignore in your fs-specific helper.
> 
> The ideal solution would be to implement a better libmount (perhaps
> libmount2) where the /sbin/mount.<type> helpers are replaced with
> dlopen() modules. This way, the library would handle all the details
> such as command line and fstab options.

Agreed. 

Thanks,
Alan

>    Karel
> 
> -- 
> Karel Zak  <kzak@redhat.com>
> http://karelzak.blogspot.com
>