Re: [PATCH v2] coredump: exit_files() in coredump_wait() if MMF_DUMP_MAPPED_SHARED is not set

[Pedro Falcato]

On Thu, Jun 18, 2026 at 05:07:56PM +0100, Pedro Falcato wrote:
> On Thu, Jun 18, 2026 at 11:03:01PM +0800, Xin Zhao wrote:
> > A coredump typically takes some time to complete. If we happen to hold a
> > write lock with flock just before triggering the coredump, that write lock
> > will not be released during the entire coredump process. As a result,
> > other processes attempting to acquire the same write lock may experience
> > significant delays.
> > 
> > To address this, call exit_files() in the end of coredump_wait(), if
> > MMF_DUMP_MAPPED_SHARED is not set.
> > 
> > Signed-off-by: Xin Zhao <jackzxcui1989@xxxxxxx>
> > ---
> > 
> > Change in v2:
> > - Get rid of the implement of adding new fcntl API, the issue does not
> >   worth inflicting the cost on everyone,
> >   as suggested by Al Viro.
> > - Call exit_files() in coredump_wait(),
> >   as suggested by Eric W. Biederman.
> >   Add MMF_DUMP_MAPPED_SHARED mm_flags_test() check to filter cases that
> >   need to dump file-backed shared memory.
> > 
> > v1:
> > - Link to v1: https://lore.kernel.org/all/20260618030700.2511668-1-jackzxcui1989@xxxxxxx/
> > ---
> >  fs/coredump.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index bb6fdb1f4..e20baf44f 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -548,6 +548,9 @@ static int coredump_wait(int exit_code, struct core_state *core_state)
> >  		}
> >  	}
> >  
> > +	if (!mm_flags_test(MMF_DUMP_MAPPED_SHARED, tsk->mm))
> > +		exit_files(tsk);
> 
> Memory mapped files keep their own separate references to the files
> (in struct vm_area_struct::vm_file), so there is no need to attempt to
> work around this. Unless I'm misunderstanding what you're attempting
> to work around.

Waiit, I think I get it - you have a flock on a file, and you're scared
that if you unlock early, some other process can lock it and modify some other
file we have mapped? If so, that does make some sense. Please add that as
a comment and/or into the git log, because it feels very much non-obvious
to me.

-- 
Pedro