Message-ID: <CACKH++ZXsLOhtReCucvxkqUATcXNtuW3A=idjskOt+fdme35Jg@mail.gmail.com>
Date: Wed, 27 Nov 2024 22:39:53 +0900
From: Rui Ueyama <rui314@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, Jan Kara <jack@...e.cz>, 
	Amir Goldstein <amir73il@...il.com>, Josef Bacik <josef@...icpanda.com>, 
	Thorsten Leemhuis <regressions@...mhuis.info>, regressions@...ts.linux.dev, 
	LKML <linux-kernel@...r.kernel.org>, 
	Linux-fsdevel <linux-fsdevel@...r.kernel.org>, Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [REGRESSION] mold linker depends on ETXTBSY, but open(2) no
 longer returns it

On Wed, Nov 27, 2024 at 9:11 PM Christian Brauner <brauner@...nel.org> wrote:
>
> On Wed, Nov 27, 2024 at 07:33:53AM +0900, Rui Ueyama wrote:
> > On Mon, Nov 11, 2024 at 9:02 PM Thorsten Leemhuis
> > <regressions@...mhuis.info> wrote:
> > >
> > > [adding a few CCs, dropping stable]
> > >
> > > On 28.10.24 12:15, Rui Ueyama wrote:
> > > > I'm the creator and the maintainer of the mold linker
> > > > (https://github.com/rui314/mold). Recently, we discovered that mold
> > > > started causing process crashes in certain situations due to a change
> > > > in the Linux kernel. Here are the details:
> > > >
> > > > - In general, overwriting an existing file is much faster than
> > > > creating an empty file and writing to it on Linux, so mold attempts to
> > > > reuse an existing executable file if it exists.
> > > >
> > > > - If a program is running, opening the executable file for writing
> > > > previously failed with ETXTBSY. If that happens, mold falls back to
> > > > creating a new file.
> > > >
> > > > - However, the Linux kernel recently changed the behavior so that
> > > > writing to an executable file is now always permitted
> > > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2a010c412853).
> > >
> > > FWIW, that is 2a010c41285345 ("fs: don't block i_writecount during
> > > exec") [v6.11-rc1] from Christian Brauner.
> > >
> > > > That caused mold to write to an executable file even if there's a
> > > > process running that file. Since changes to mmap'ed files are
> > > > immediately visible to other processes, any processes running that
> > > > file would almost certainly crash in a very mysterious way.
> > > > Identifying the cause of these random crashes took us a few days.
> > > >
> > > > Rejecting writes to an executable file that is currently running is a
> > > > well-known behavior, and Linux had operated that way for a very long
> > > > time. So, I don’t believe relying on this behavior was our mistake;
> > > > rather, I see this as a regression in the Linux kernel.
> > > >
> > > > Here is a bug report to the mold linker:
> > > > https://github.com/rui314/mold/issues/1361
> > >
> > > Thx for the report. I might be missing something, but from here it looks
> > > like nothing happened. So please allow me to ask:
> > >
> > > What's the status? Did anyone look into this? Is this sill happening?
>
> Linus, it seems that the mold linker relies on the deny_write_access()
> mechanism for executables. The mold linker tries to open a file for
> writing and if ETXTBSY is returned mold falls back to creating a new
> file.
>
> There is now a fix in mold upstream in
> https://github.com/rui314/mold/commit/8e4f7b53832d8af4f48a633a8385cbc932d1944e
>
> However, mold upstream still insists on a revert (no judgement on my
> part in case that sentence is misinterpreted).

I don't have a strong opinion on whether returning ETXTBSY is
desirable or not. We can cooperate to make a smooth transition to the
new behavior of open(2). That being said, making an abrupt kernel
change that breaks userland in a very mysterious way is, in my
opinion, not acceptable. I'm not personally affected by this issue,
but I needed to speak for our users who may upgrade their kernels
before upgrading their linker.

> Note, that the revert will cause issues for the fanotify pre-content
> hook patch series in [1] which was the cause for the removal of the
> deny_write_access() protection for executables so that on page faults
> the contents of executables could be filled-in by userspace. This is
> useful when dealing with very large executables and is apparently used
> by Meta.
>
> [1]: https://lore.kernel.org/r/20241121112218.8249-1-jack@suse.cz
>
> While Amir tells me that they may have a way around this I expect this
> to be hacky.
>
> This will also trigger a revert/rework of the LTP testsuite which has
> adapted various tests to the deny_write_access() removal for
> executables.
>
> There's been some delay in responding to this after my initial comment
> on Github because I entered into a month of sickness. So I just got
> reminded of this issue now. In any case, here's a tag that you can pull
> if you agree with the revert.
>
> The following changes since commit 7eef7e306d3c40a0c5b9ff6adc9b273cc894dbd5:
>
>   Merge tag 'for-6.13/dm-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm (2024-11-25 18:54:00 -0800)
>
> are available in the Git repository at:
>
>   git@...olite.kernel.org:pub/scm/linux/kernel/git/vfs/vfs tags/vfs-6.13.exec.deny_write_access.revert
>
> for you to fetch changes up to 3b832035387ff508fdcf0fba66701afc78f79e3d:
>
>   Revert "fs: don't block i_writecount during exec" (2024-11-27 12:51:30 +0100)
>
> ----------------------------------------------------------------
> vfs-6.13.exec.deny_write_access.revert
>
> ----------------------------------------------------------------
> Christian Brauner (1):
>       Revert "fs: don't block i_writecount during exec"
>
>  fs/binfmt_elf.c       |  2 ++
>  fs/binfmt_elf_fdpic.c |  5 ++++-
>  fs/binfmt_misc.c      |  7 +++++--
>  fs/exec.c             | 23 +++++++++++++++--------
>  kernel/fork.c         | 26 +++++++++++++++++++++++---
>  5 files changed, 49 insertions(+), 14 deletions(-)

Powered by blists - more mailing lists