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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAOQ4uxieVpcGHoD2Q+tND-2R7137-VZSg4mDwAx3UBoU6wJZmA@mail.gmail.com>
Date: Sun, 25 May 2025 12:39:07 +0200
From: Amir Goldstein <amir73il@...il.com>
To: André Almeida <andrealmeid@...lia.com>
Cc: Karel Zak <kzak@...hat.com>, Christian Brauner <brauner@...nel.org>, 
	Miklos Szeredi <miklos@...redi.hu>, linux-unionfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kernel-dev@...lia.com, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] ovl: Allow mount options to be parsed on remount

On Thu, May 22, 2025 at 5:22 PM André Almeida <andrealmeid@...lia.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@...lia.com> wrote:
> >>
> >> Em 22/05/2025 06:52, Amir Goldstein escreveu:
> >>> On Thu, May 22, 2025 at 8:20 AM André Almeida <andrealmeid@...lia.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@...lia.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.