On 2024/7/3 00:15, Hugh Dickins wrote:
On Tue, 2 Jul 2024, Baolin Wang wrote:
On 2024/7/2 15:40, Hugh Dickins wrote:
Even on 6.10-rc6, I've been seeing elusive "Bad page state"s (often on
flags when freeing, yet the flags shown are not bad: PG_locked had been
set and cleared??), and VM_BUG_ON_PAGE(page_ref_count(page) == 0)s from
deferred_split_scan()'s folio_put(), and a variety of other BUG and WARN
symptoms implying double free by deferred split and large folio migration.
6.7 commit 9bcef5973e31 ("mm: memcg: fix split queue list crash when large
folio migration") was right to fix the memcg-dependent locking broken in
85ce2c517ade ("memcontrol: only transfer the memcg data for migration"),
but missed a subtlety of deferred_split_scan(): it moves folios to its own
local list to work on them without split_queue_lock, during which time
folio->_deferred_list is not empty, but even the "right" lock does nothing
to secure the folio and the list it is on.
Fortunately, deferred_split_scan() is careful to use folio_try_get(): so
folio_migrate_mapping() can avoid the race by folio_undo_large_rmappable()
while the old folio's reference count is temporarily frozen to 0 - adding
such a freeze in the !mapping case too (originally, folio lock and
(I should have added "isolation and" into that list of conditions.)
unmapping and no swap cache left an anon folio unreachable, so no freezing
was needed there: but the deferred split queue offers a way to reach it).
Thanks Hugh.
But after reading your analysis, I am concerned that the
folio_undo_large_rmappable() and deferred_split_scan() may still encounter a
race condition with the local list, even with your patch.
Suppose folio A has already been queued into the local list in
deferred_split_scan() by thread A, but fails to 'folio_trylock' and then
releases the reference count. At the same time, folio A can be frozen by
another thread B in folio_migrate_mapping(). In such a case,
folio_undo_large_rmappable() would remove folio A from the local list without
*any* lock protection, creating a race condition with the local list iteration
in deferred_split_scan().
It's a good doubt to raise, but I think we're okay: because Kirill
designed the deferred split list (and its temporary local list) to
be safe in that way.
You're right that if thread B's freeze to 0 wins the race, thread B
will be messing with a list on thread A's stack while thread A is
quite possibly running; but thread A will not leave that stack frame
without taking again the split_queue_lock which thread B holds while
removing from the list.
We would certainly not want to introduce such a subtlety right now!
But never mind page migration, this is how it has always worked when
racing with the folio being freed at the same time - maybe
deferred_split_scan() wins the freeing race and is the one to remove
folio from deferred split list, or maybe the other freer does that.
Yes, thanks for explanation. And after thinking more, the
'list_for_each_entry_safe' in deferred_split_scan() can maintain the
list iteration safety, so I think this is safe.
I forget whether there was an initial flurry of races to be fixed when
it came in, but it has been stable against concurrent freeing for years.
Please think this over again: do not trust my honeyed words!
Anyway, I think this patch can still fix some possible races. Feel free to
add:
Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
Thanks, but I certainly don't want this to go into the tree if it's
still flawed as you suggest.
Now I have no doubt for this fix, and please continue to keep my
Reviewed-by tag, thanks.
