[PATCH] ovl: Allow mount options to be parsed on remount

[PATCH] ovl: Allow mount options to be parsed on remount

[André Almeida]

Allow mount options to be parsed on remount when using the new mount(8)
API. This allows to give a precise error code to userspace when the
remount is using wrong arguments instead of a generic -EINVAL error.

Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
Hi folks,

I was playing with xfstest with overlayfs and I got those fails:

$ sudo TEST_DIR=/tmp/dir1 TEST_DEV=/dev/vdb SCRATCH_DEV=/dev/vdc SCRATCH_MNT=/tmp/dir2 ./check -overlay
...
Failures: generic/294 generic/306 generic/452 generic/599 generic/623 overlay/035
Failed 6 of 859 tests

5 of those 6 fails were related to the same issue, where fsconfig
syscall returns EINVAL instead of EROFS:

-mount: cannot remount device read-write, is write-protected
+mount: /tmp/dir2/ovl-mnt: fsconfig() failed: overlay: No changes allowed in reconfigure

I tracked down the origin of this issue being commit ceecc2d87f00 ("ovl:
reserve ability to reconfigure mount options with new mount api"), where
ovl_parse_param() was modified to reject any reconfiguration when using
the new mount API, returning -EINVAL. This was done to avoid non-sense
parameters being accepted by the new API, as exemplified in the commit
message: 

	mount -t overlay overlay -o lowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work /mnt/merged
    
    and then issue a remount via:
    
            # force mount(8) to use mount(2)
            export LIBMOUNT_FORCE_MOUNT2=always
            mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged

    with completely nonsensical mount options whatsoever it will succeed
    nonetheless. 

However, after manually reverting such commit, I found out that
currently those nonsensical mount options are being reject by the
kernel:

$ mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
mount: /mnt/merged: fsconfig() failed: overlay: Unknown parameter 'WOOTWOOT'.

$ mount -t overlay overlay -o remount,lowerdir=/DOESNT-EXIST /mnt/merged
mount: /mnt/merged: special device overlay does not exist.
       dmesg(1) may have more information after failed mount system call

And now 5 tests are passing because the code can now returns EROFS:
Failures: generic/623
Failed 1 of 1 tests

So this patch basically allows for the parameters to be parsed and to
return an appropriated error message instead of a generic EINVAL one.

Please let me know if this looks like going in the right direction.

Thanks!
---
 fs/overlayfs/params.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index f42488c019572479d8fdcfc1efd62b21d2995875..f6b7acc0fee6174c48fcc8b87481fbcb60e6d421 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -600,15 +600,6 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
 		 */
 		if (fc->oldapi)
 			return 0;
-
-		/*
-		 * Give us the freedom to allow changing mount options
-		 * with the new mount api in the future. So instead of
-		 * silently ignoring everything we report a proper
-		 * error. This is only visible for users of the new
-		 * mount api.
-		 */
-		return invalfc(fc, "No changes allowed in reconfigure");
 	}
 
 	opt = fs_parse(fc, ovl_parameter_spec, param, &result);

---
base-commit: b87e2318cdaa14024b62ab428b3471d81eafaf1a
change-id: 20250521-ovl_ro-b38a57d2984d

Best regards,
-- 
André Almeida <andrealmeid@igalia.com>

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[Amir Goldstein]

On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
>
> Allow mount options to be parsed on remount when using the new mount(8)
> API. This allows to give a precise error code to userspace when the
> remount is using wrong arguments instead of a generic -EINVAL error.
>
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
> Hi folks,
>
> I was playing with xfstest with overlayfs and I got those fails:
>
> $ sudo TEST_DIR=/tmp/dir1 TEST_DEV=/dev/vdb SCRATCH_DEV=/dev/vdc SCRATCH_MNT=/tmp/dir2 ./check -overlay

I advise you to set the base FSTYP as README.overlay suggests
Some tests may require it to run properly or to run at all.
Probably related to failures you are seeing though...

> ...
> Failures: generic/294 generic/306 generic/452 generic/599 generic/623 overlay/035
> Failed 6 of 859 tests
>
> 5 of those 6 fails were related to the same issue, where fsconfig
> syscall returns EINVAL instead of EROFS:
>

I see the test generic/623 failure - this test needs to be fixed for overlay
or not run on overlayfs.

I do not see those other 5 failures although before running the test I did:
export LIBMOUNT_FORCE_MOUNT2=always

Not sure what I am doing differently.

> -mount: cannot remount device read-write, is write-protected
> +mount: /tmp/dir2/ovl-mnt: fsconfig() failed: overlay: No changes allowed in reconfigure
>
> I tracked down the origin of this issue being commit ceecc2d87f00 ("ovl:
> reserve ability to reconfigure mount options with new mount api"), where
> ovl_parse_param() was modified to reject any reconfiguration when using
> the new mount API, returning -EINVAL. This was done to avoid non-sense
> parameters being accepted by the new API, as exemplified in the commit
> message:
>
>         mount -t overlay overlay -o lowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work /mnt/merged
>
>     and then issue a remount via:
>
>             # force mount(8) to use mount(2)
>             export LIBMOUNT_FORCE_MOUNT2=always
>             mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
>
>     with completely nonsensical mount options whatsoever it will succeed
>     nonetheless.
>
> However, after manually reverting such commit, I found out that
> currently those nonsensical mount options are being reject by the
> kernel:
>
> $ mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
> mount: /mnt/merged: fsconfig() failed: overlay: Unknown parameter 'WOOTWOOT'.
>
> $ mount -t overlay overlay -o remount,lowerdir=/DOESNT-EXIST /mnt/merged
> mount: /mnt/merged: special device overlay does not exist.
>        dmesg(1) may have more information after failed mount system call
>
> And now 5 tests are passing because the code can now returns EROFS:
> Failures: generic/623
> Failed 1 of 1 tests
>
> So this patch basically allows for the parameters to be parsed and to
> return an appropriated error message instead of a generic EINVAL one.
>
> Please let me know if this looks like going in the right direction.

The purpose of the code that you reverted was not to disallow
nonsensical arguments.
The purpose was to not allow using mount arguments that
overlayfs cannot reconfigure.

Changing rw->ro should be allowed if no other arguments are
changed, but I cannot tell you for sure if and how this was implemented.
Christian?

>
> Thanks!
> ---
>  fs/overlayfs/params.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index f42488c019572479d8fdcfc1efd62b21d2995875..f6b7acc0fee6174c48fcc8b87481fbcb60e6d421 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -600,15 +600,6 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
>                  */
>                 if (fc->oldapi)
>                         return 0;
> -
> -               /*
> -                * Give us the freedom to allow changing mount options
> -                * with the new mount api in the future. So instead of
> -                * silently ignoring everything we report a proper
> -                * error. This is only visible for users of the new
> -                * mount api.
> -                */
> -               return invalfc(fc, "No changes allowed in reconfigure");
>         }
>
>         opt = fs_parse(fc, ovl_parameter_spec, param, &result);
>

NACK on this as it is.

Possibly, we need to identify the "only change RDONLY" case
and allow only that.

Thanks,
Amir.

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[Christian Brauner]

On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
> >
> > Allow mount options to be parsed on remount when using the new mount(8)
> > API. This allows to give a precise error code to userspace when the
> > remount is using wrong arguments instead of a generic -EINVAL error.
> >
> > Signed-off-by: André Almeida <andrealmeid@igalia.com>
> > ---
> > Hi folks,
> >
> > I was playing with xfstest with overlayfs and I got those fails:
> >
> > $ sudo TEST_DIR=/tmp/dir1 TEST_DEV=/dev/vdb SCRATCH_DEV=/dev/vdc SCRATCH_MNT=/tmp/dir2 ./check -overlay
> 
> I advise you to set the base FSTYP as README.overlay suggests
> Some tests may require it to run properly or to run at all.
> Probably related to failures you are seeing though...
> 
> > ...
> > Failures: generic/294 generic/306 generic/452 generic/599 generic/623 overlay/035
> > Failed 6 of 859 tests
> >
> > 5 of those 6 fails were related to the same issue, where fsconfig
> > syscall returns EINVAL instead of EROFS:
> >
> 
> I see the test generic/623 failure - this test needs to be fixed for overlay
> or not run on overlayfs.
> 
> I do not see those other 5 failures although before running the test I did:
> export LIBMOUNT_FORCE_MOUNT2=always
> 
> Not sure what I am doing differently.
> 
> > -mount: cannot remount device read-write, is write-protected
> > +mount: /tmp/dir2/ovl-mnt: fsconfig() failed: overlay: No changes allowed in reconfigure
> >
> > I tracked down the origin of this issue being commit ceecc2d87f00 ("ovl:
> > reserve ability to reconfigure mount options with new mount api"), where
> > ovl_parse_param() was modified to reject any reconfiguration when using
> > the new mount API, returning -EINVAL. This was done to avoid non-sense
> > parameters being accepted by the new API, as exemplified in the commit
> > message:
> >
> >         mount -t overlay overlay -o lowerdir=/mnt/a:/mnt/b,upperdir=/mnt/upper,workdir=/mnt/work /mnt/merged
> >
> >     and then issue a remount via:
> >
> >             # force mount(8) to use mount(2)
> >             export LIBMOUNT_FORCE_MOUNT2=always
> >             mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
> >
> >     with completely nonsensical mount options whatsoever it will succeed
> >     nonetheless.
> >
> > However, after manually reverting such commit, I found out that
> > currently those nonsensical mount options are being reject by the
> > kernel:
> >
> > $ mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: fsconfig() failed: overlay: Unknown parameter 'WOOTWOOT'.
> >
> > $ mount -t overlay overlay -o remount,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: special device overlay does not exist.
> >        dmesg(1) may have more information after failed mount system call
> >
> > And now 5 tests are passing because the code can now returns EROFS:
> > Failures: generic/623
> > Failed 1 of 1 tests
> >
> > So this patch basically allows for the parameters to be parsed and to
> > return an appropriated error message instead of a generic EINVAL one.
> >
> > Please let me know if this looks like going in the right direction.
> 
> The purpose of the code that you reverted was not to disallow
> nonsensical arguments.
> The purpose was to not allow using mount arguments that
> overlayfs cannot reconfigure.
> 
> Changing rw->ro should be allowed if no other arguments are
> changed, but I cannot tell you for sure if and how this was implemented.
> Christian?
> 
> >
> > Thanks!
> > ---
> >  fs/overlayfs/params.c | 9 ---------
> >  1 file changed, 9 deletions(-)
> >
> > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> > index f42488c019572479d8fdcfc1efd62b21d2995875..f6b7acc0fee6174c48fcc8b87481fbcb60e6d421 100644
> > --- a/fs/overlayfs/params.c
> > +++ b/fs/overlayfs/params.c
> > @@ -600,15 +600,6 @@ static int ovl_parse_param(struct fs_context *fc, struct fs_parameter *param)
> >                  */
> >                 if (fc->oldapi)
> >                         return 0;
> > -
> > -               /*
> > -                * Give us the freedom to allow changing mount options
> > -                * with the new mount api in the future. So instead of
> > -                * silently ignoring everything we report a proper
> > -                * error. This is only visible for users of the new
> > -                * mount api.
> > -                */
> > -               return invalfc(fc, "No changes allowed in reconfigure");
> >         }
> >
> >         opt = fs_parse(fc, ovl_parameter_spec, param, &result);
> >
> 
> NACK on this as it is.
> 
> Possibly, we need to identify the "only change RDONLY" case
> and allow only that.

Agreed. And this patch is seriously buggy. Just look at this:

> > However, after manually reverting such commit, I found out that
> > currently those nonsensical mount options are being reject by the
> > kernel:
> >
> > $ mount -t overlay overlay -o remount,WOOTWOOT,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: fsconfig() failed: overlay: Unknown parameter 'WOOTWOOT'.
> >
> > $ mount -t overlay overlay -o remount,lowerdir=/DOESNT-EXIST /mnt/merged
> > mount: /mnt/merged: special device overlay does not exist.
> >        dmesg(1) may have more information after failed mount system call

Well of course that fails because the lowedir you're pointing this at
doesn't exist. But consider someone passing a valid lowerdir path or
other valid options then suddenly we're changing the lowerdir parameters
for a running overlayfs instance which is obviously an immediate
security issue because we've just managed to create UAFs all over the
place.

Either the EINVAL for the new mount api is removed and we continue
unconditionally returning 0 for both the new and old mount api or
we allow a limited set of safe remount options.

But changing layers definitely isn't one of them and unconditionally
letting this code reach fs_parse() is an absolute nono.

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[André Almeida]

Hi Christian, Amir,

Thanks for the feedback :)

Em 21/05/2025 08:20, Christian Brauner escreveu:
> On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
>> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
>>>

[...]

>>
>> I see the test generic/623 failure - this test needs to be fixed for overlay
>> or not run on overlayfs.
>>
>> I do not see those other 5 failures although before running the test I did:
>> export LIBMOUNT_FORCE_MOUNT2=always
>>
>> Not sure what I am doing differently.
>>

I have created a smaller reproducer for this, have a look:

  mkdir -p ovl/lower ovl/upper ovl/merge ovl/work ovl/mnt
  sudo mount -t overlay overlay -o lowerdir=ovl/lower,upperdir=ovl/ 
upper,workdir=ovl/work ovl/mnt
  sudo mount ovl/mnt -o remount,ro

And this returns:

  mount: /tmp/ovl/mnt: fsconfig() failed: overlay: No changes allowed in 
  reconfigure.
        dmesg(1) may have more information after failed mount system call.

However, when I use mount like this:

  sudo mount -t overlay overlay -o remount,ro ovl/mnt

mount succeeds. Having a look at strace, I found out that the first 
mount command tries to set lowerdir to "ovl/lower" again, which will to 
return -EINVAL from ovl_parse_param():

    fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
    fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 
-1 EINVAL (Invalid argument)

Now, the second mount command sets just the "ro" flag, which will return 
after vfs_parse_sb_flag(), before getting to ovl_parse_param():

    fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
    fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0

After applying my patch and running the first mount command again, we 
can set that this flag is set only after setting all the strings:

    fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 0
    fsconfig(4, FSCONFIG_SET_STRING, "upperdir", "/tmp/ovl/upper", 0) = 0
    fsconfig(4, FSCONFIG_SET_STRING, "workdir", "/tmp/ovl/work", 0) = 0
    fsconfig(4, FSCONFIG_SET_STRING, "uuid", "on", 0) = 0
    fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0

I understood that the patch that I proposed is wrong, and now I wonder 
if the kernel needs to be fixed at all, or if the bug is how mount is 
using fsconfig() in the first mount command?

Thanks,
	André

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[Amir Goldstein]

On Thu, May 22, 2025 at 8:20 AM André Almeida <andrealmeid@igalia.com> wrote:
>
> Hi Christian, Amir,
>
> Thanks for the feedback :)
>
> Em 21/05/2025 08:20, Christian Brauner escreveu:
> > On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
> >> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
> >>>
>
> [...]
>
> >>
> >> I see the test generic/623 failure - this test needs to be fixed for overlay
> >> or not run on overlayfs.
> >>
> >> I do not see those other 5 failures although before running the test I did:
> >> export LIBMOUNT_FORCE_MOUNT2=always
> >>
> >> Not sure what I am doing differently.
> >>
>
> I have created a smaller reproducer for this, have a look:
>
>   mkdir -p ovl/lower ovl/upper ovl/merge ovl/work ovl/mnt
>   sudo mount -t overlay overlay -o lowerdir=ovl/lower,upperdir=ovl/
> upper,workdir=ovl/work ovl/mnt
>   sudo mount ovl/mnt -o remount,ro
>

Why would you use this command?
Why would you want to re-specify the lower/upperdir when remounting ro?
And more specifically, fstests does not use this command in the tests
that you mention that they fail, so what am I missing?

> And this returns:
>
>   mount: /tmp/ovl/mnt: fsconfig() failed: overlay: No changes allowed in
>   reconfigure.
>         dmesg(1) may have more information after failed mount system call.
>
> However, when I use mount like this:
>
>   sudo mount -t overlay overlay -o remount,ro ovl/mnt
>
> mount succeeds. Having a look at strace, I found out that the first
> mount command tries to set lowerdir to "ovl/lower" again, which will to
> return -EINVAL from ovl_parse_param():
>
>     fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>     fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) =
> -1 EINVAL (Invalid argument)
>
> Now, the second mount command sets just the "ro" flag, which will return
> after vfs_parse_sb_flag(), before getting to ovl_parse_param():
>
>     fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>     fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>
> After applying my patch and running the first mount command again, we
> can set that this flag is set only after setting all the strings:
>
>     fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 0
>     fsconfig(4, FSCONFIG_SET_STRING, "upperdir", "/tmp/ovl/upper", 0) = 0
>     fsconfig(4, FSCONFIG_SET_STRING, "workdir", "/tmp/ovl/work", 0) = 0
>     fsconfig(4, FSCONFIG_SET_STRING, "uuid", "on", 0) = 0
>     fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>
> I understood that the patch that I proposed is wrong, and now I wonder
> if the kernel needs to be fixed at all, or if the bug is how mount is
> using fsconfig() in the first mount command?

Maybe I am not reading your report correctly, but as this commands works:

mount -t overlay overlay -o remount,ro ovl/mnt

and the fstests that call _scratch_remount() work
I don't think there is anything to fix and I do not understand
what is the complaint.

Thanks,
Amir.

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[André Almeida]

Em 22/05/2025 06:52, Amir Goldstein escreveu:
> On Thu, May 22, 2025 at 8:20 AM André Almeida <andrealmeid@igalia.com> wrote:
>>
>> Hi Christian, Amir,
>>
>> Thanks for the feedback :)
>>
>> Em 21/05/2025 08:20, Christian Brauner escreveu:
>>> On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
>>>> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
>>>>>
>>
>> [...]
>>
>>>>
>>>> I see the test generic/623 failure - this test needs to be fixed for overlay
>>>> or not run on overlayfs.
>>>>
>>>> I do not see those other 5 failures although before running the test I did:
>>>> export LIBMOUNT_FORCE_MOUNT2=always
>>>>
>>>> Not sure what I am doing differently.
>>>>
>>
>> I have created a smaller reproducer for this, have a look:
>>
>>    mkdir -p ovl/lower ovl/upper ovl/merge ovl/work ovl/mnt
>>    sudo mount -t overlay overlay -o lowerdir=ovl/lower,upperdir=ovl/
>> upper,workdir=ovl/work ovl/mnt
>>    sudo mount ovl/mnt -o remount,ro
>>
> 
> Why would you use this command?
> Why would you want to re-specify the lower/upperdir when remounting ro?
> And more specifically, fstests does not use this command in the tests
> that you mention that they fail, so what am I missing?
> 

I've added "set -x" to tests/generic/294 to see exactly which mount 
parameters were being used and I got this from the output:

+ _try_scratch_mount -o remount,ro
+ local mount_ret
+ '[' overlay == overlay ']'
+ _overlay_scratch_mount -o remount,ro
+ echo '-o remount,ro'
+ grep -q remount
+ /usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro
mount: /tmp/dir2/ovl-mnt: fsconfig() failed: ...

So, from what I can see, fstests is using this command. Not sure if I 
did something wrong when setting up fstests.

>> And this returns:
>>
>>    mount: /tmp/ovl/mnt: fsconfig() failed: overlay: No changes allowed in
>>    reconfigure.
>>          dmesg(1) may have more information after failed mount system call.
>>
>> However, when I use mount like this:
>>
>>    sudo mount -t overlay overlay -o remount,ro ovl/mnt
>>
>> mount succeeds. Having a look at strace, I found out that the first
>> mount command tries to set lowerdir to "ovl/lower" again, which will to
>> return -EINVAL from ovl_parse_param():
>>
>>      fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>>      fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) =
>> -1 EINVAL (Invalid argument)
>>
>> Now, the second mount command sets just the "ro" flag, which will return
>> after vfs_parse_sb_flag(), before getting to ovl_parse_param():
>>
>>      fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>>      fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>>
>> After applying my patch and running the first mount command again, we
>> can set that this flag is set only after setting all the strings:
>>
>>      fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_STRING, "upperdir", "/tmp/ovl/upper", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_STRING, "workdir", "/tmp/ovl/work", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_STRING, "uuid", "on", 0) = 0
>>      fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>>
>> I understood that the patch that I proposed is wrong, and now I wonder
>> if the kernel needs to be fixed at all, or if the bug is how mount is
>> using fsconfig() in the first mount command?
> 
> Maybe I am not reading your report correctly, but as this commands works:
> 
> mount -t overlay overlay -o remount,ro ovl/mnt
> 
> and the fstests that call _scratch_remount() work
> I don't think there is anything to fix and I do not understand
> what is the complaint.
> 
> Thanks,
> Amir.

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[Amir Goldstein]

cc libfuse maintainer

On Thu, May 22, 2025 at 4:30 PM André Almeida <andrealmeid@igalia.com> wrote:
>
> Em 22/05/2025 06:52, Amir Goldstein escreveu:
> > On Thu, May 22, 2025 at 8:20 AM André Almeida <andrealmeid@igalia.com> wrote:
> >>
> >> Hi Christian, Amir,
> >>
> >> Thanks for the feedback :)
> >>
> >> Em 21/05/2025 08:20, Christian Brauner escreveu:
> >>> On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
> >>>> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
> >>>>>
> >>
> >> [...]
> >>
> >>>>
> >>>> I see the test generic/623 failure - this test needs to be fixed for overlay
> >>>> or not run on overlayfs.
> >>>>
> >>>> I do not see those other 5 failures although before running the test I did:
> >>>> export LIBMOUNT_FORCE_MOUNT2=always
> >>>>
> >>>> Not sure what I am doing differently.
> >>>>
> >>
> >> I have created a smaller reproducer for this, have a look:
> >>
> >>    mkdir -p ovl/lower ovl/upper ovl/merge ovl/work ovl/mnt
> >>    sudo mount -t overlay overlay -o lowerdir=ovl/lower,upperdir=ovl/
> >> upper,workdir=ovl/work ovl/mnt
> >>    sudo mount ovl/mnt -o remount,ro
> >>
> >
> > Why would you use this command?
> > Why would you want to re-specify the lower/upperdir when remounting ro?
> > And more specifically, fstests does not use this command in the tests
> > that you mention that they fail, so what am I missing?
> >
>
> I've added "set -x" to tests/generic/294 to see exactly which mount
> parameters were being used and I got this from the output:
>
> + _try_scratch_mount -o remount,ro
> + local mount_ret
> + '[' overlay == overlay ']'
> + _overlay_scratch_mount -o remount,ro
> + echo '-o remount,ro'
> + grep -q remount
> + /usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro
> mount: /tmp/dir2/ovl-mnt: fsconfig() failed: ...
>
> So, from what I can see, fstests is using this command. Not sure if I
> did something wrong when setting up fstests.
>

No you are right, I misread your reproducer.
The problem is that my test machine has older libmount 2.38.1
without the new mount API.


> >> And this returns:
> >>
> >>    mount: /tmp/ovl/mnt: fsconfig() failed: overlay: No changes allowed in
> >>    reconfigure.
> >>          dmesg(1) may have more information after failed mount system call.
> >>
> >> However, when I use mount like this:
> >>
> >>    sudo mount -t overlay overlay -o remount,ro ovl/mnt
> >>
> >> mount succeeds. Having a look at strace, I found out that the first
> >> mount command tries to set lowerdir to "ovl/lower" again, which will to
> >> return -EINVAL from ovl_parse_param():
> >>
> >>      fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
> >>      fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) =
> >> -1 EINVAL (Invalid argument)
> >>
> >> Now, the second mount command sets just the "ro" flag, which will return
> >> after vfs_parse_sb_flag(), before getting to ovl_parse_param():
> >>
> >>      fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
> >>      fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
> >>
> >> After applying my patch and running the first mount command again, we
> >> can set that this flag is set only after setting all the strings:
> >>
> >>      fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 0
> >>      fsconfig(4, FSCONFIG_SET_STRING, "upperdir", "/tmp/ovl/upper", 0) = 0
> >>      fsconfig(4, FSCONFIG_SET_STRING, "workdir", "/tmp/ovl/work", 0) = 0
> >>      fsconfig(4, FSCONFIG_SET_STRING, "uuid", "on", 0) = 0
> >>      fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
> >>
> >> I understood that the patch that I proposed is wrong, and now I wonder
> >> if the kernel needs to be fixed at all, or if the bug is how mount is
> >> using fsconfig() in the first mount command?
> >

If you ask me, when a user does:
/usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro

The library only needs to do the FSCONFIG_SET_FLAG command and has no
business re-sending the other config commands, but that's just me.

BTW, which version of libmount (mount --version) are you using?
I think there were a few problematic versions when the new mount api
was first introduced.

Thanks,
Amir.

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[André Almeida]

Em 22/05/2025 12:13, Amir Goldstein escreveu:
> cc libfuse maintainer
> 
> On Thu, May 22, 2025 at 4:30 PM André Almeida <andrealmeid@igalia.com> wrote:
>>
>> Em 22/05/2025 06:52, Amir Goldstein escreveu:
>>> On Thu, May 22, 2025 at 8:20 AM André Almeida <andrealmeid@igalia.com> wrote:
>>>>
>>>> Hi Christian, Amir,
>>>>
>>>> Thanks for the feedback :)
>>>>
>>>> Em 21/05/2025 08:20, Christian Brauner escreveu:
>>>>> On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
>>>>>> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
>>>>>>>
>>>>
>>>> [...]
>>>>
>>>>>>
>>>>>> I see the test generic/623 failure - this test needs to be fixed for overlay
>>>>>> or not run on overlayfs.
>>>>>>
>>>>>> I do not see those other 5 failures although before running the test I did:
>>>>>> export LIBMOUNT_FORCE_MOUNT2=always
>>>>>>
>>>>>> Not sure what I am doing differently.
>>>>>>
>>>>
>>>> I have created a smaller reproducer for this, have a look:
>>>>
>>>>     mkdir -p ovl/lower ovl/upper ovl/merge ovl/work ovl/mnt
>>>>     sudo mount -t overlay overlay -o lowerdir=ovl/lower,upperdir=ovl/
>>>> upper,workdir=ovl/work ovl/mnt
>>>>     sudo mount ovl/mnt -o remount,ro
>>>>
>>>
>>> Why would you use this command?
>>> Why would you want to re-specify the lower/upperdir when remounting ro?
>>> And more specifically, fstests does not use this command in the tests
>>> that you mention that they fail, so what am I missing?
>>>
>>
>> I've added "set -x" to tests/generic/294 to see exactly which mount
>> parameters were being used and I got this from the output:
>>
>> + _try_scratch_mount -o remount,ro
>> + local mount_ret
>> + '[' overlay == overlay ']'
>> + _overlay_scratch_mount -o remount,ro
>> + echo '-o remount,ro'
>> + grep -q remount
>> + /usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro
>> mount: /tmp/dir2/ovl-mnt: fsconfig() failed: ...
>>
>> So, from what I can see, fstests is using this command. Not sure if I
>> did something wrong when setting up fstests.
>>
> 
> No you are right, I misread your reproducer.
> The problem is that my test machine has older libmount 2.38.1
> without the new mount API.
> 
> 
>>>> And this returns:
>>>>
>>>>     mount: /tmp/ovl/mnt: fsconfig() failed: overlay: No changes allowed in
>>>>     reconfigure.
>>>>           dmesg(1) may have more information after failed mount system call.
>>>>
>>>> However, when I use mount like this:
>>>>
>>>>     sudo mount -t overlay overlay -o remount,ro ovl/mnt
>>>>
>>>> mount succeeds. Having a look at strace, I found out that the first
>>>> mount command tries to set lowerdir to "ovl/lower" again, which will to
>>>> return -EINVAL from ovl_parse_param():
>>>>
>>>>       fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>>>>       fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) =
>>>> -1 EINVAL (Invalid argument)
>>>>
>>>> Now, the second mount command sets just the "ro" flag, which will return
>>>> after vfs_parse_sb_flag(), before getting to ovl_parse_param():
>>>>
>>>>       fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
>>>>       fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>>>>
>>>> After applying my patch and running the first mount command again, we
>>>> can set that this flag is set only after setting all the strings:
>>>>
>>>>       fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 0
>>>>       fsconfig(4, FSCONFIG_SET_STRING, "upperdir", "/tmp/ovl/upper", 0) = 0
>>>>       fsconfig(4, FSCONFIG_SET_STRING, "workdir", "/tmp/ovl/work", 0) = 0
>>>>       fsconfig(4, FSCONFIG_SET_STRING, "uuid", "on", 0) = 0
>>>>       fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
>>>>
>>>> I understood that the patch that I proposed is wrong, and now I wonder
>>>> if the kernel needs to be fixed at all, or if the bug is how mount is
>>>> using fsconfig() in the first mount command?
>>>
> 
> If you ask me, when a user does:
> /usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro
> 
> The library only needs to do the FSCONFIG_SET_FLAG command and has no
> business re-sending the other config commands, but that's just me.
> 

Yes, this makes sense to me as well.

> BTW, which version of libmount (mount --version) are you using?
> I think there were a few problematic versions when the new mount api
> was first introduced.
> 

mount from util-linux 2.41 (libmount 2.41.0: btrfs, verity, namespaces, 
idmapping, fd-based-mount, statmount, assert, debug)

> Thanks,
> Amir.

Re: [PATCH] ovl: Allow mount options to be parsed on remount

[Amir Goldstein]

On Thu, May 22, 2025 at 5:22 PM André Almeida <andrealmeid@igalia.com> wrote:
>
> Em 22/05/2025 12:13, Amir Goldstein escreveu:
> > cc libfuse maintainer
> >
> > On Thu, May 22, 2025 at 4:30 PM André Almeida <andrealmeid@igalia.com> wrote:
> >>
> >> Em 22/05/2025 06:52, Amir Goldstein escreveu:
> >>> On Thu, May 22, 2025 at 8:20 AM André Almeida <andrealmeid@igalia.com> wrote:
> >>>>
> >>>> Hi Christian, Amir,
> >>>>
> >>>> Thanks for the feedback :)
> >>>>
> >>>> Em 21/05/2025 08:20, Christian Brauner escreveu:
> >>>>> On Wed, May 21, 2025 at 12:35:57PM +0200, Amir Goldstein wrote:
> >>>>>> On Wed, May 21, 2025 at 8:45 AM André Almeida <andrealmeid@igalia.com> wrote:
> >>>>>>>
> >>>>
> >>>> [...]
> >>>>
> >>>>>>
> >>>>>> I see the test generic/623 failure - this test needs to be fixed for overlay
> >>>>>> or not run on overlayfs.
> >>>>>>
> >>>>>> I do not see those other 5 failures although before running the test I did:
> >>>>>> export LIBMOUNT_FORCE_MOUNT2=always
> >>>>>>
> >>>>>> Not sure what I am doing differently.
> >>>>>>
> >>>>
> >>>> I have created a smaller reproducer for this, have a look:
> >>>>
> >>>>     mkdir -p ovl/lower ovl/upper ovl/merge ovl/work ovl/mnt
> >>>>     sudo mount -t overlay overlay -o lowerdir=ovl/lower,upperdir=ovl/
> >>>> upper,workdir=ovl/work ovl/mnt
> >>>>     sudo mount ovl/mnt -o remount,ro
> >>>>
> >>>
> >>> Why would you use this command?
> >>> Why would you want to re-specify the lower/upperdir when remounting ro?
> >>> And more specifically, fstests does not use this command in the tests
> >>> that you mention that they fail, so what am I missing?
> >>>
> >>
> >> I've added "set -x" to tests/generic/294 to see exactly which mount
> >> parameters were being used and I got this from the output:
> >>
> >> + _try_scratch_mount -o remount,ro
> >> + local mount_ret
> >> + '[' overlay == overlay ']'
> >> + _overlay_scratch_mount -o remount,ro
> >> + echo '-o remount,ro'
> >> + grep -q remount
> >> + /usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro
> >> mount: /tmp/dir2/ovl-mnt: fsconfig() failed: ...
> >>
> >> So, from what I can see, fstests is using this command. Not sure if I
> >> did something wrong when setting up fstests.
> >>
> >
> > No you are right, I misread your reproducer.
> > The problem is that my test machine has older libmount 2.38.1
> > without the new mount API.
> >
> >
> >>>> And this returns:
> >>>>
> >>>>     mount: /tmp/ovl/mnt: fsconfig() failed: overlay: No changes allowed in
> >>>>     reconfigure.
> >>>>           dmesg(1) may have more information after failed mount system call.
> >>>>
> >>>> However, when I use mount like this:
> >>>>
> >>>>     sudo mount -t overlay overlay -o remount,ro ovl/mnt
> >>>>
> >>>> mount succeeds. Having a look at strace, I found out that the first
> >>>> mount command tries to set lowerdir to "ovl/lower" again, which will to
> >>>> return -EINVAL from ovl_parse_param():
> >>>>
> >>>>       fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
> >>>>       fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) =
> >>>> -1 EINVAL (Invalid argument)
> >>>>
> >>>> Now, the second mount command sets just the "ro" flag, which will return
> >>>> after vfs_parse_sb_flag(), before getting to ovl_parse_param():
> >>>>
> >>>>       fspick(3, "", FSPICK_NO_AUTOMOUNT|FSPICK_EMPTY_PATH) = 4
> >>>>       fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
> >>>>
> >>>> After applying my patch and running the first mount command again, we
> >>>> can set that this flag is set only after setting all the strings:
> >>>>
> >>>>       fsconfig(4, FSCONFIG_SET_STRING, "lowerdir", "/tmp/ovl/lower", 0) = 0
> >>>>       fsconfig(4, FSCONFIG_SET_STRING, "upperdir", "/tmp/ovl/upper", 0) = 0
> >>>>       fsconfig(4, FSCONFIG_SET_STRING, "workdir", "/tmp/ovl/work", 0) = 0
> >>>>       fsconfig(4, FSCONFIG_SET_STRING, "uuid", "on", 0) = 0
> >>>>       fsconfig(4, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
> >>>>
> >>>> I understood that the patch that I proposed is wrong, and now I wonder
> >>>> if the kernel needs to be fixed at all, or if the bug is how mount is
> >>>> using fsconfig() in the first mount command?
> >>>
> >
> > If you ask me, when a user does:
> > /usr/bin/mount /tmp/dir2/ovl-mnt -o remount,ro
> >
> > The library only needs to do the FSCONFIG_SET_FLAG command and has no
> > business re-sending the other config commands, but that's just me.
> >
>
> Yes, this makes sense to me as well.
>
> > BTW, which version of libmount (mount --version) are you using?
> > I think there were a few problematic versions when the new mount api
> > was first introduced.
> >
>
> mount from util-linux 2.41 (libmount 2.41.0: btrfs, verity, namespaces,
> idmapping, fd-based-mount, statmount, assert, debug)
>

All right, I upgraded my test machine to a newer distro so
I see those errors.

I will post a patch to xfstests to use LIBMOUNT_FORCE_MOUNT2
for overlayfs remount.

Thanks,
Amir.