Re: [PATCH RFC v2 1/2] mm: Don't skip swap entry even if zap_details specified

[Peter Xu <peterx@redhat.com>]

On Sun, Jan 23, 2022 at 10:29:40PM -0800, Hugh Dickins wrote:
> > However, as I stated before, all these use cases always have another step to
> > take the lock and redo the range.  Then even if some migration entry got
> > wrongly skipped it'll always be fixed.  What we need to find is some caller
> > that calls zap_pte_range() without later taking the page lock and redo that.
> > That's the only possibility to trigger a real issue on the shmem accounting.
> 
> I agree that the fallback "if (folio_mapped() unmap_mapping_folio()",
> while holding folio lock, ensures that there cannot be a migration entry
> substituted for present pte at that time, so no problem if migration entry
> was wrongly skipped on the earlier unlocked pass.
> 
> But you're forgetting the complementary mistake: that the earlier unlocked
> pass might have zapped a migration entry (corresponding to an anon COWed
> page) when it should have skipped it (while punching a hole).

IMHO we won't wrongly zap a migration entry because when it's file backed we've
got non-NULL zap_details, so we'll skip all migration entries.  IOW, we can
only wrongly skip some entries, not wrongly zap some.

But I get your point, and thanks for pointing out what I missed - I think I
forgot the private mappings completely somehow when writting that up..

I have a quick idea on reproducer now (perhaps file size shrinking on private
pages being swapped out), I'll try to write a real reproducer and update later.

[...]

> I did not understand what you were asking there; but in your followup
> mail, I think you came to understand what I meant better.  Yes, I
> believe you could safely replace struct address_space *zap_mapping
> by a more understandable boolean (skip_cows? its inverse would be
> easier to understand, but we don't want almost everyone to have to
> pass a zap_details initialized to true there).

The only even_cows==true for zap_details is with unmap_mapping_range(), where
its caller passed over even_cows==true as parameter.  So IMHO that helper
helped to construct the zap_details anyway.

I'll try it out starting with naming it zap_details.even_cows; I'll make it the
last patch as a cleanup.

> > > > > > >  			rss[mm_counter(page)]--;
> > > > > > >  		}
> > > 
> > > I have given no thought as to whether more "else"s are needed there.
> > 
> > It's hwpoison that's in the else.  Nothing else should.
> > 
> > I didn't mention it probably because I forgot.  I did think about it when
> > drafting, and IMHO we should simply zap that hwpoison entry because:
> > 
> >   (1) Zap means the user knows this data is meaningless, so at least we
> >       shouldn't SIGBUS for that anymore.
> > 
> >   (2) If we keep it there, it could errornously trigger SIGBUS later if the
> >       guest accessed that pte again somehow.
> > 
> > I plan to mention that in the commit message, but I can also add some comments
> > directly into the code.  Let me know your thoughts.
> 
> It's comes down, again, to what punching a hole in a file should do
> to the private data that has been COWed from it.  Strictly, it should
> not interfere with it, even when the COWed page has become poisonous:
> the entry should be left in to generate SIGBUS.  Whereas ordinary
> unmapping or truncating or MADV_DONTNEEDing would zap it.

Makes sense, I'll take care of that in the new version too.  Thanks,

-- 
Peter Xu