linux-kernel - Re: [v3 1/3] mm/rmap: Recognize non-writable TLB entries during TLB batch flush

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 30 Oct 2023 07:52:05 +0000
From:   Nadav Amit <namit@...are.com>
To:     Byungchul Park <byungchul@...com>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>,
        "kernel_team@...ynix.com" <kernel_team@...ynix.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "ying.huang@...el.com" <ying.huang@...el.com>,
        "xhao@...ux.alibaba.com" <xhao@...ux.alibaba.com>,
        "mgorman@...hsingularity.net" <mgorman@...hsingularity.net>,
        "hughd@...gle.com" <hughd@...gle.com>,
        "willy@...radead.org" <willy@...radead.org>,
        "david@...hat.com" <david@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
Subject: Re: [v3 1/3] mm/rmap: Recognize non-writable TLB entries during TLB
 batch flush


Below are some points you might find useful:

> +
> /*
>  * Blindly accessing user memory from NMI context can be dangerous
>  * if we're in the middle of switching the current user task or
> diff --git a/include/linux/mm_types_task.h b/include/linux/mm_types_task.h
> index aa44fff8bb9d..35ba9425d48d 100644
> --- a/include/linux/mm_types_task.h
> +++ b/include/linux/mm_types_task.h
> @@ -59,8 +59,8 @@ struct tlbflush_unmap_batch {
> 	 */
> 	struct arch_tlbflush_unmap_batch arch;
> 
> -	/* True if a flush is needed. */
> -	bool flush_required;
> +	/* The number of flush requested. */

Number of what? Base pages I presume.

> +	int nr_flush_required;

Perhaps unsigned would be better suited?

> 
> 	/*
> 	 * If true then the PTE was dirty when unmapped. The entry must be
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 77f01ac385f7..63189c023357 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1324,6 +1324,7 @@ struct task_struct {
> #endif
> 
> 	struct tlbflush_unmap_batch	tlb_ubc;
> +	struct tlbflush_unmap_batch	tlb_ubc_nowr;

tlb_ubc_nowr is - I think - less informative the tlb_ubc_ro (and a comment
would be useful).

[snip]

> 
> +
> +int nr_flush_required(void)
> +{
> +	return current->tlb_ubc.nr_flush_required;
> +}
> +
> +int nr_flush_required_nowr(void)
> +{
> +	return current->tlb_ubc_nowr.nr_flush_required;
> +}

I haven’t gone through the users of these functions yet, as they are not included
in this patch (which is usually not great).

Anyhow, it might be a bit wasteful to have a function call for such a function. See
if it is possible to avoid that call.

> +
> /*
>  * Flush TLB entries for recently unmapped pages from remote CPUs. It is
>  * important if a PTE was dirty when it was unmapped that it's flushed
> @@ -615,11 +641,12 @@ void try_to_unmap_flush(void)
> {
> 	struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
> 
> -	if (!tlb_ubc->flush_required)
> +	fold_ubc_nowr();
> +	if (!tlb_ubc->nr_flush_required)
> 		return;
> 
> 	arch_tlbbatch_flush(&tlb_ubc->arch);
> -	tlb_ubc->flush_required = false;
> +	tlb_ubc->nr_flush_required = 0;
> 	tlb_ubc->writable = false;
> }
> 
> @@ -627,8 +654,9 @@ void try_to_unmap_flush(void)
> void try_to_unmap_flush_dirty(void)
> {
> 	struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
> +	struct tlbflush_unmap_batch *tlb_ubc_nowr = &current->tlb_ubc_nowr;
> 
> -	if (tlb_ubc->writable)
> +	if (tlb_ubc->writable || tlb_ubc_nowr->writable)
> 		try_to_unmap_flush();
> }
> 
> @@ -645,15 +673,16 @@ void try_to_unmap_flush_dirty(void)
> static void set_tlb_ubc_flush_pending(struct mm_struct *mm, pte_t pteval,
> 				      unsigned long uaddr)
> {
> -	struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
> +	struct tlbflush_unmap_batch *tlb_ubc;
> 	int batch;
> 	bool writable = pte_dirty(pteval);
> 
> 	if (!pte_accessible(mm, pteval))
> 		return;
> 
> +	tlb_ubc = pte_write(pteval) || writable ? &current->tlb_ubc : &current->tlb_ubc_nowr;

Using the ternary operator here is a bit confusing. You can use an “if”
instead or if you mind is set doing it this way at least make it easier to
read:

	tlb_ubc = (pte_write(pteval) || writable) ? &current->tlb_ubc :
						    &current->tlb_ubc_nowr;

And of course, add a comment.

> 	arch_tlbbatch_add_pending(&tlb_ubc->arch, mm, uaddr);
> -	tlb_ubc->flush_required = true;
> +	tlb_ubc->nr_flush_required += 1;

Presumably overflow is impossible for other reasons, but something like that
worries me.