| Message ID | 20230511182426.1898675-1-axelrasmussen@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] mm: userfaultfd: add new UFFDIO_SIGBUS ioctl | expand |
On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: > On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen > <axelrasmussen@google.com> wrote: > > > > So the basic way to use this new feature is: > > > > - On the new host, the guest's memory is registered with userfaultfd, in > > either MISSING or MINOR mode (doesn't really matter for this purpose). > > - On any first access, we get a userfaultfd event. At this point we can > > communicate with the old host to find out if the page was poisoned. > > - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker > > so any future accesses will SIGBUS. Because the pte is now "present", > > future accesses won't generate more userfaultfd events, they'll just > > SIGBUS directly. > > I want to clarify the SIGBUS mechanism here when KVM is involved, > keeping in mind that we need to be able to inject an MCE into the > guest for this to be useful. > > 1. vCPU gets an EPT violation --> KVM attempts GUP. > 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. > 3. KVM finds that GUP failed and returns -EFAULT. > > This is different than if GUP found poison, in which case KVM will > actually queue up a SIGBUS *containing the address of the fault*, and > userspace can use it to inject an appropriate MCE into the guest. With > UFFDIO_SIGBUS, we are missing the address! > > I see three options: > 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think > this is pointless. > 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a > UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS > instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated > accesses, just like how we get repeated signals for real poison. > 3. Use this in conjunction with the additional KVM EFAULT info that > Anish proposed (the first part of [1]). > > I think option 3 is fine. :) Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) Besides what James mentioned on "missing addr", I didn't quickly see what's the major difference comparing to the old hwpoison injection methods even without the addr requirement. If we want the addr for MCE then it's more of a question to ask. I also didn't quickly see why for whatever new way to inject a pte error we need to have it registered with uffd. Could it be something like MADV_PGERR (even if MADV_HWPOISON won't suffice) so you can inject even without an userfault context (but still usable when uffd registered)? And it'll be alawys nice to have a cover letter too (if there'll be a new version) explaining the bits. Thanks,
On Wed, May 17, 2023 at 06:12:33PM -0400, Peter Xu wrote: > On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: > > On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen > > <axelrasmussen@google.com> wrote: > > > > > > So the basic way to use this new feature is: > > > > > > - On the new host, the guest's memory is registered with userfaultfd, in > > > either MISSING or MINOR mode (doesn't really matter for this purpose). > > > - On any first access, we get a userfaultfd event. At this point we can > > > communicate with the old host to find out if the page was poisoned. > > > - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker > > > so any future accesses will SIGBUS. Because the pte is now "present", > > > future accesses won't generate more userfaultfd events, they'll just > > > SIGBUS directly. > > > > I want to clarify the SIGBUS mechanism here when KVM is involved, > > keeping in mind that we need to be able to inject an MCE into the > > guest for this to be useful. > > > > 1. vCPU gets an EPT violation --> KVM attempts GUP. > > 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. > > 3. KVM finds that GUP failed and returns -EFAULT. > > > > This is different than if GUP found poison, in which case KVM will > > actually queue up a SIGBUS *containing the address of the fault*, and > > userspace can use it to inject an appropriate MCE into the guest. With > > UFFDIO_SIGBUS, we are missing the address! > > > > I see three options: > > 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think > > this is pointless. > > 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a > > UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS > > instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated > > accesses, just like how we get repeated signals for real poison. > > 3. Use this in conjunction with the additional KVM EFAULT info that > > Anish proposed (the first part of [1]). > > > > I think option 3 is fine. :) > > Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) I just remember Axel mentioned this in the commit message, and just in case this is why option 4) was ruled out: They expect that once poisoned, pages can never become "un-poisoned". So, when we live migrate the VM, we need to preserve the poisoned status of these pages. Just to supplement on this point: we do have unpoison (echoing to "debug/hwpoison/hwpoison_unpoison"), or am I wrong? > > Besides what James mentioned on "missing addr", I didn't quickly see what's > the major difference comparing to the old hwpoison injection methods even > without the addr requirement. If we want the addr for MCE then it's more of > a question to ask. > > I also didn't quickly see why for whatever new way to inject a pte error we > need to have it registered with uffd. Could it be something like > MADV_PGERR (even if MADV_HWPOISON won't suffice) so you can inject even > without an userfault context (but still usable when uffd registered)? > > And it'll be alawys nice to have a cover letter too (if there'll be a new > version) explaining the bits. > > Thanks, > > -- > Peter Xu
On Wed, May 17, 2023 at 3:20 PM Peter Xu <peterx@redhat.com> wrote: > > On Wed, May 17, 2023 at 06:12:33PM -0400, Peter Xu wrote: > > On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: > > > On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen > > > <axelrasmussen@google.com> wrote: > > > > > > > > So the basic way to use this new feature is: > > > > > > > > - On the new host, the guest's memory is registered with userfaultfd, in > > > > either MISSING or MINOR mode (doesn't really matter for this purpose). > > > > - On any first access, we get a userfaultfd event. At this point we can > > > > communicate with the old host to find out if the page was poisoned. > > > > - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker > > > > so any future accesses will SIGBUS. Because the pte is now "present", > > > > future accesses won't generate more userfaultfd events, they'll just > > > > SIGBUS directly. > > > > > > I want to clarify the SIGBUS mechanism here when KVM is involved, > > > keeping in mind that we need to be able to inject an MCE into the > > > guest for this to be useful. > > > > > > 1. vCPU gets an EPT violation --> KVM attempts GUP. > > > 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. > > > 3. KVM finds that GUP failed and returns -EFAULT. > > > > > > This is different than if GUP found poison, in which case KVM will > > > actually queue up a SIGBUS *containing the address of the fault*, and > > > userspace can use it to inject an appropriate MCE into the guest. With > > > UFFDIO_SIGBUS, we are missing the address! > > > > > > I see three options: > > > 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think > > > this is pointless. > > > 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a > > > UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS > > > instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated > > > accesses, just like how we get repeated signals for real poison. > > > 3. Use this in conjunction with the additional KVM EFAULT info that > > > Anish proposed (the first part of [1]). > > > > > > I think option 3 is fine. :) > > > > Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) > > I just remember Axel mentioned this in the commit message, and just in case > this is why option 4) was ruled out: > > They expect that once poisoned, pages can never become > "un-poisoned". So, when we live migrate the VM, we need to preserve > the poisoned status of these pages. > > Just to supplement on this point: we do have unpoison (echoing to > "debug/hwpoison/hwpoison_unpoison"), or am I wrong? > > > > > Besides what James mentioned on "missing addr", I didn't quickly see what's > > the major difference comparing to the old hwpoison injection methods even > > without the addr requirement. If we want the addr for MCE then it's more of > > a question to ask. > > > > I also didn't quickly see why for whatever new way to inject a pte error we > > need to have it registered with uffd. Could it be something like > > MADV_PGERR (even if MADV_HWPOISON won't suffice) so you can inject even > > without an userfault context (but still usable when uffd registered)? > > > > And it'll be alawys nice to have a cover letter too (if there'll be a new > > version) explaining the bits. I do plan a v2, if for no other reason than to update the documentation. Happy to add a cover letter with it as well. +Jiaqi back to CC, this is one piece of a larger memory poisoning / recovery design Jiaqi is working on, so he may have some ideas why MADV_HWPOISON or MADV_PGER will or won't work. One idea is, at least for our use case, we have to have the range be userfaultfd registered, because we need to intercept the first access and check at that point whether or not it should be poisoned. But, I think in principle a scheme like this could work: 1. Intercept first access with UFFD 2. Issue MADV_HWPOISON or MADV_PGERR or etc to put a pte denoting the poisoned page in place 3. UFFDIO_WAKE to have the faulting thread retry, see the new entry, and SIGBUS It's arguably slightly weird, since normally UFFD events are resolved with UFFDIO_* operations, but I don't see why it *couldn't* work. Then again I am not super familiar with MADV_HWPOISON, I will have to do a bit of reading to understand if its semantics are the same (future accesses to this address get SIGBUS). > > > > Thanks, > > > > -- > > Peter Xu > > -- > Peter Xu >
Hi, Axel, On Wed, May 17, 2023 at 03:28:36PM -0700, Axel Rasmussen wrote: > I do plan a v2, if for no other reason than to update the > documentation. Happy to add a cover letter with it as well. > > +Jiaqi back to CC, this is one piece of a larger memory poisoning / > recovery design Jiaqi is working on, so he may have some ideas why > MADV_HWPOISON or MADV_PGER will or won't work. > > One idea is, at least for our use case, we have to have the range be > userfaultfd registered, because we need to intercept the first access > and check at that point whether or not it should be poisoned. But, I > think in principle a scheme like this could work: > > 1. Intercept first access with UFFD > 2. Issue MADV_HWPOISON or MADV_PGERR or etc to put a pte denoting the > poisoned page in place > 3. UFFDIO_WAKE to have the faulting thread retry, see the new entry, and SIGBUS > > It's arguably slightly weird, since normally UFFD events are resolved > with UFFDIO_* operations, but I don't see why it *couldn't* work. > > Then again I am not super familiar with MADV_HWPOISON, I will have to > do a bit of reading to understand if its semantics are the same > (future accesses to this address get SIGBUS). Yes, it'll be great if this can be checked up before sending v2. What you said match exactly what I was in mind. I hope it will already work, or we can always discuss what is missing.
On Wed, May 17, 2023 at 3:29 PM Axel Rasmussen <axelrasmussen@google.com> wrote: > > On Wed, May 17, 2023 at 3:20 PM Peter Xu <peterx@redhat.com> wrote: > > > > On Wed, May 17, 2023 at 06:12:33PM -0400, Peter Xu wrote: > > > On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: > > > > On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen > > > > <axelrasmussen@google.com> wrote: > > > > > > > > > > So the basic way to use this new feature is: > > > > > > > > > > - On the new host, the guest's memory is registered with userfaultfd, in > > > > > either MISSING or MINOR mode (doesn't really matter for this purpose). > > > > > - On any first access, we get a userfaultfd event. At this point we can > > > > > communicate with the old host to find out if the page was poisoned. > > > > > - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker > > > > > so any future accesses will SIGBUS. Because the pte is now "present", > > > > > future accesses won't generate more userfaultfd events, they'll just > > > > > SIGBUS directly. > > > > > > > > I want to clarify the SIGBUS mechanism here when KVM is involved, > > > > keeping in mind that we need to be able to inject an MCE into the > > > > guest for this to be useful. > > > > > > > > 1. vCPU gets an EPT violation --> KVM attempts GUP. > > > > 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. > > > > 3. KVM finds that GUP failed and returns -EFAULT. > > > > > > > > This is different than if GUP found poison, in which case KVM will > > > > actually queue up a SIGBUS *containing the address of the fault*, and > > > > userspace can use it to inject an appropriate MCE into the guest. With > > > > UFFDIO_SIGBUS, we are missing the address! > > > > > > > > I see three options: > > > > 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think > > > > this is pointless. > > > > 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a > > > > UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS > > > > instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated > > > > accesses, just like how we get repeated signals for real poison. > > > > 3. Use this in conjunction with the additional KVM EFAULT info that > > > > Anish proposed (the first part of [1]). > > > > > > > > I think option 3 is fine. :) > > > > > > Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) > > > > I just remember Axel mentioned this in the commit message, and just in case > > this is why option 4) was ruled out: > > > > They expect that once poisoned, pages can never become > > "un-poisoned". So, when we live migrate the VM, we need to preserve > > the poisoned status of these pages. > > > > Just to supplement on this point: we do have unpoison (echoing to > > "debug/hwpoison/hwpoison_unpoison"), or am I wrong? If I read unpoison_memory() correctly, once there is a real hardware memory corruption (hw_memory_failure will be set), unpoison will stop working and return EOPNOTSUPP. I know some cloud providers evacuating VMs once a single memory error happens, so not supporting unpoison is probably not a big deal for them. BUT others do keep VM running until more errors show up later, which could be long after the 1st error. > > > > > > > > Besides what James mentioned on "missing addr", I didn't quickly see what's > > > the major difference comparing to the old hwpoison injection methods even > > > without the addr requirement. If we want the addr for MCE then it's more of > > > a question to ask. > > > > > > I also didn't quickly see why for whatever new way to inject a pte error we > > > need to have it registered with uffd. Could it be something like > > > MADV_PGERR (even if MADV_HWPOISON won't suffice) so you can inject even > > > without an userfault context (but still usable when uffd registered)? > > > > > > And it'll be alawys nice to have a cover letter too (if there'll be a new > > > version) explaining the bits. > > I do plan a v2, if for no other reason than to update the > documentation. Happy to add a cover letter with it as well. > > +Jiaqi back to CC, this is one piece of a larger memory poisoning / > recovery design Jiaqi is working on, so he may have some ideas why > MADV_HWPOISON or MADV_PGER will or won't work. Per https://man7.org/linux/man-pages/man2/madvise.2.html, MADV_HWPOISON "is available only for privileged (CAP_SYS_ADMIN) processes." So for a non-root VMM, MADV_HWPOISON is out of option. Another issue with MADV_HWPOISON is, it requires to first successfully get_user_pages_fast(). I don't think it will work if memory is not mapped yet. With the UFFDIO_SIGBUS feature introduced in this patchset, it may even be possible to free the emulated-hwpoison page back to the kernel so we don't lose a 4K page. I didn't find any ref/doc for MADV_PGERR. Is it something you suggest to build, Peter? > > One idea is, at least for our use case, we have to have the range be > userfaultfd registered, because we need to intercept the first access > and check at that point whether or not it should be poisoned. But, I > think in principle a scheme like this could work: > > 1. Intercept first access with UFFD > 2. Issue MADV_HWPOISON or MADV_PGERR or etc to put a pte denoting the > poisoned page in place > 3. UFFDIO_WAKE to have the faulting thread retry, see the new entry, and SIGBUS > > It's arguably slightly weird, since normally UFFD events are resolved > with UFFDIO_* operations, but I don't see why it *couldn't* work. > > Then again I am not super familiar with MADV_HWPOISON, I will have to > do a bit of reading to understand if its semantics are the same > (future accesses to this address get SIGBUS). > > > > > > > > Thanks, > > > > > > -- > > > Peter Xu > > > > -- > > Peter Xu > >
On Wed, May 17, 2023 at 05:43:53PM -0700, Jiaqi Yan wrote: > On Wed, May 17, 2023 at 3:29 PM Axel Rasmussen <axelrasmussen@google.com> wrote: > > > > On Wed, May 17, 2023 at 3:20 PM Peter Xu <peterx@redhat.com> wrote: > > > > > > On Wed, May 17, 2023 at 06:12:33PM -0400, Peter Xu wrote: > > > > On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: > > > > > On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen > > > > > <axelrasmussen@google.com> wrote: > > > > > > > > > > > > So the basic way to use this new feature is: > > > > > > > > > > > > - On the new host, the guest's memory is registered with userfaultfd, in > > > > > > either MISSING or MINOR mode (doesn't really matter for this purpose). > > > > > > - On any first access, we get a userfaultfd event. At this point we can > > > > > > communicate with the old host to find out if the page was poisoned. > > > > > > - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker > > > > > > so any future accesses will SIGBUS. Because the pte is now "present", > > > > > > future accesses won't generate more userfaultfd events, they'll just > > > > > > SIGBUS directly. > > > > > > > > > > I want to clarify the SIGBUS mechanism here when KVM is involved, > > > > > keeping in mind that we need to be able to inject an MCE into the > > > > > guest for this to be useful. > > > > > > > > > > 1. vCPU gets an EPT violation --> KVM attempts GUP. > > > > > 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. > > > > > 3. KVM finds that GUP failed and returns -EFAULT. > > > > > > > > > > This is different than if GUP found poison, in which case KVM will > > > > > actually queue up a SIGBUS *containing the address of the fault*, and > > > > > userspace can use it to inject an appropriate MCE into the guest. With > > > > > UFFDIO_SIGBUS, we are missing the address! > > > > > > > > > > I see three options: > > > > > 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think > > > > > this is pointless. > > > > > 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a > > > > > UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS > > > > > instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated > > > > > accesses, just like how we get repeated signals for real poison. > > > > > 3. Use this in conjunction with the additional KVM EFAULT info that > > > > > Anish proposed (the first part of [1]). > > > > > > > > > > I think option 3 is fine. :) > > > > > > > > Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) > > > > > > I just remember Axel mentioned this in the commit message, and just in case > > > this is why option 4) was ruled out: > > > > > > They expect that once poisoned, pages can never become > > > "un-poisoned". So, when we live migrate the VM, we need to preserve > > > the poisoned status of these pages. > > > > > > Just to supplement on this point: we do have unpoison (echoing to > > > "debug/hwpoison/hwpoison_unpoison"), or am I wrong? > > If I read unpoison_memory() correctly, once there is a real hardware > memory corruption (hw_memory_failure will be set), unpoison will stop > working and return EOPNOTSUPP. > > I know some cloud providers evacuating VMs once a single memory error > happens, so not supporting unpoison is probably not a big deal for > them. BUT others do keep VM running until more errors show up later, > which could be long after the 1st error. We're talking about postcopy migrating a VM has poisoned page on src, rather than on dst host, am I right? IOW, the dest hwpoison should be fake. If so, then I would assume that's the case where all the pages on the dest host is still all good (so hw_memory_failure not yet set, or I doubt the judgement of being a migration target after all)? The other thing is even if dest host has hw poisoned page, I'm not sure whether hw_memory_failure is the only way to solve this. I saw that this is something got worked on before from Zhenwei, David used to have some reasoning on why it was suggested like using a global knob: https://lore.kernel.org/all/d7927214-e433-c26d-7a9c-a291ced81887@redhat.com/ Two major issues here afaics: - Zhenwei's approach only considered x86 hwpoison - it relies on kpte having !present in entries but that's x86 specific rather than generic to memory_failure.c. - It is _assumed_ that hwpoison injection is for debugging only. I'm not sure whether you can fix 1) by some other ways, e.g., what if the host just remember all the hardware poisoned pfns (or remember soft-poisoned ones, but then here we need to be careful on removing them from the list when it's hwpoisoned for real)? It sounds like there's opportunity on providing a generic solution rather than relying on !pte_present(). For 2) IMHO that's not a big issue, you can declare it'll be used in !debug but production systems so as to boost the feature importance with a real use case. So far I'd say it'll be great to leverage what it's already there in linux and make it as generic as possible. The only issue is probably CAP_ADMIN... not sure whether we can have some way to provide !ADMIN somehow, or you can simply work around this issue. > > > > > > > > > > > > Besides what James mentioned on "missing addr", I didn't quickly see what's > > > > the major difference comparing to the old hwpoison injection methods even > > > > without the addr requirement. If we want the addr for MCE then it's more of > > > > a question to ask. > > > > > > > > I also didn't quickly see why for whatever new way to inject a pte error we > > > > need to have it registered with uffd. Could it be something like > > > > MADV_PGERR (even if MADV_HWPOISON won't suffice) so you can inject even > > > > without an userfault context (but still usable when uffd registered)? > > > > > > > > And it'll be alawys nice to have a cover letter too (if there'll be a new > > > > version) explaining the bits. > > > > I do plan a v2, if for no other reason than to update the > > documentation. Happy to add a cover letter with it as well. > > > > +Jiaqi back to CC, this is one piece of a larger memory poisoning / > > recovery design Jiaqi is working on, so he may have some ideas why > > MADV_HWPOISON or MADV_PGER will or won't work. > > Per https://man7.org/linux/man-pages/man2/madvise.2.html, > MADV_HWPOISON "is available only for privileged (CAP_SYS_ADMIN) > processes." So for a non-root VMM, MADV_HWPOISON is out of option. It makes sense to me especially when the page can be shared with other tasks. > > Another issue with MADV_HWPOISON is, it requires to first successfully > get_user_pages_fast(). I don't think it will work if memory is not > mapped yet. Fair point, so probably current MADV_HWPOISON got ruled out. hwpoison-inject seems fine where only the PFN is needed rather than the pte. But same issue on CAP_ADMIN indeed. > > With the UFFDIO_SIGBUS feature introduced in this patchset, it may > even be possible to free the emulated-hwpoison page back to the kernel > so we don't lose a 4K page. > > I didn't find any ref/doc for MADV_PGERR. Is it something you suggest > to build, Peter? That's something I made up just to show my question on why such an interface (even if wanted) needs to be bound to userfaultfd, e.g. a madvise() seems working if someone sololy want to install a poisoned pte. IIUC even with an madvise one may not need CAP_ADMIN since we can limit the op to current mm only, I assume it's safe. Here you'd want to return VM_FAULT_HWPOISON for whatever swap pte you'd like to install (in do_swap_page) with whatever new interface (assuming still a new madvise). As James mentioned, I think KVM liked that to recognize -EHWPOISON from -EFAULT. I'd say we can even consider reusing PTE_MARKER_SWAPIN_ERROR to let it just return VM_FAULT_HWPOISON directly if so. Thanks,
On Thu, May 18, 2023 at 9:05 AM Peter Xu <peterx@redhat.com> wrote: > > On Wed, May 17, 2023 at 05:43:53PM -0700, Jiaqi Yan wrote: > > On Wed, May 17, 2023 at 3:29 PM Axel Rasmussen <axelrasmussen@google.com> wrote: > > > > > > On Wed, May 17, 2023 at 3:20 PM Peter Xu <peterx@redhat.com> wrote: > > > > > > > > On Wed, May 17, 2023 at 06:12:33PM -0400, Peter Xu wrote: > > > > > On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: > > > > > > On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen > > > > > > <axelrasmussen@google.com> wrote: > > > > > > > > > > > > > > So the basic way to use this new feature is: > > > > > > > > > > > > > > - On the new host, the guest's memory is registered with userfaultfd, in > > > > > > > either MISSING or MINOR mode (doesn't really matter for this purpose). > > > > > > > - On any first access, we get a userfaultfd event. At this point we can > > > > > > > communicate with the old host to find out if the page was poisoned. > > > > > > > - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker > > > > > > > so any future accesses will SIGBUS. Because the pte is now "present", > > > > > > > future accesses won't generate more userfaultfd events, they'll just > > > > > > > SIGBUS directly. > > > > > > > > > > > > I want to clarify the SIGBUS mechanism here when KVM is involved, > > > > > > keeping in mind that we need to be able to inject an MCE into the > > > > > > guest for this to be useful. > > > > > > > > > > > > 1. vCPU gets an EPT violation --> KVM attempts GUP. > > > > > > 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. > > > > > > 3. KVM finds that GUP failed and returns -EFAULT. > > > > > > > > > > > > This is different than if GUP found poison, in which case KVM will > > > > > > actually queue up a SIGBUS *containing the address of the fault*, and > > > > > > userspace can use it to inject an appropriate MCE into the guest. With > > > > > > UFFDIO_SIGBUS, we are missing the address! > > > > > > > > > > > > I see three options: > > > > > > 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think > > > > > > this is pointless. > > > > > > 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a > > > > > > UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS > > > > > > instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated > > > > > > accesses, just like how we get repeated signals for real poison. > > > > > > 3. Use this in conjunction with the additional KVM EFAULT info that > > > > > > Anish proposed (the first part of [1]). > > > > > > > > > > > > I think option 3 is fine. :) > > > > > > > > > > Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) > > > > > > > > I just remember Axel mentioned this in the commit message, and just in case > > > > this is why option 4) was ruled out: > > > > > > > > They expect that once poisoned, pages can never become > > > > "un-poisoned". So, when we live migrate the VM, we need to preserve > > > > the poisoned status of these pages. > > > > > > > > Just to supplement on this point: we do have unpoison (echoing to > > > > "debug/hwpoison/hwpoison_unpoison"), or am I wrong? > > > > If I read unpoison_memory() correctly, once there is a real hardware > > memory corruption (hw_memory_failure will be set), unpoison will stop > > working and return EOPNOTSUPP. > > > > I know some cloud providers evacuating VMs once a single memory error > > happens, so not supporting unpoison is probably not a big deal for > > them. BUT others do keep VM running until more errors show up later, > > which could be long after the 1st error. > > We're talking about postcopy migrating a VM has poisoned page on src, > rather than on dst host, am I right? IOW, the dest hwpoison should be > fake. > > If so, then I would assume that's the case where all the pages on the dest > host is still all good (so hw_memory_failure not yet set, or I doubt the > judgement of being a migration target after all)? > > The other thing is even if dest host has hw poisoned page, I'm not sure > whether hw_memory_failure is the only way to solve this. > > I saw that this is something got worked on before from Zhenwei, David used > to have some reasoning on why it was suggested like using a global knob: > > https://lore.kernel.org/all/d7927214-e433-c26d-7a9c-a291ced81887@redhat.com/ > > Two major issues here afaics: > > - Zhenwei's approach only considered x86 hwpoison - it relies on kpte > having !present in entries but that's x86 specific rather than generic > to memory_failure.c. > > - It is _assumed_ that hwpoison injection is for debugging only. > > I'm not sure whether you can fix 1) by some other ways, e.g., what if the > host just remember all the hardware poisoned pfns (or remember > soft-poisoned ones, but then here we need to be careful on removing them > from the list when it's hwpoisoned for real)? It sounds like there's > opportunity on providing a generic solution rather than relying on > !pte_present(). > > For 2) IMHO that's not a big issue, you can declare it'll be used in !debug > but production systems so as to boost the feature importance with a real > use case. > > So far I'd say it'll be great to leverage what it's already there in linux > and make it as generic as possible. The only issue is probably > CAP_ADMIN... not sure whether we can have some way to provide !ADMIN > somehow, or you can simply work around this issue. As you mention below I think the key distinction is the scope - I think MADV_HWPOISON affects the whole system, including other processes. For our purposes, we really just want to "poison" this particular virtual address (the HVA, from the VM's perspective), not even other mappings of the same shared memory. I think that behavior is different from MADV_HWPOISON, at least. > > > > > > > > > > > > > > > > > Besides what James mentioned on "missing addr", I didn't quickly see what's > > > > > the major difference comparing to the old hwpoison injection methods even > > > > > without the addr requirement. If we want the addr for MCE then it's more of > > > > > a question to ask. > > > > > > > > > > I also didn't quickly see why for whatever new way to inject a pte error we > > > > > need to have it registered with uffd. Could it be something like > > > > > MADV_PGERR (even if MADV_HWPOISON won't suffice) so you can inject even > > > > > without an userfault context (but still usable when uffd registered)? > > > > > > > > > > And it'll be alawys nice to have a cover letter too (if there'll be a new > > > > > version) explaining the bits. > > > > > > I do plan a v2, if for no other reason than to update the > > > documentation. Happy to add a cover letter with it as well. > > > > > > +Jiaqi back to CC, this is one piece of a larger memory poisoning / > > > recovery design Jiaqi is working on, so he may have some ideas why > > > MADV_HWPOISON or MADV_PGER will or won't work. > > > > Per https://man7.org/linux/man-pages/man2/madvise.2.html, > > MADV_HWPOISON "is available only for privileged (CAP_SYS_ADMIN) > > processes." So for a non-root VMM, MADV_HWPOISON is out of option. > > It makes sense to me especially when the page can be shared with other > tasks. > > > > > Another issue with MADV_HWPOISON is, it requires to first successfully > > get_user_pages_fast(). I don't think it will work if memory is not > > mapped yet. > > Fair point, so probably current MADV_HWPOISON got ruled out. > hwpoison-inject seems fine where only the PFN is needed rather than the > pte. But same issue on CAP_ADMIN indeed. > > > > > With the UFFDIO_SIGBUS feature introduced in this patchset, it may > > even be possible to free the emulated-hwpoison page back to the kernel > > so we don't lose a 4K page. > > > > I didn't find any ref/doc for MADV_PGERR. Is it something you suggest > > to build, Peter? > > That's something I made up just to show my question on why such an > interface (even if wanted) needs to be bound to userfaultfd, e.g. a > madvise() seems working if someone sololy want to install a poisoned pte. I look at it a bit differently... Even existing UFFDIO_* operations could technically be separated from userfaultfd. You could imagine a MADV_MAP_PAGE instead of UFFDIO_CONTINUE. UFFDIO_COPY is a bit trickier since it takes an argument, but it could be done with process_madvise(). (Granted, I'm not sure this would be useful... But this is equally true for UFFDIO_SIGBUS; it seems non-live-migration use cases could use MADV_HWPOISON, and for live migration use cases we will be using UFFD.) We've sort of setup a convention with userfaultfd where at a high level users are supposed to: 1. Receive events from the uffd 2. Resolve those events with UFFDIO_* ioctls 3. Wake up with UFFDIO_WAKE to retry the fault that generated the original event (can be combined with step 2 of course) So for me, even if MADV_PGERR or similar existed, I would be tempted to add a UFFDIO_SIGBUS as well, even if it just calls the same underlying function to do the same thing, if only for consistency (with the idea "UFFD events are resolved by UFFD ioctls") from the user's perspective. > > IIUC even with an madvise one may not need CAP_ADMIN since we can limit the > op to current mm only, I assume it's safe. > > Here you'd want to return VM_FAULT_HWPOISON for whatever swap pte you'd > like to install (in do_swap_page) with whatever new interface (assuming > still a new madvise). As James mentioned, I think KVM liked that to > recognize -EHWPOISON from -EFAULT. I'd say we can even consider reusing > PTE_MARKER_SWAPIN_ERROR to let it just return VM_FAULT_HWPOISON directly if > so. > > Thanks, > > -- > Peter Xu >
On Thu, May 18, 2023 at 01:38:09PM -0700, Axel Rasmussen wrote: > On Thu, May 18, 2023 at 9:05 AM Peter Xu <peterx@redhat.com> wrote: > > > > On Wed, May 17, 2023 at 05:43:53PM -0700, Jiaqi Yan wrote: > > > On Wed, May 17, 2023 at 3:29 PM Axel Rasmussen <axelrasmussen@google.com> wrote: > > > > > > > > On Wed, May 17, 2023 at 3:20 PM Peter Xu <peterx@redhat.com> wrote: > > > > > > > > > > On Wed, May 17, 2023 at 06:12:33PM -0400, Peter Xu wrote: > > > > > > On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: > > > > > > > On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen > > > > > > > <axelrasmussen@google.com> wrote: > > > > > > > > > > > > > > > > So the basic way to use this new feature is: > > > > > > > > > > > > > > > > - On the new host, the guest's memory is registered with userfaultfd, in > > > > > > > > either MISSING or MINOR mode (doesn't really matter for this purpose). > > > > > > > > - On any first access, we get a userfaultfd event. At this point we can > > > > > > > > communicate with the old host to find out if the page was poisoned. > > > > > > > > - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker > > > > > > > > so any future accesses will SIGBUS. Because the pte is now "present", > > > > > > > > future accesses won't generate more userfaultfd events, they'll just > > > > > > > > SIGBUS directly. > > > > > > > > > > > > > > I want to clarify the SIGBUS mechanism here when KVM is involved, > > > > > > > keeping in mind that we need to be able to inject an MCE into the > > > > > > > guest for this to be useful. > > > > > > > > > > > > > > 1. vCPU gets an EPT violation --> KVM attempts GUP. > > > > > > > 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. > > > > > > > 3. KVM finds that GUP failed and returns -EFAULT. > > > > > > > > > > > > > > This is different than if GUP found poison, in which case KVM will > > > > > > > actually queue up a SIGBUS *containing the address of the fault*, and > > > > > > > userspace can use it to inject an appropriate MCE into the guest. With > > > > > > > UFFDIO_SIGBUS, we are missing the address! > > > > > > > > > > > > > > I see three options: > > > > > > > 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think > > > > > > > this is pointless. > > > > > > > 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a > > > > > > > UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS > > > > > > > instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated > > > > > > > accesses, just like how we get repeated signals for real poison. > > > > > > > 3. Use this in conjunction with the additional KVM EFAULT info that > > > > > > > Anish proposed (the first part of [1]). > > > > > > > > > > > > > > I think option 3 is fine. :) > > > > > > > > > > > > Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) > > > > > > > > > > I just remember Axel mentioned this in the commit message, and just in case > > > > > this is why option 4) was ruled out: > > > > > > > > > > They expect that once poisoned, pages can never become > > > > > "un-poisoned". So, when we live migrate the VM, we need to preserve > > > > > the poisoned status of these pages. > > > > > > > > > > Just to supplement on this point: we do have unpoison (echoing to > > > > > "debug/hwpoison/hwpoison_unpoison"), or am I wrong? > > > > > > If I read unpoison_memory() correctly, once there is a real hardware > > > memory corruption (hw_memory_failure will be set), unpoison will stop > > > working and return EOPNOTSUPP. > > > > > > I know some cloud providers evacuating VMs once a single memory error > > > happens, so not supporting unpoison is probably not a big deal for > > > them. BUT others do keep VM running until more errors show up later, > > > which could be long after the 1st error. > > > > We're talking about postcopy migrating a VM has poisoned page on src, > > rather than on dst host, am I right? IOW, the dest hwpoison should be > > fake. > > > > If so, then I would assume that's the case where all the pages on the dest > > host is still all good (so hw_memory_failure not yet set, or I doubt the > > judgement of being a migration target after all)? > > > > The other thing is even if dest host has hw poisoned page, I'm not sure > > whether hw_memory_failure is the only way to solve this. > > > > I saw that this is something got worked on before from Zhenwei, David used > > to have some reasoning on why it was suggested like using a global knob: > > > > https://lore.kernel.org/all/d7927214-e433-c26d-7a9c-a291ced81887@redhat.com/ > > > > Two major issues here afaics: > > > > - Zhenwei's approach only considered x86 hwpoison - it relies on kpte > > having !present in entries but that's x86 specific rather than generic > > to memory_failure.c. > > > > - It is _assumed_ that hwpoison injection is for debugging only. > > > > I'm not sure whether you can fix 1) by some other ways, e.g., what if the > > host just remember all the hardware poisoned pfns (or remember > > soft-poisoned ones, but then here we need to be careful on removing them > > from the list when it's hwpoisoned for real)? It sounds like there's > > opportunity on providing a generic solution rather than relying on > > !pte_present(). > > > > For 2) IMHO that's not a big issue, you can declare it'll be used in !debug > > but production systems so as to boost the feature importance with a real > > use case. > > > > So far I'd say it'll be great to leverage what it's already there in linux > > and make it as generic as possible. The only issue is probably > > CAP_ADMIN... not sure whether we can have some way to provide !ADMIN > > somehow, or you can simply work around this issue. > > As you mention below I think the key distinction is the scope - I > think MADV_HWPOISON affects the whole system, including other > processes. > > For our purposes, we really just want to "poison" this particular > virtual address (the HVA, from the VM's perspective), not even other > mappings of the same shared memory. I think that behavior is different > from MADV_HWPOISON, at least. > > > > > > > > > > > > > > > > > > > > > > > Besides what James mentioned on "missing addr", I didn't quickly see what's > > > > > > the major difference comparing to the old hwpoison injection methods even > > > > > > without the addr requirement. If we want the addr for MCE then it's more of > > > > > > a question to ask. > > > > > > > > > > > > I also didn't quickly see why for whatever new way to inject a pte error we > > > > > > need to have it registered with uffd. Could it be something like > > > > > > MADV_PGERR (even if MADV_HWPOISON won't suffice) so you can inject even > > > > > > without an userfault context (but still usable when uffd registered)? > > > > > > > > > > > > And it'll be alawys nice to have a cover letter too (if there'll be a new > > > > > > version) explaining the bits. > > > > > > > > I do plan a v2, if for no other reason than to update the > > > > documentation. Happy to add a cover letter with it as well. > > > > > > > > +Jiaqi back to CC, this is one piece of a larger memory poisoning / > > > > recovery design Jiaqi is working on, so he may have some ideas why > > > > MADV_HWPOISON or MADV_PGER will or won't work. > > > > > > Per https://man7.org/linux/man-pages/man2/madvise.2.html, > > > MADV_HWPOISON "is available only for privileged (CAP_SYS_ADMIN) > > > processes." So for a non-root VMM, MADV_HWPOISON is out of option. > > > > It makes sense to me especially when the page can be shared with other > > tasks. > > > > > > > > Another issue with MADV_HWPOISON is, it requires to first successfully > > > get_user_pages_fast(). I don't think it will work if memory is not > > > mapped yet. > > > > Fair point, so probably current MADV_HWPOISON got ruled out. > > hwpoison-inject seems fine where only the PFN is needed rather than the > > pte. But same issue on CAP_ADMIN indeed. > > > > > > > > With the UFFDIO_SIGBUS feature introduced in this patchset, it may > > > even be possible to free the emulated-hwpoison page back to the kernel > > > so we don't lose a 4K page. > > > > > > I didn't find any ref/doc for MADV_PGERR. Is it something you suggest > > > to build, Peter? > > > > That's something I made up just to show my question on why such an > > interface (even if wanted) needs to be bound to userfaultfd, e.g. a > > madvise() seems working if someone sololy want to install a poisoned pte. > > I look at it a bit differently... > > Even existing UFFDIO_* operations could technically be separated from > userfaultfd. You could imagine a MADV_MAP_PAGE instead of > UFFDIO_CONTINUE. UFFDIO_COPY is a bit trickier since it takes an > argument, but it could be done with process_madvise(). (Granted, I'm > not sure this would be useful... But this is equally true for > UFFDIO_SIGBUS; it seems non-live-migration use cases could use > MADV_HWPOISON, and for live migration use cases we will be using > UFFD.) > > We've sort of setup a convention with userfaultfd where at a high > level users are supposed to: > > 1. Receive events from the uffd > 2. Resolve those events with UFFDIO_* ioctls > 3. Wake up with UFFDIO_WAKE to retry the fault that generated the > original event (can be combined with step 2 of course) > > So for me, even if MADV_PGERR or similar existed, I would be tempted > to add a UFFDIO_SIGBUS as well, even if it just calls the same > underlying function to do the same thing, if only for consistency > (with the idea "UFFD events are resolved by UFFD ioctls") from the > user's perspective. I don't worry too much on "consistency", but I'm trying to understand whether it's more beneficial to combine it with uffd or being generic. One thing I was thinking is if I have a library that manages some memory for the user, the library can use such madvise()/... to poison specific small pages (without registering uffd with sigbus mode, also no lose on page faults of other normal pages) so when illegal access it can trap it for current mm rather than silently happen (e.g. use after free). Unpoison is also easy there, we can simply DONTNEED it. One defect of such general solution for your case is we need one more UFFDIO_WAKE, but since we're talking about real poisoned pages on src, so I guess it's not a concern (unlike most of the rest ioctls). I've no strong opinion if you still want to do that with an userfault ioctl. After all, I can't provide a solid example but just some rough ideas. But I hope I explained why I think it's still different from other ioctls (e.g., an "atomic update a page" operation doesn't sound reasonable at all as generic operation for any !uffd context, so that definitely suites more as an uffd specific ioctl). If with uffd, perhaps avoid calling it sigbus? As we have FEATURE_SIGBUS and I'm afraid it'll cause confusion. UFFDIO_HWPOISON may sound more suitable? Thanks,
On Thu, May 18, 2023 at 05:38:14PM -0400, Peter Xu wrote: > If with uffd, perhaps avoid calling it sigbus? As we have FEATURE_SIGBUS > and I'm afraid it'll cause confusion. UFFDIO_HWPOISON may sound more > suitable? Or UFFDIO_POISON (to identify it from real hw poisons)..
On 18.05.23 22:38, Axel Rasmussen wrote: > On Thu, May 18, 2023 at 9:05 AM Peter Xu <peterx@redhat.com> wrote: >> >> On Wed, May 17, 2023 at 05:43:53PM -0700, Jiaqi Yan wrote: >>> On Wed, May 17, 2023 at 3:29 PM Axel Rasmussen <axelrasmussen@google.com> wrote: >>>> >>>> On Wed, May 17, 2023 at 3:20 PM Peter Xu <peterx@redhat.com> wrote: >>>>> >>>>> On Wed, May 17, 2023 at 06:12:33PM -0400, Peter Xu wrote: >>>>>> On Thu, May 11, 2023 at 03:00:09PM -0700, James Houghton wrote: >>>>>>> On Thu, May 11, 2023 at 11:24 AM Axel Rasmussen >>>>>>> <axelrasmussen@google.com> wrote: >>>>>>>> >>>>>>>> So the basic way to use this new feature is: >>>>>>>> >>>>>>>> - On the new host, the guest's memory is registered with userfaultfd, in >>>>>>>> either MISSING or MINOR mode (doesn't really matter for this purpose). >>>>>>>> - On any first access, we get a userfaultfd event. At this point we can >>>>>>>> communicate with the old host to find out if the page was poisoned. >>>>>>>> - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker >>>>>>>> so any future accesses will SIGBUS. Because the pte is now "present", >>>>>>>> future accesses won't generate more userfaultfd events, they'll just >>>>>>>> SIGBUS directly. >>>>>>> >>>>>>> I want to clarify the SIGBUS mechanism here when KVM is involved, >>>>>>> keeping in mind that we need to be able to inject an MCE into the >>>>>>> guest for this to be useful. >>>>>>> >>>>>>> 1. vCPU gets an EPT violation --> KVM attempts GUP. >>>>>>> 2. GUP finds a PTE_MARKER_UFFD_SIGBUS and returns VM_FAULT_SIGBUS. >>>>>>> 3. KVM finds that GUP failed and returns -EFAULT. >>>>>>> >>>>>>> This is different than if GUP found poison, in which case KVM will >>>>>>> actually queue up a SIGBUS *containing the address of the fault*, and >>>>>>> userspace can use it to inject an appropriate MCE into the guest. With >>>>>>> UFFDIO_SIGBUS, we are missing the address! >>>>>>> >>>>>>> I see three options: >>>>>>> 1. Make KVM_RUN queue up a signal for any VM_FAULT_SIGBUS. I think >>>>>>> this is pointless. >>>>>>> 2. Don't have UFFDIO_SIGBUS install a PTE entry, but instead have a >>>>>>> UFFDIO_WAKE_MODE_SIGBUS, where upon waking, we return VM_FAULT_SIGBUS >>>>>>> instead of VM_FAULT_RETRY. We will keep getting userfaults on repeated >>>>>>> accesses, just like how we get repeated signals for real poison. >>>>>>> 3. Use this in conjunction with the additional KVM EFAULT info that >>>>>>> Anish proposed (the first part of [1]). >>>>>>> >>>>>>> I think option 3 is fine. :) >>>>>> >>>>>> Or... option 4) just to use either MADV_HWPOISON or hwpoison-inject? :) >>>>> >>>>> I just remember Axel mentioned this in the commit message, and just in case >>>>> this is why option 4) was ruled out: >>>>> >>>>> They expect that once poisoned, pages can never become >>>>> "un-poisoned". So, when we live migrate the VM, we need to preserve >>>>> the poisoned status of these pages. >>>>> >>>>> Just to supplement on this point: we do have unpoison (echoing to >>>>> "debug/hwpoison/hwpoison_unpoison"), or am I wrong? >>> >>> If I read unpoison_memory() correctly, once there is a real hardware >>> memory corruption (hw_memory_failure will be set), unpoison will stop >>> working and return EOPNOTSUPP. >>> >>> I know some cloud providers evacuating VMs once a single memory error >>> happens, so not supporting unpoison is probably not a big deal for >>> them. BUT others do keep VM running until more errors show up later, >>> which could be long after the 1st error. >> >> We're talking about postcopy migrating a VM has poisoned page on src, >> rather than on dst host, am I right? IOW, the dest hwpoison should be >> fake. >> >> If so, then I would assume that's the case where all the pages on the dest >> host is still all good (so hw_memory_failure not yet set, or I doubt the >> judgement of being a migration target after all)? >> >> The other thing is even if dest host has hw poisoned page, I'm not sure >> whether hw_memory_failure is the only way to solve this. >> >> I saw that this is something got worked on before from Zhenwei, David used >> to have some reasoning on why it was suggested like using a global knob: >> >> https://lore.kernel.org/all/d7927214-e433-c26d-7a9c-a291ced81887@redhat.com/ >> >> Two major issues here afaics: >> >> - Zhenwei's approach only considered x86 hwpoison - it relies on kpte >> having !present in entries but that's x86 specific rather than generic >> to memory_failure.c. >> >> - It is _assumed_ that hwpoison injection is for debugging only. >> >> I'm not sure whether you can fix 1) by some other ways, e.g., what if the >> host just remember all the hardware poisoned pfns (or remember >> soft-poisoned ones, but then here we need to be careful on removing them >> from the list when it's hwpoisoned for real)? It sounds like there's >> opportunity on providing a generic solution rather than relying on >> !pte_present(). >> >> For 2) IMHO that's not a big issue, you can declare it'll be used in !debug >> but production systems so as to boost the feature importance with a real >> use case. >> >> So far I'd say it'll be great to leverage what it's already there in linux >> and make it as generic as possible. The only issue is probably >> CAP_ADMIN... not sure whether we can have some way to provide !ADMIN >> somehow, or you can simply work around this issue. > > As you mention below I think the key distinction is the scope - I > think MADV_HWPOISON affects the whole system, including other > processes. > > For our purposes, we really just want to "poison" this particular > virtual address (the HVA, from the VM's perspective), not even other > mappings of the same shared memory. I think that behavior is different > from MADV_HWPOISON, at least. MADV_HWPOISON really is the wrong interface to use. See "man madvise". We don't want to allow arbitrary users to hwpoison+offline absolutely healthy physical memory, which is what MADV_HWPOISON is all about. As you say, we want to turn an unpopulated (!present) virtual address to mimic like we had a MCE on a page that would have been previously mapped here: install a hwpoison marker without actually poisoning any present page. In fact, we'd even want to fail if there *is* something mapped. Sure, one could teach MADV_HWPOISON to allow unprivileged users to do that for !present PTE entries, and fail for unprivileged users if there is a present PTE entry. I'm not sure if that's the cleanest approach, though, and a new MADV as suggested in this thread would eventually be cleaner.
Hi, Jiaqi, On Fri, May 19, 2023 at 08:04:09AM -0700, Jiaqi Yan wrote: > I don't think CAP_ADMIN is something we can work around: a VMM must be > a good citizen to avoid introducing any vulnerability to the host or > guest. > > On the other hand, "Userfaults allow the implementation of on-demand > paging from userland and more generally they allow userland to take > control of various memory page faults, something otherwise only the > kernel code could do." [3]. I am not familiar with the UFFD internals, > but our use case seems to match what UFFD wants to provide: without > affecting the whole world, give a specific userspace (without > CAP_ADMIN) the ability to handle page faults (indirectly emulate a > HWPOISON page (in my mind I treat it as SetHWPOISON(page) + > TestHWPOISON(page) operation in kernel's PF code)). So is it fair to > say what Axel provided here is "provide !ADMIN somehow"? > > [3]https://docs.kernel.org/admin-guide/mm/userfaultfd.html Userfault keywords on "user", IMHO. We don't strictly need userfault to resolve anything regarding CAP_ADMIN problems. MADV_DONTNEED also dosn't need CAP_ADMIN, same to any new madvise() if we want to make it useful for injecting poisoned ptes with !ADMIN and limit it within current->mm. But I think you're right that userfaultfd always tried to avoid having ADMIN and keep everything within its own scope of permissions. So again, totally no objection on make it uffd specific for now if you guys are all happy with it, but just to be clear that it's (to me) mostly for avoiding another WAKE, and afaics that's not really for solving the ADMIN issue here. Thanks,
On Tue, May 23, 2023 at 10:59:05AM -0700, Axel Rasmussen wrote: > > Actually.. I think maybe we should have 1 patch changing SWAPIN_ERROR from > > VM_FAULT_SIGBUS to VM_FAULT_HWPOISON. > > > > Let's imagine a VM having anonymous page backing and got a swapin error > > when faulted on one of the guest page. Instead of crashing the hypervisor > > with sigbus we should probably make it a MCE injected into the guest too, > > because there's no page corrupt in bare metal in this specific case, > > however to the guest it's the same as having one page corrupted just like a > > real MCE. > > This is a great idea, you're right that injecting an MCE into the > guest is exactly the end goal, and it seems like VM_FAULT_HWPOISON > will "just work". Also the name UFFDIO_POISON resolves any confusion > with UFFD_FEATURE_SIGBUS, so that's a nice side benefit. Hopefully! Please double check it with KVM running altogether to make sure the patch works exactly as expected. [...] > We'll want hugetlbfs support for this operation too, but it's only > really useful (at least for our use case) after HGM is merged. But, > there's no strong reason not to just implement both all at once - I'll > extend v2 to also work properly with hugetlbfs. Probably it isn't too > hard, I just need to do a bit more reading of how swap markers are > handled in hugetlbfs. Sure. We have too many flags separating different types of memory, so I think it'll be nice if when it can still trivially work for everything. For this specific case, since your goal is definitely having hugetlb hgm supported so it'll be even more trickier if only hgm will be supported but not !hgm hugetlbs, so we'd better target it initially with all mem types. Thanks,
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 0fd96d6e39ce..edc2928dae2b 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1966,6 +1966,66 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg) return ret; } +static inline int userfaultfd_sigbus(struct userfaultfd_ctx *ctx, unsigned long arg) +{ + __s64 ret; + struct uffdio_sigbus uffdio_sigbus; + struct uffdio_sigbus __user *user_uffdio_sigbus; + struct userfaultfd_wake_range range; + + user_uffdio_sigbus = (struct uffdio_sigbus __user *)arg; + + ret = -EAGAIN; + if (atomic_read(&ctx->mmap_changing)) + goto out; + + ret = -EFAULT; + if (copy_from_user(&uffdio_sigbus, user_uffdio_sigbus, + /* don't copy the output fields */ + sizeof(uffdio_sigbus) - (sizeof(__s64)))) + goto out; + + ret = validate_range(ctx->mm, uffdio_sigbus.range.start, + uffdio_sigbus.range.len); + if (ret) + goto out; + + ret = -EINVAL; + /* double check for wraparound just in case. */ + if (uffdio_sigbus.range.start + uffdio_sigbus.range.len <= + uffdio_sigbus.range.start) { + goto out; + } + if (uffdio_sigbus.mode & ~UFFDIO_SIGBUS_MODE_DONTWAKE) + goto out; + + if (mmget_not_zero(ctx->mm)) { + ret = mfill_atomic_sigbus(ctx->mm, uffdio_sigbus.range.start, + uffdio_sigbus.range.len, + &ctx->mmap_changing, 0); + mmput(ctx->mm); + } else { + return -ESRCH; + } + + if (unlikely(put_user(ret, &user_uffdio_sigbus->updated))) + return -EFAULT; + if (ret < 0) + goto out; + + /* len == 0 would wake all */ + BUG_ON(!ret); + range.len = ret; + if (!(uffdio_sigbus.mode & UFFDIO_SIGBUS_MODE_DONTWAKE)) { + range.start = uffdio_sigbus.range.start; + wake_userfault(ctx, &range); + } + ret = range.len == uffdio_sigbus.range.len ? 0 : -EAGAIN; + +out: + return ret; +} + static inline unsigned int uffd_ctx_features(__u64 user_features) { /* @@ -2067,6 +2127,9 @@ static long userfaultfd_ioctl(struct file *file, unsigned cmd, case UFFDIO_CONTINUE: ret = userfaultfd_continue(ctx, arg); break; + case UFFDIO_SIGBUS: + ret = userfaultfd_sigbus(ctx, arg); + break; } return ret; } diff --git a/include/linux/swapops.h b/include/linux/swapops.h index 3a451b7afcb3..fa778a0ae730 100644 --- a/include/linux/swapops.h +++ b/include/linux/swapops.h @@ -405,7 +405,8 @@ typedef unsigned long pte_marker; #define PTE_MARKER_UFFD_WP BIT(0) #define PTE_MARKER_SWAPIN_ERROR BIT(1) -#define PTE_MARKER_MASK (BIT(2) - 1) +#define PTE_MARKER_UFFD_SIGBUS BIT(2) +#define PTE_MARKER_MASK (BIT(3) - 1) static inline swp_entry_t make_pte_marker_entry(pte_marker marker) { diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h index d78b01524349..6de1084939c5 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -46,6 +46,7 @@ enum mfill_atomic_mode { MFILL_ATOMIC_COPY, MFILL_ATOMIC_ZEROPAGE, MFILL_ATOMIC_CONTINUE, + MFILL_ATOMIC_SIGBUS, NR_MFILL_ATOMIC_MODES, }; @@ -83,6 +84,9 @@ extern ssize_t mfill_atomic_zeropage(struct mm_struct *dst_mm, extern ssize_t mfill_atomic_continue(struct mm_struct *dst_mm, unsigned long dst_start, unsigned long len, atomic_t *mmap_changing, uffd_flags_t flags); +extern ssize_t mfill_atomic_sigbus(struct mm_struct *dst_mm, unsigned long start, + unsigned long len, atomic_t *mmap_changing, + uffd_flags_t flags); extern int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, unsigned long len, bool enable_wp, atomic_t *mmap_changing); diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 66dd4cd277bd..616e33d3db97 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -39,7 +39,8 @@ UFFD_FEATURE_MINOR_SHMEM | \ UFFD_FEATURE_EXACT_ADDRESS | \ UFFD_FEATURE_WP_HUGETLBFS_SHMEM | \ - UFFD_FEATURE_WP_UNPOPULATED) + UFFD_FEATURE_WP_UNPOPULATED | \ + UFFD_FEATURE_SIGBUS_IOCTL) #define UFFD_API_IOCTLS \ ((__u64)1 << _UFFDIO_REGISTER | \ (__u64)1 << _UFFDIO_UNREGISTER | \ @@ -49,12 +50,14 @@ (__u64)1 << _UFFDIO_COPY | \ (__u64)1 << _UFFDIO_ZEROPAGE | \ (__u64)1 << _UFFDIO_WRITEPROTECT | \ - (__u64)1 << _UFFDIO_CONTINUE) + (__u64)1 << _UFFDIO_CONTINUE | \ + (__u64)1 << _UFFDIO_SIGBUS) #define UFFD_API_RANGE_IOCTLS_BASIC \ ((__u64)1 << _UFFDIO_WAKE | \ (__u64)1 << _UFFDIO_COPY | \ + (__u64)1 << _UFFDIO_WRITEPROTECT | \ (__u64)1 << _UFFDIO_CONTINUE | \ - (__u64)1 << _UFFDIO_WRITEPROTECT) + (__u64)1 << _UFFDIO_SIGBUS) /* * Valid ioctl command number range with this API is from 0x00 to @@ -71,6 +74,7 @@ #define _UFFDIO_ZEROPAGE (0x04) #define _UFFDIO_WRITEPROTECT (0x06) #define _UFFDIO_CONTINUE (0x07) +#define _UFFDIO_SIGBUS (0x08) #define _UFFDIO_API (0x3F) /* userfaultfd ioctl ids */ @@ -91,6 +95,8 @@ struct uffdio_writeprotect) #define UFFDIO_CONTINUE _IOWR(UFFDIO, _UFFDIO_CONTINUE, \ struct uffdio_continue) +#define UFFDIO_SIGBUS _IOWR(UFFDIO, _UFFDIO_SIGBUS, \ + struct uffdio_sigbus) /* read() structure */ struct uffd_msg { @@ -225,6 +231,7 @@ struct uffdio_api { #define UFFD_FEATURE_EXACT_ADDRESS (1<<11) #define UFFD_FEATURE_WP_HUGETLBFS_SHMEM (1<<12) #define UFFD_FEATURE_WP_UNPOPULATED (1<<13) +#define UFFD_FEATURE_SIGBUS_IOCTL (1<<14) __u64 features; __u64 ioctls; @@ -321,6 +328,18 @@ struct uffdio_continue { __s64 mapped; }; +struct uffdio_sigbus { + struct uffdio_range range; +#define UFFDIO_SIGBUS_MODE_DONTWAKE ((__u64)1<<0) + __u64 mode; + + /* + * Fields below here are written by the ioctl and must be at the end: + * the copy_from_user will not read past here. + */ + __s64 updated; +}; + /* * Flags for the userfaultfd(2) system call itself. */ diff --git a/mm/memory.c b/mm/memory.c index f69fbc251198..e4b4207c2590 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3675,6 +3675,10 @@ static vm_fault_t handle_pte_marker(struct vm_fault *vmf) if (WARN_ON_ONCE(!marker)) return VM_FAULT_SIGBUS; + /* SIGBUS explicitly requested for this PTE. */ + if (marker & PTE_MARKER_UFFD_SIGBUS) + return VM_FAULT_SIGBUS; + /* Higher priority than uffd-wp when data corrupted */ if (marker & PTE_MARKER_SWAPIN_ERROR) return VM_FAULT_SIGBUS; diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c index e97a0b4889fc..933587eebd5d 100644 --- a/mm/userfaultfd.c +++ b/mm/userfaultfd.c @@ -278,6 +278,51 @@ static int mfill_atomic_pte_continue(pmd_t *dst_pmd, goto out; } +/* Handles UFFDIO_SIGBUS for all non-hugetlb VMAs. */ +static int mfill_atomic_pte_sigbus(pmd_t *dst_pmd, + struct vm_area_struct *dst_vma, + unsigned long dst_addr, + uffd_flags_t flags) +{ + int ret; + struct mm_struct *dst_mm = dst_vma->vm_mm; + pte_t _dst_pte, *dst_pte; + spinlock_t *ptl; + + _dst_pte = make_pte_marker(PTE_MARKER_UFFD_SIGBUS); + dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl); + + if (vma_is_shmem(dst_vma)) { + struct inode *inode; + pgoff_t offset, max_off; + + /* serialize against truncate with the page table lock */ + inode = dst_vma->vm_file->f_inode; + offset = linear_page_index(dst_vma, dst_addr); + max_off = DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE); + ret = -EFAULT; + if (unlikely(offset >= max_off)) + goto out_unlock; + } + + ret = -EEXIST; + /* + * For now, we don't handle unmapping pages, so only support filling in + * none PTEs, or replacing PTE markers. + */ + if (!pte_none_mostly(*dst_pte)) + goto out_unlock; + + set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte); + + /* No need to invalidate - it was non-present before */ + update_mmu_cache(dst_vma, dst_addr, dst_pte); + ret = 0; +out_unlock: + pte_unmap_unlock(dst_pte, ptl); + return ret; +} + static pmd_t *mm_alloc_pmd(struct mm_struct *mm, unsigned long address) { pgd_t *pgd; @@ -328,8 +373,12 @@ static __always_inline ssize_t mfill_atomic_hugetlb( * supported by hugetlb. A PMD_SIZE huge pages may exist as used * by THP. Since we can not reliably insert a zero page, this * feature is not supported. + * + * PTE marker handling for hugetlb is a bit special, so for now + * UFFDIO_SIGBUS is not supported. */ - if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE)) { + if (uffd_flags_mode_is(flags, MFILL_ATOMIC_ZEROPAGE) || + uffd_flags_mode_is(flags, MFILL_ATOMIC_SIGBUS)) { mmap_read_unlock(dst_mm); return -EINVAL; } @@ -473,6 +522,9 @@ static __always_inline ssize_t mfill_atomic_pte(pmd_t *dst_pmd, if (uffd_flags_mode_is(flags, MFILL_ATOMIC_CONTINUE)) { return mfill_atomic_pte_continue(dst_pmd, dst_vma, dst_addr, flags); + } else if (uffd_flags_mode_is(flags, MFILL_ATOMIC_SIGBUS)) { + return mfill_atomic_pte_sigbus(dst_pmd, dst_vma, + dst_addr, flags); } /* @@ -694,6 +746,14 @@ ssize_t mfill_atomic_continue(struct mm_struct *dst_mm, unsigned long start, uffd_flags_set_mode(flags, MFILL_ATOMIC_CONTINUE)); } +ssize_t mfill_atomic_sigbus(struct mm_struct *dst_mm, unsigned long start, + unsigned long len, atomic_t *mmap_changing, + uffd_flags_t flags) +{ + return mfill_atomic(dst_mm, start, 0, len, mmap_changing, + uffd_flags_set_mode(flags, MFILL_ATOMIC_SIGBUS)); +} + long uffd_wp_range(struct vm_area_struct *dst_vma, unsigned long start, unsigned long len, bool enable_wp) {
The basic idea here is to "simulate" memory poisoning for VMs. A VM running on some host might encounter a memory error, after which some page(s) are poisoned (i.e., future accesses SIGBUS). They expect that once poisoned, pages can never become "un-poisoned". So, when we live migrate the VM, we need to preserve the poisoned status of these pages. When live migrating, we try to get the guest running on its new host as quickly as possible. So, we start it running before all memory has been copied, and before we're certain which pages should be poisoned or not. So the basic way to use this new feature is: - On the new host, the guest's memory is registered with userfaultfd, in either MISSING or MINOR mode (doesn't really matter for this purpose). - On any first access, we get a userfaultfd event. At this point we can communicate with the old host to find out if the page was poisoned. - If so, we can respond with a UFFDIO_SIGBUS - this places a swap marker so any future accesses will SIGBUS. Because the pte is now "present", future accesses won't generate more userfaultfd events, they'll just SIGBUS directly. UFFDIO_SIGBUS does not handle unmapping previously-present PTEs. This isn't needed, because during live migration we want to intercept all accesses with userfaultfd (not just writes, so WP mode isn't useful for this). So whether minor or missing mode is being used (or both), the PTE won't be present in any case, so handling that case isn't needed. Signed-off-by: Axel Rasmussen <axelrasmussen@google.com> --- fs/userfaultfd.c | 63 ++++++++++++++++++++++++++++++++ include/linux/swapops.h | 3 +- include/linux/userfaultfd_k.h | 4 ++ include/uapi/linux/userfaultfd.h | 25 +++++++++++-- mm/memory.c | 4 ++ mm/userfaultfd.c | 62 ++++++++++++++++++++++++++++++- 6 files changed, 156 insertions(+), 5 deletions(-)