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
