Re: [PATCH 06/32] sched: Add task_struct->faults_disabled_mapping

Jan Kara
2023-05-10T01:07:46Z

On Tue 09-05-23 12:56:31, Kent Overstreet wrote:
> From: Kent Overstreet <kent.overstreet@gmail.com>
>
> This is used by bcachefs to fix a page cache coherency issue with
> O_DIRECT writes.
>
> Also relevant: mapping->invalidate_lock, see below.
>
> O_DIRECT writes (and other filesystem operations that modify file data
> while bypassing the page cache) need to shoot down ranges of the page
> cache - and additionally, need locking to prevent those pages from
> pulled back in.
>
> But O_DIRECT writes invoke the page fault handler (via get_user_pages),
> and the page fault handler will need to take that same lock - this is a
> classic recursive deadlock if userspace has mmaped the file they're DIO
> writing to and uses those pages for the buffer to write from, and it's a
> lock ordering deadlock in general.
>
> Thus we need a way to signal from the dio code to the page fault handler
> when we already are holding the pagecache add lock on an address space -
> this patch just adds a member to task_struct for this purpose. For now
> only bcachefs is implementing this locking, though it may be moved out
> of bcachefs and made available to other filesystems in the future.

It would be nice to have at least a link to the code that's actually using
the field you are adding.

Also I think we were already through this discussion [1] and we ended up
agreeing that your scheme actually solves only the AA deadlock but a
malicious userspace can easily create AB BA deadlock by running direct IO
to file A using mapped file B as a buffer *and* direct IO to file B using
mapped file A as a buffer.

[1] https://lore.kernel.org/all/20191218124052.GB19387@quack2.suse.cz

> ---------------------------------
>
> The closest current VFS equivalent is mapping->invalidate_lock, which
> comes from XFS. However, it's not used by direct IO. Instead, direct IO
> paths shoot down the page cache twice - before starting the IO and at
> the end, and they're still technically racy w.r.t. page cache coherency.
>
> This is a more complete approach: in the future we might consider
> replacing mapping->invalidate_lock with the bcachefs code.

Yes, and this is because we never provided 100% consistent buffered VS
direct IO behavior on the same file exactly because we never found the
complexity worth the usefulness...

Honza

>
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Darrick J. Wong <djwong@kernel.org>
> Cc: linux-fsdevel@vger.kernel.org
> ---
> include/linux/sched.h | 1 +
> init/init_task.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 63d242164b..f2a56f64f7 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -869,6 +869,7 @@ struct task_struct {
>
> struct mm_struct *mm;
> struct mm_struct *active_mm;
> + struct address_space *faults_disabled_mapping;
>
> int exit_state;
> int exit_code;
> diff --git a/init/init_task.c b/init/init_task.c
> index ff6c4b9bfe..f703116e05 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -85,6 +85,7 @@ struct task_struct init_task
> .nr_cpus_allowed= NR_CPUS,
> .mm = NULL,
> .active_mm = &init_mm,
> + .faults_disabled_mapping = NULL,
> .restart_block = {
> .fn = do_no_restart_syscall,
> },
> --
> 2.40.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR