On 2023/4/6 22:57, Aaron Lu wrote:
On Thu, Apr 06, 2023 at 10:04:16PM +0800, Aaron Lu wrote:
On Tue, Apr 04, 2023 at 11:47:16PM +0800, Rongwei Wang wrote:
The si->lock must be held when deleting the si from the available list. Otherwise, another thread can re-add the si to the available list, which can lead to memory corruption. The only place we have found where this happens is in the swapoff path. This case can be described as below:
core 0 core 1 swapoff
del_from_avail_list(si) waiting
try lock si->lock acquire swap_avail_lock and re-add si into swap_avail_head
confused here.If del_from_avail_list(si) finished in swaoff path, then this si should not exist in any of the per-node avail list and core 1 should not be able to re-add it.
I think a possible sequence could be like this:
cpuX cpuY swapoff put_swap_folio()
del_from_avail_list(si) taken si->lock spin_lock(&si->lock);
swap_range_free() was_full && SWP_WRITEOK -> re-add! drop si->locktaken si->lock proceed removing si
End result: si left on avail_list after being swapped off.
The problem is, in add_to_avail_list(), it has no idea this si is being swapped off and taking si->lock then del_from_avail_list() could avoid this problem, so I think this patch did the right thing but the changelog about how this happened needs updating and after that:
Hi Aaron
That's my fault. Actually, I don't refers specifically to swap_range_free() path in commit, mainly because cpuY can stand all threads which is waiting swap_avail_lock, then to call add_to_avail_list(), not only swap_range_free(), e.g. maybe swapon().
Reviewed-by: Aaron Lu aaron.lu@intel.com
Thanks for your time.
-wrw
Thanks, Aaron