On Tue, Jan 14, 2025 at 01:13:36PM -0800, Dave Hansen wrote: > On 1/14/25 12:41, Lorenzo Stoakes wrote: > ... > > However, MADV_HWPOISON, MADV_SOFT_OFFLINE seems fundamentally broken for tagged > > addresses: > > > > #ifdef CONFIG_MEMORY_FAILURE > > if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE) > > return madvise_inject_error(behavior, start, start + len_in); > > #endif > > > > ^ this is invoked before untagged_addr_remote() is called (as no mmap lock is > > acquired) and so no attempt at untagging happens at all...! > > Except this call path: > > madvise_inject_error() -> > get_user_pages_fast() -> > gup_fast_fallback() > > does its own untagging: > > start = untagged_addr(start) & PAGE_MASK; > Yeah you're right! Good spot. > It might also have some funky behavior if start+len_in overflows. But, > just as in the other case, it's invalid to begin with so I think > userspace kinda gets to keep the pieces. Right yeah. > > But I do 100% agree that this is non-obvious. In a perfect world, tagged > addresses would get untagged at the user/kernel boundary in _one_ choke > point. But the world is hard and that would make things too easy and > then we wouldn't get paid the big bucks. ;) Yeah agreed especially on that last bit ;) I think it'd be good to have a comment there, I will stick on my todo to add one. Or Liam - if you're doing some changes here - maybe you could add? Just something highlighting that gup_fast does the untagging? > > To clarify things, I don't think it'd be the worst thing to just move > the madvise_inject_error() down and have that case acquire > mmap_read_lock(). Sure, it's not required, but it's basically debugging > code and I can't imagine it's avoiding the lock for performance reasons. Yeah it's odd that, but that code is going to the lengths of using gup_fast so I have to assume that maybe some debug user really does care about perf? It'd need some more digging to really feel confident to use a lock there I think.
