From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org, thuth@redhat.com,
        frankja@linux.ibm.com, borntraeger@de.ibm.com,
        Ulrich.Weigand@de.ibm.com, heiko.carstens@de.ibm.com,
        david@redhat.com, ultrachin@163.com, akpm@linux-foundation.org,
        vbabka@suse.cz, brookxu.cn@gmail.com, xiaoggchen@tencent.com,
        linuszeng@tencent.com, yihuilu@tencent.com, mhocko@suse.com,
        daniel.m.jordan@oracle.com, axboe@kernel.dk, legion@kernel.org,
        peterz@infradead.org, aarcange@redhat.com, christian@brauner.io,
        tglx@linutronix.de
In-Reply-To: <20211111095008.264412-3-imbrenda@linux.ibm.com> (Claudio
        Imbrenda's message of "Thu, 11 Nov 2021 10:50:05 +0100")
References: <20211111095008.264412-1-imbrenda@linux.ibm.com>
        <20211111095008.264412-3-imbrenda@linux.ibm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
Date:   Thu, 11 Nov 2021 12:43:47 -0600
Message-ID: <87y25uzg64.fsf@email.froward.int.ebiederm.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [RFC v1 1/4] exit: add arch mmput hook in exit_mm
Precedence: bulk

Claudio Imbrenda <imbrenda@linux.ibm.com> writes:

> This simple patch adds a hook for the mmput in exit_mm. This allows
> archs to perform the mmput in custom ways if desired (e.g.
> asynchronously)
>
> No functional change intended.

Ouch! No.

You changes don't include an architecture taking advantage of this
so there is not way to see how this is used in practice and maintain
the code.

Further you are making the generic code much harder to read and
follow replacing generic code with something random that some buggy
architecture implements that no one understands.

Saying "some buggy architecture implements and no one understands"
is a bit hyperbole but that is how these hooks feel when digging in
to changing the code.  It takes weeks to months to read through
ask questions etc to clean hooks like this up and change the
code in an appropriate way.

As things are ill specified like this really do need change eventually.

So please use much more targeted routines for architecture code to call.
Especially when dealing with something as fundamental as the lifetime of
a core kernel object.

If you want an example of how silly some of these kinds of things
can get just look at arch_ptrace_stop and sigkill_pending.  Linus has
just merged my fixes for these things.  There are worse examples, I just
remember them off the top of my head.

If this is merged as is, this feels like code that will be subtly wrong
for a decade before someone figures it out and fixes it.

Eric


> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
>  include/asm-generic/mmu_context.h | 4 ++++
>  kernel/exit.c                     | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/mmu_context.h b/include/asm-generic/mmu_context.h
> index 91727065bacb..900931a6a105 100644
> --- a/include/asm-generic/mmu_context.h
> +++ b/include/asm-generic/mmu_context.h
> @@ -73,4 +73,8 @@ static inline void deactivate_mm(struct task_struct *tsk,
>  }
>  #endif
>  
> +#ifndef arch_exit_mm_mmput
> +#define arch_exit_mm_mmput mmput
> +#endif
> +
>  #endif /* __ASM_GENERIC_MMU_CONTEXT_H */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index f702a6a63686..6eb1fdcc434e 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -504,7 +504,7 @@ static void exit_mm(void)
>  	task_unlock(current);
>  	mmap_read_unlock(mm);
>  	mm_update_next_owner(mm);
> -	mmput(mm);
> +	arch_exit_mm_mmput(mm);
>  	if (test_thread_flag(TIF_MEMDIE))
>  		exit_oom_victim();
>  }