linux-kernel - Re: [PATCH v2] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240711141318.95c48cf7295f5e2d6f659aac@linux-foundation.org>
Date: Thu, 11 Jul 2024 14:13:18 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: zhangchun <zhang.chuna@....com>
Cc: <jiaoxupo@....com>, <linux-kernel@...r.kernel.org>,
 <linux-mm@...ck.org>, <shaohaojize@....com>, <zhang.zhansheng@....com>,
 <zhang.zhengming@....com>
Subject: Re: [PATCH v2] mm: Give kmap_lock before call flush_tlb_kernel_rang，avoid kmap_high deadlock.

On Thu, 11 Jul 2024 15:07:56 +0800 zhangchun <zhang.chuna@....com> wrote:

> >> --- a/mm/highmem.c
> >> +++ b/mm/highmem.c
> >> @@ -220,8 +220,11 @@ static void flush_all_zero_pkmaps(void)
> >>  		set_page_address(page, NULL);
> >>  		need_flush = 1;
> >>  	}
> >> -	if (need_flush)
> >> +	if (need_flush) {
> >> +		unlock_kmap();
> >>  		flush_tlb_kernel_range(PKMAP_ADDR(0), PKMAP_ADDR(LAST_PKMAP));
> >> +		lock_kmap();
> >> +	}
> >>  }
> 
> >Why is dropping the lock like this safe?  What data is it protecting and why is it OK to 
> >leave that data unprotected here?
> 
> kmap_lock is used to protect pkmap_count, pkmap_page_table and last_pkmap_nr(static variable). 
> When call flush_tlb_kernel_range(PKMAP_ADDR(0), PKMAP_ADDR(LAST_PKMAP)), flush_tlb_kernel_range
> will neither modify nor read these variables. Leave that data unprotected here is safe.

No, the risk here is that when the lock is dropped, other threads will
modify the data.  And when this thread (the one running
flush_all_zero_pkmaps()) retakes the lock, that data may now be
unexpectedly altered.