Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Thu, Mar 11, 2021 at 04:52:10PM +, Luck, Tony wrote: > > I think we need to at least fix the existing bug before we add more > > signals. AFAICS the MCE_IN_KERNEL_COPYIN code is busted for kernel > > threads. > > Can a kernel thread do get_user() or copy_from_user()? Do we have kernel > threads > that have an associated user address space to copy from? kthread_use_mm()
RE: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> I think we need to at least fix the existing bug before we add more > signals. AFAICS the MCE_IN_KERNEL_COPYIN code is busted for kernel > threads. Can a kernel thread do get_user() or copy_from_user()? Do we have kernel threads that have an associated user address space to copy from? -Tony
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Wed, 10 Mar 2021 17:28:12 -0800
Andy Lutomirski wrote:
> On Wed, Mar 10, 2021 at 5:19 PM Aili Yao wrote:
> >
> > On Mon, 8 Mar 2021 11:00:28 -0800
> > Andy Lutomirski wrote:
> >
> > > > On Mar 8, 2021, at 10:31 AM, Luck, Tony wrote:
> > > >
> > > >
> > > >>
> > > >> Can you point me at that SIGBUS code in a current kernel?
> > > >
> > > > It is in kill_me_maybe(). mce_vaddr is setup when we disassemble
> > > > whatever get_user()
> > > > or copy from user variant was in use in the kernel when the poison
> > > > memory was consumed.
> > > >
> > > >if (p->mce_vaddr != (void __user *)-1l) {
> > > >force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr,
> > > > PAGE_SHIFT);
> > >
> > > Hmm. On the one hand, no one has complained yet. On the other hand,
> > > hardware that supports this isn’t exactly common.
> > >
> > > We may need some actual ABI design here. We also need to make sure that
> > > things like io_uring accesses or, more generally, anything using the
> > > use_mm / use_temporary_mm ends up either sending no signal or sending a
> > > signal to the right target.
> > >
> > > >
> > > > Would it be any better if we used the BUS_MCEERR_AO code that goes into
> > > > siginfo?
> > >
> > > Dunno.
> >
> > I have one thought here but don't know if it's proper:
> >
> > Previous patch use force_sig_mceerr to the user process for such a
> > scenario; with this method
> > The SIGBUS can't be ignored as force_sig_mceerr() was designed to.
> >
> > If the user process don't want this signal, will it set signal config to
> > ignore?
> > Maybe we can use a send_sig_mceerr() instead of force_sig_mceerr(), if
> > process want to
> > ignore the SIGBUS, then it will ignore that, or it can also process the
> > SIGBUS?
>
> I don't think the signal blocking mechanism makes sense for this.
> Blocking a signal is for saying that, if another process sends the
> signal (or an async event like ctrl-C), then the process doesn't want
> it. Blocking doesn't block synchronous things like faults.
>
> I think we need to at least fix the existing bug before we add more
> signals. AFAICS the MCE_IN_KERNEL_COPYIN code is busted for kernel
> threads.
Got this, Thanks!
I read https://man7.org/linux/man-pages/man2/write.2.html, and it seems the
write syscall is not expecting
an signal, maybe a specific error code for this scenario is enough.
--
Thanks!
Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Wed, Mar 10, 2021 at 5:19 PM Aili Yao wrote:
>
> On Mon, 8 Mar 2021 11:00:28 -0800
> Andy Lutomirski wrote:
>
> > > On Mar 8, 2021, at 10:31 AM, Luck, Tony wrote:
> > >
> > >
> > >>
> > >> Can you point me at that SIGBUS code in a current kernel?
> > >
> > > It is in kill_me_maybe(). mce_vaddr is setup when we disassemble
> > > whatever get_user()
> > > or copy from user variant was in use in the kernel when the poison memory
> > > was consumed.
> > >
> > >if (p->mce_vaddr != (void __user *)-1l) {
> > >force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
> >
> > Hmm. On the one hand, no one has complained yet. On the other hand,
> > hardware that supports this isn’t exactly common.
> >
> > We may need some actual ABI design here. We also need to make sure that
> > things like io_uring accesses or, more generally, anything using the use_mm
> > / use_temporary_mm ends up either sending no signal or sending a signal to
> > the right target.
> >
> > >
> > > Would it be any better if we used the BUS_MCEERR_AO code that goes into
> > > siginfo?
> >
> > Dunno.
>
> I have one thought here but don't know if it's proper:
>
> Previous patch use force_sig_mceerr to the user process for such a scenario;
> with this method
> The SIGBUS can't be ignored as force_sig_mceerr() was designed to.
>
> If the user process don't want this signal, will it set signal config to
> ignore?
> Maybe we can use a send_sig_mceerr() instead of force_sig_mceerr(), if
> process want to
> ignore the SIGBUS, then it will ignore that, or it can also process the
> SIGBUS?
I don't think the signal blocking mechanism makes sense for this.
Blocking a signal is for saying that, if another process sends the
signal (or an async event like ctrl-C), then the process doesn't want
it. Blocking doesn't block synchronous things like faults.
I think we need to at least fix the existing bug before we add more
signals. AFAICS the MCE_IN_KERNEL_COPYIN code is busted for kernel
threads.
--Andy
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Mon, 8 Mar 2021 11:00:28 -0800
Andy Lutomirski wrote:
> > On Mar 8, 2021, at 10:31 AM, Luck, Tony wrote:
> >
> >
> >>
> >> Can you point me at that SIGBUS code in a current kernel?
> >
> > It is in kill_me_maybe(). mce_vaddr is setup when we disassemble whatever
> > get_user()
> > or copy from user variant was in use in the kernel when the poison memory
> > was consumed.
> >
> >if (p->mce_vaddr != (void __user *)-1l) {
> >force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
>
> Hmm. On the one hand, no one has complained yet. On the other hand, hardware
> that supports this isn’t exactly common.
>
> We may need some actual ABI design here. We also need to make sure that
> things like io_uring accesses or, more generally, anything using the use_mm /
> use_temporary_mm ends up either sending no signal or sending a signal to the
> right target.
>
> >
> > Would it be any better if we used the BUS_MCEERR_AO code that goes into
> > siginfo?
>
> Dunno.
I have one thought here but don't know if it's proper:
Previous patch use force_sig_mceerr to the user process for such a scenario;
with this method
The SIGBUS can't be ignored as force_sig_mceerr() was designed to.
If the user process don't want this signal, will it set signal config to ignore?
Maybe we can use a send_sig_mceerr() instead of force_sig_mceerr(), if process
want to
ignore the SIGBUS, then it will ignore that, or it can also process the SIGBUS?
--
Thanks!
Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Tue, 9 Mar 2021 10:14:52 +0800
Aili Yao wrote:
> On Mon, 8 Mar 2021 18:31:07 +
> "Luck, Tony" wrote:
>
> > > Can you point me at that SIGBUS code in a current kernel?
> >
> > It is in kill_me_maybe(). mce_vaddr is setup when we disassemble whatever
> > get_user()
> > or copy from user variant was in use in the kernel when the poison memory
> > was consumed.
> >
> > if (p->mce_vaddr != (void __user *)-1l) {
> > force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
> >
> > Would it be any better if we used the BUS_MCEERR_AO code that goes into
> > siginfo?
> >
> > That would make it match up better with what happens when poison is found
> > asynchronously by the patrol scrubber. I.e. the semantics are:
> >
> > AR: You just touched poison at this address and need to do something about
> > that.
> > AO: Just letting you know that you have some poison at the address in
> > siginfo.
> >
> > -Tony
>
> Is the kill action for this scenario in memory_failure()?
Does the current logic kill the process twice for this scenario ?
I am confused.
--
Thanks!
Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Mon, 8 Mar 2021 18:31:07 +
"Luck, Tony" wrote:
> > Can you point me at that SIGBUS code in a current kernel?
>
> It is in kill_me_maybe(). mce_vaddr is setup when we disassemble whatever
> get_user()
> or copy from user variant was in use in the kernel when the poison memory was
> consumed.
>
> if (p->mce_vaddr != (void __user *)-1l) {
> force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
>
> Would it be any better if we used the BUS_MCEERR_AO code that goes into
> siginfo?
>
> That would make it match up better with what happens when poison is found
> asynchronously by the patrol scrubber. I.e. the semantics are:
>
> AR: You just touched poison at this address and need to do something about
> that.
> AO: Just letting you know that you have some poison at the address in siginfo.
>
> -Tony
Is the kill action for this scenario in memory_failure()?
--
Thanks!
Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> On Mar 8, 2021, at 10:31 AM, Luck, Tony wrote:
>
>
>>
>> Can you point me at that SIGBUS code in a current kernel?
>
> It is in kill_me_maybe(). mce_vaddr is setup when we disassemble whatever
> get_user()
> or copy from user variant was in use in the kernel when the poison memory was
> consumed.
>
>if (p->mce_vaddr != (void __user *)-1l) {
>force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
Hmm. On the one hand, no one has complained yet. On the other hand, hardware
that supports this isn’t exactly common.
We may need some actual ABI design here. We also need to make sure that things
like io_uring accesses or, more generally, anything using the use_mm /
use_temporary_mm ends up either sending no signal or sending a signal to the
right target.
>
> Would it be any better if we used the BUS_MCEERR_AO code that goes into
> siginfo?
Dunno.
>
> That would make it match up better with what happens when poison is found
> asynchronously by the patrol scrubber. I.e. the semantics are:
>
> AR: You just touched poison at this address and need to do something about
> that.
> AO: Just letting you know that you have some poison at the address in siginfo.
>
> -Tony
RE: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> Can you point me at that SIGBUS code in a current kernel?
It is in kill_me_maybe(). mce_vaddr is setup when we disassemble whatever
get_user()
or copy from user variant was in use in the kernel when the poison memory was
consumed.
if (p->mce_vaddr != (void __user *)-1l) {
force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
Would it be any better if we used the BUS_MCEERR_AO code that goes into siginfo?
That would make it match up better with what happens when poison is found
asynchronously by the patrol scrubber. I.e. the semantics are:
AR: You just touched poison at this address and need to do something about that.
AO: Just letting you know that you have some poison at the address in siginfo.
-Tony
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> On Mar 8, 2021, at 1:49 AM, Aili Yao wrote: > > On Sun, 7 Mar 2021 11:16:24 -0800 > Andy Lutomirski wrote: > >>> Some programs may use read(2), write(2), etc as ways to check if >>> memory is valid without getting a signal. They might not want >>> signals, which means that this feature might need to be configurable. >> >> That sounds like an appalling hack. If users need such a mechanism >> we should create some better way to do that. >> > > Appalling hack or not, it works. So, if we’re going to send a signal to > user code that looks like it originated from a bina fide architectural > recoverable fault, it needs to be recoverable. A load from a failed > NVDIMM page is such a fault. A *kernel* load is not. So we need to > distinguish it somehow. Sorry for my previous mis-understanding, and i have some questions: if programs use read,write to check if if memory is valid, does it really want to cover the poison case? >> >> I don't know. >> When for such a case, an error is returned, can the program realize it's hwposion issue not other software error and process correctly? >> >> Again, I don't know. But changing the API like this seems potentialy >> dangerous and needs to be done with care. >> if this is the proper action, the original posion flow in current code from read and write need to change too. >>> >>> Sorry, another question: >>> When programs use read(2), write(2) as ways to check if memory is valid, >>> does it really want to check if the user page the program provided is >>> valid, not the destination or disk space valid? >> >> They may well be trying to see if their memory is valid. > > Thanks for your reply, and I don't know what to do. > For current code, if user program write to a block device(maybe a test try) > and if its user copy page corrupt when in kernel copy, the process is killed > with a SIGBUS. > And for the page fault case in this thread, the process is error returned. Can you point me at that SIGBUS code in a current kernel? > > -- > Thanks! > Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Sun, 7 Mar 2021 11:16:24 -0800 Andy Lutomirski wrote: > > > > >> Some programs may use read(2), write(2), etc as ways to check if > > > > >> memory is valid without getting a signal. They might not want > > > > >> signals, which means that this feature might need to be > > > > >> configurable. > > > > > > > > > > That sounds like an appalling hack. If users need such a mechanism > > > > > we should create some better way to do that. > > > > > > > > > > > > > Appalling hack or not, it works. So, if we’re going to send a signal to > > > > user code that looks like it originated from a bina fide architectural > > > > recoverable fault, it needs to be recoverable. A load from a failed > > > > NVDIMM page is such a fault. A *kernel* load is not. So we need to > > > > distinguish it somehow. > > > > > > Sorry for my previous mis-understanding, and i have some questions: > > > if programs use read,write to check if if memory is valid, does it really > > > want to cover the poison case? > > I don't know. > > > > When for such a case, an error is returned, can the program realize it's > > > hwposion issue not other software error and process correctly? > > Again, I don't know. But changing the API like this seems potentialy > dangerous and needs to be done with care. > > > > > > > if this is the proper action, the original posion flow in current code > > > from read and write need to change too. > > > > > > > Sorry, another question: > > When programs use read(2), write(2) as ways to check if memory is valid, > > does it really want to check if the user page the program provided is > > valid, not the destination or disk space valid? > > They may well be trying to see if their memory is valid. Thanks for your reply, and I don't know what to do. For current code, if user program write to a block device(maybe a test try) and if its user copy page corrupt when in kernel copy, the process is killed with a SIGBUS. And for the page fault case in this thread, the process is error returned. -- Thanks! Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Wed, Mar 3, 2021 at 4:51 AM Aili Yao wrote: > > On Wed, 3 Mar 2021 20:24:02 +0800 > Aili Yao wrote: > > > On Mon, 1 Mar 2021 11:09:36 -0800 > > Andy Lutomirski wrote: > > > > > > On Mar 1, 2021, at 11:02 AM, Luck, Tony wrote: > > > > > > > > > > > >> > > > >> Some programs may use read(2), write(2), etc as ways to check if > > > >> memory is valid without getting a signal. They might not want > > > >> signals, which means that this feature might need to be configurable. > > > > > > > > That sounds like an appalling hack. If users need such a mechanism > > > > we should create some better way to do that. > > > > > > > > > > Appalling hack or not, it works. So, if we’re going to send a signal to > > > user code that looks like it originated from a bina fide architectural > > > recoverable fault, it needs to be recoverable. A load from a failed > > > NVDIMM page is such a fault. A *kernel* load is not. So we need to > > > distinguish it somehow. > > > > Sorry for my previous mis-understanding, and i have some questions: > > if programs use read,write to check if if memory is valid, does it really > > want to cover the poison case? I don't know. > > When for such a case, an error is returned, can the program realize it's > > hwposion issue not other software error and process correctly? Again, I don't know. But changing the API like this seems potentialy dangerous and needs to be done with care. > > > > if this is the proper action, the original posion flow in current code from > > read and write need to change too. > > > > Sorry, another question: > When programs use read(2), write(2) as ways to check if memory is valid, > does it really want to check if the user page the program provided is valid, > not the destination or disk space valid? They may well be trying to see if their memory is valid. > the patch will not affect this purpose as it's only valid for user page > which program provide to write or some syscall similiar parameter > > -- > Thanks! > Aili Yao -- Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Mon, 1 Mar 2021 11:09:36 -0800 Andy Lutomirski wrote: > > On Mar 1, 2021, at 11:02 AM, Luck, Tony wrote: > > > > > >> > >> Some programs may use read(2), write(2), etc as ways to check if > >> memory is valid without getting a signal. They might not want > >> signals, which means that this feature might need to be configurable. > > > > That sounds like an appalling hack. If users need such a mechanism > > we should create some better way to do that. > > > > Appalling hack or not, it works. So, if we’re going to send a signal to user > code that looks like it originated from a bina fide architectural recoverable > fault, it needs to be recoverable. A load from a failed NVDIMM page is such > a fault. A *kernel* load is not. So we need to distinguish it somehow. Sorry for my previous mis-understanding, and i have some questions: if programs use read,write to check if if memory is valid, does it really want to cover the poison case? When for such a case, an error is returned, can the program realize it's hwposion issue not other software error and process correctly? if this is the proper action, the original posion flow in current code from read and write need to change too. -- Thanks! Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Wed, 3 Mar 2021 20:24:02 +0800 Aili Yao wrote: > On Mon, 1 Mar 2021 11:09:36 -0800 > Andy Lutomirski wrote: > > > > On Mar 1, 2021, at 11:02 AM, Luck, Tony wrote: > > > > > > > > >> > > >> Some programs may use read(2), write(2), etc as ways to check if > > >> memory is valid without getting a signal. They might not want > > >> signals, which means that this feature might need to be configurable. > > > > > > That sounds like an appalling hack. If users need such a mechanism > > > we should create some better way to do that. > > > > > > > Appalling hack or not, it works. So, if we’re going to send a signal to > > user code that looks like it originated from a bina fide architectural > > recoverable fault, it needs to be recoverable. A load from a failed NVDIMM > > page is such a fault. A *kernel* load is not. So we need to distinguish it > > somehow. > > Sorry for my previous mis-understanding, and i have some questions: > if programs use read,write to check if if memory is valid, does it really > want to cover the poison case? > When for such a case, an error is returned, can the program realize it's > hwposion issue not other software error and process correctly? > > if this is the proper action, the original posion flow in current code from > read and write need to change too. > Sorry, another question: When programs use read(2), write(2) as ways to check if memory is valid, does it really want to check if the user page the program provided is valid, not the destination or disk space valid? the patch will not affect this purpose as it's only valid for user page which program provide to write or some syscall similiar parameter. -- Thanks! Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> On Mar 1, 2021, at 11:02 AM, Luck, Tony wrote: > > >> >> Some programs may use read(2), write(2), etc as ways to check if >> memory is valid without getting a signal. They might not want >> signals, which means that this feature might need to be configurable. > > That sounds like an appalling hack. If users need such a mechanism > we should create some better way to do that. > Appalling hack or not, it works. So, if we’re going to send a signal to user code that looks like it originated from a bina fide architectural recoverable fault, it needs to be recoverable. A load from a failed NVDIMM page is such a fault. A *kernel* load is not. So we need to distinguish it somehow. > An aeon ago ACPI created the RASF table as a way for the OS to > ask the platform to scan a block of physical memory using the patrol > scrubber in the memory controller. I never did anything with it in Linux > because it was just too complex and didn't know of any use cases. > > Users would want to check virtual addresses. Perhaps some new > option MADV_CHECKFORPOISON to madvise(2) ? > > -Tony
RE: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> Some programs may use read(2), write(2), etc as ways to check if > memory is valid without getting a signal. They might not want > signals, which means that this feature might need to be configurable. That sounds like an appalling hack. If users need such a mechanism we should create some better way to do that. An aeon ago ACPI created the RASF table as a way for the OS to ask the platform to scan a block of physical memory using the patrol scrubber in the memory controller. I never did anything with it in Linux because it was just too complex and didn't know of any use cases. Users would want to check virtual addresses. Perhaps some new option MADV_CHECKFORPOISON to madvise(2) ? -Tony
RE: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> Programs that get a signal might expect that the RIP that the signal > frame points to is the instruction that caused the signal and that the > instruction faulted without side effects. For SIGSEGV, I would be > especially nervous about this. Maybe SIGBUS is safer. For SIGSEGV, > it's entirely valid to look at CR2 / si_fault_addr, fix it up, and > return. This would be completely *invalid* with your patch. I'm not > sure what to do about this. The original plan was that s/w like databases would be able to write their own application specific recovery code. E.g. they hit poison while reading some "table". The process gets a SIGBUS with siginfo telling the handler the virtual address range that has been lost. The code uses mmap(MAP_FIXED) to map a new page into the lost address and fills it with suitable data (either reconstructing lost data by replaying transactions, or filling the table with some "data unknown" indicator). Then the SIGBUS handler returns to re-execute the instruction that failed. As far as I know nobody has been that creative in production s/w. But I think there are folks with a siglongjmp() to a "this whole transaction just failed" safe point. -Tony
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
Hi Luto: > > > > At the very least, this needs a clear explanation of why your proposed > > > > behavior is better than the existing behavior. > > > > > > The explanation is buried in that "can't trust the process" line. > > > > > > E.g. user space isn't good about checking for failed write(2) syscalls. > > > So if the poison was in a user buffer passed to write(fd, buffer, count) > > > sending a SIGBUS would be the action if they read the poison directly, > > > so it seems reasonable to send the same signal if the kernel read their > > > poison for them. > > > > > > It would avoid users that didn't check the return value merrily proceeding > > > as if everything was ok. > > > > Hi luto: > >I will add more infomation: > >Even if the process will check return value of syscall like write, I > > don't think > > process will take proper action for this. > >In test example, the return value will be errno is 14 (Bad Address), the > > process may not realize > > this is a hw issue, and may take wrong action not as expected. > >And totally, A hw error will rarely happen, and the hw error hitting > > this branch will be > > more unlikely, the impaction without this patch is quite minor, but this is > > still not good enough, we should > > make it better, right? > > There are a few issues I can imagine: > > Some programs may use read(2), write(2), etc as ways to check if > memory is valid without getting a signal. They might not want > signals, which means that this feature might need to be configurable. I checked the code again and found that: For poison page access, the process may not ignore the SIGBUS signal even if it was set to 1298 /* 1299 * Force a signal that the process can't ignore: if necessary 1300 * we unblock the signal and change any SIG_IGN to SIG_DFL. 1301 * 1302 * Note: If we unblock the signal, we always reset it to SIG_DFL, 1303 * since we do not want to have a signal handler that was blocked 1304 * be invoked when user space had explicitly blocked it. 1305 * 1306 * We don't want to have recursive SIGSEGV's etc, for example, 1307 * that is why we also clear SIGNAL_UNKILLABLE. 1308 */ 1309 static int 1310 force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t) > It's worth making sure that this doesn't end up sending duplicate > signals. If nothing else, this would impact the vsyscall emulation > code. I am not totally get the "duplicate signals" meaning , SIGBUS is a fatal signal and if it was processed, the process should exit and another same signal will not be processed i think. Or if the process capture the signal and not to exit, duplicate SIGBUS signal seems not a problem if that happens For vsyscall emulation: I do check the related code, and this may be a read operation like instruction fetch for the issue, it will not hit the modified branch but go to emulation code, it seems we can't differentiate between a vsyscall emulation page fault and a hwposion page fault, for current code it may access the invalid page again and lead to a panic. This patch will not cover this scenario. > Programs that get a signal might expect that the RIP that the signal > frame points to is the instruction that caused the signal and that the > instruction faulted without side effects. For SIGSEGV, I would be > especially nervous about this. Maybe SIGBUS is safer. For SIGSEGV, > it's entirely valid to look at CR2 / si_fault_addr, fix it up, and > return. This would be completely *invalid* with your patch. I'm not > sure what to do about this. Do you mean the patch will replace the SIGSEGV with SIGBUS for hwposion case? I think SIGBUS is more accurate for the error. Normally for poison access, the process shouldn't be returned and an exit will be good or we need another code stream for this I think. This is the legacy way to process user poison access error like other posion code branch in kernel. Thanks! Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Wed, Feb 24, 2021 at 8:47 PM Aili Yao wrote: > > On Tue, 23 Feb 2021 08:42:59 -0800 > "Luck, Tony" wrote: > > > On Tue, Feb 23, 2021 at 07:33:46AM -0800, Andy Lutomirski wrote: > > > > > > > On Feb 23, 2021, at 4:44 AM, Aili Yao wrote: > > > > > > > > On Fri, 5 Feb 2021 17:01:35 +0800 > > > > Aili Yao wrote: > > > > > > > >> When one page is already hwpoisoned by MCE AO action, processes may not > > > >> be killed, processes mapping this page may make a syscall include this > > > >> page and result to trigger a VM_FAULT_HWPOISON fault, as it's in kernel > > > >> mode it may be fixed by fixup_exception, current code will just return > > > >> error code to user code. > > > >> > > > >> This is not sufficient, we should send a SIGBUS to the process and log > > > >> the info to console, as we can't trust the process will handle the > > > >> error > > > >> correctly. > > > >> > > > >> Suggested-by: Feng Yang > > > >> Signed-off-by: Aili Yao > > > >> --- > > > >> arch/x86/mm/fault.c | 62 + > > > >> 1 file changed, 40 insertions(+), 22 deletions(-) > > > >> > > > > Hi luto; > > > > Is there any feedback? > > > > > > At the very least, this needs a clear explanation of why your proposed > > > behavior is better than the existing behavior. > > > > The explanation is buried in that "can't trust the process" line. > > > > E.g. user space isn't good about checking for failed write(2) syscalls. > > So if the poison was in a user buffer passed to write(fd, buffer, count) > > sending a SIGBUS would be the action if they read the poison directly, > > so it seems reasonable to send the same signal if the kernel read their > > poison for them. > > > > It would avoid users that didn't check the return value merrily proceeding > > as if everything was ok. > > Hi luto: >I will add more infomation: >Even if the process will check return value of syscall like write, I don't > think > process will take proper action for this. >In test example, the return value will be errno is 14 (Bad Address), the > process may not realize > this is a hw issue, and may take wrong action not as expected. >And totally, A hw error will rarely happen, and the hw error hitting this > branch will be > more unlikely, the impaction without this patch is quite minor, but this is > still not good enough, we should > make it better, right? There are a few issues I can imagine: Some programs may use read(2), write(2), etc as ways to check if memory is valid without getting a signal. They might not want signals, which means that this feature might need to be configurable. It's worth making sure that this doesn't end up sending duplicate signals. If nothing else, this would impact the vsyscall emulation code. Programs that get a signal might expect that the RIP that the signal frame points to is the instruction that caused the signal and that the instruction faulted without side effects. For SIGSEGV, I would be especially nervous about this. Maybe SIGBUS is safer. For SIGSEGV, it's entirely valid to look at CR2 / si_fault_addr, fix it up, and return. This would be completely *invalid* with your patch. I'm not sure what to do about this. --Andy
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Tue, 23 Feb 2021 08:42:59 -0800 "Luck, Tony" wrote: > On Tue, Feb 23, 2021 at 07:33:46AM -0800, Andy Lutomirski wrote: > > > > > On Feb 23, 2021, at 4:44 AM, Aili Yao wrote: > > > > > > On Fri, 5 Feb 2021 17:01:35 +0800 > > > Aili Yao wrote: > > > > > >> When one page is already hwpoisoned by MCE AO action, processes may not > > >> be killed, processes mapping this page may make a syscall include this > > >> page and result to trigger a VM_FAULT_HWPOISON fault, as it's in kernel > > >> mode it may be fixed by fixup_exception, current code will just return > > >> error code to user code. > > >> > > >> This is not sufficient, we should send a SIGBUS to the process and log > > >> the info to console, as we can't trust the process will handle the error > > >> correctly. > > >> > > >> Suggested-by: Feng Yang > > >> Signed-off-by: Aili Yao > > >> --- > > >> arch/x86/mm/fault.c | 62 + > > >> 1 file changed, 40 insertions(+), 22 deletions(-) > > >> > > > Hi luto; > > > Is there any feedback? > > > > At the very least, this needs a clear explanation of why your proposed > > behavior is better than the existing behavior. > > The explanation is buried in that "can't trust the process" line. > > E.g. user space isn't good about checking for failed write(2) syscalls. > So if the poison was in a user buffer passed to write(fd, buffer, count) > sending a SIGBUS would be the action if they read the poison directly, > so it seems reasonable to send the same signal if the kernel read their > poison for them. > > It would avoid users that didn't check the return value merrily proceeding > as if everything was ok. Hi luto: I will add more infomation: Even if the process will check return value of syscall like write, I don't think process will take proper action for this. In test example, the return value will be errno is 14 (Bad Address), the process may not realize this is a hw issue, and may take wrong action not as expected. And totally, A hw error will rarely happen, and the hw error hitting this branch will be more unlikely, the impaction without this patch is quite minor, but this is still not good enough, we should make it better, right? Thanks Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Tue, Feb 23, 2021 at 07:33:46AM -0800, Andy Lutomirski wrote: > > > On Feb 23, 2021, at 4:44 AM, Aili Yao wrote: > > > > On Fri, 5 Feb 2021 17:01:35 +0800 > > Aili Yao wrote: > > > >> When one page is already hwpoisoned by MCE AO action, processes may not > >> be killed, processes mapping this page may make a syscall include this > >> page and result to trigger a VM_FAULT_HWPOISON fault, as it's in kernel > >> mode it may be fixed by fixup_exception, current code will just return > >> error code to user code. > >> > >> This is not sufficient, we should send a SIGBUS to the process and log > >> the info to console, as we can't trust the process will handle the error > >> correctly. > >> > >> Suggested-by: Feng Yang > >> Signed-off-by: Aili Yao > >> --- > >> arch/x86/mm/fault.c | 62 + > >> 1 file changed, 40 insertions(+), 22 deletions(-) > >> > > Hi luto; > > Is there any feedback? > > At the very least, this needs a clear explanation of why your proposed > behavior is better than the existing behavior. The explanation is buried in that "can't trust the process" line. E.g. user space isn't good about checking for failed write(2) syscalls. So if the poison was in a user buffer passed to write(fd, buffer, count) sending a SIGBUS would be the action if they read the poison directly, so it seems reasonable to send the same signal if the kernel read their poison for them. It would avoid users that didn't check the return value merrily proceeding as if everything was ok. -Tony
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
> On Feb 23, 2021, at 4:44 AM, Aili Yao wrote: > > On Fri, 5 Feb 2021 17:01:35 +0800 > Aili Yao wrote: > >> When one page is already hwpoisoned by MCE AO action, processes may not >> be killed, processes mapping this page may make a syscall include this >> page and result to trigger a VM_FAULT_HWPOISON fault, as it's in kernel >> mode it may be fixed by fixup_exception, current code will just return >> error code to user code. >> >> This is not sufficient, we should send a SIGBUS to the process and log >> the info to console, as we can't trust the process will handle the error >> correctly. >> >> Suggested-by: Feng Yang >> Signed-off-by: Aili Yao >> --- >> arch/x86/mm/fault.c | 62 + >> 1 file changed, 40 insertions(+), 22 deletions(-) >> > Hi luto; > Is there any feedback? At the very least, this needs a clear explanation of why your proposed behavior is better than the existing behavior. > > Thanks > Aili Yao
Re: [PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
On Fri, 5 Feb 2021 17:01:35 +0800 Aili Yao wrote: > When one page is already hwpoisoned by MCE AO action, processes may not > be killed, processes mapping this page may make a syscall include this > page and result to trigger a VM_FAULT_HWPOISON fault, as it's in kernel > mode it may be fixed by fixup_exception, current code will just return > error code to user code. > > This is not sufficient, we should send a SIGBUS to the process and log > the info to console, as we can't trust the process will handle the error > correctly. > > Suggested-by: Feng Yang > Signed-off-by: Aili Yao > --- > arch/x86/mm/fault.c | 62 + > 1 file changed, 40 insertions(+), 22 deletions(-) > Hi luto; Is there any feedback? Thanks Aili Yao
[PATCH v3] x86/fault: Send a SIGBUS to user process always for hwpoison page access.
When one page is already hwpoisoned by MCE AO action, processes may not
be killed, processes mapping this page may make a syscall include this
page and result to trigger a VM_FAULT_HWPOISON fault, as it's in kernel
mode it may be fixed by fixup_exception, current code will just return
error code to user code.
This is not sufficient, we should send a SIGBUS to the process and log
the info to console, as we can't trust the process will handle the error
correctly.
Suggested-by: Feng Yang
Signed-off-by: Aili Yao
---
arch/x86/mm/fault.c | 62 +
1 file changed, 40 insertions(+), 22 deletions(-)
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 08f5f74cf989..62df798abb56 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -617,6 +617,30 @@ static void set_signal_archinfo(unsigned long address,
tsk->thread.cr2 = address;
}
+static int
+do_sigbus_mceerr(unsigned long error_code, unsigned long address, vm_fault_t
fault, int prepared)
+{
+ struct task_struct *tsk = current;
+ unsigned int lsb = 0;
+
+ if (fault & (VM_FAULT_HWPOISON|VM_FAULT_HWPOISON_LARGE)) {
+ pr_err(
+ "MCE: Killing %s:%d due to hardware memory corruption fault at %lx\n",
+ tsk->comm, tsk->pid, address);
+ if (fault & VM_FAULT_HWPOISON_LARGE)
+ lsb = hstate_index_to_shift(VM_FAULT_GET_HINDEX(fault));
+ if (fault & VM_FAULT_HWPOISON)
+ lsb = PAGE_SHIFT;
+ if (!prepared) {
+ sanitize_error_code(address, _code);
+ set_signal_archinfo(address, error_code);
+ }
+ force_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, lsb);
+ return 1;
+ }
+ return 0;
+}
+
static noinline void
page_fault_oops(struct pt_regs *regs, unsigned long error_code,
unsigned long address)
@@ -694,7 +718,7 @@ page_fault_oops(struct pt_regs *regs, unsigned long
error_code,
static noinline void
kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
-unsigned long address, int signal, int si_code)
+unsigned long address, int signal, int si_code,
vm_fault_t fault)
{
WARN_ON_ONCE(user_mode(regs));
@@ -714,12 +738,17 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned
long error_code,
* In this case we need to make sure we're not recursively
* faulting through the emulate_vsyscall() logic.
*/
+
+ /* Sending MCERR Sigbus for page fault error from hwpoison */
+ if (IS_ENABLED(CONFIG_MEMORY_FAILURE)
+ && do_sigbus_mceerr(error_code, address, fault, 0))
+ return;
+
if (current->thread.sig_on_uaccess_err && signal) {
sanitize_error_code(address, _code);
set_signal_archinfo(address, error_code);
- /* XXX: hwpoison faults will set the wrong code. */
force_sig_fault(signal, si_code, (void __user
*)address);
}
@@ -782,7 +811,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long
error_code,
struct task_struct *tsk = current;
if (!user_mode(regs)) {
- kernelmode_fixup_or_oops(regs, error_code, address, pkey,
si_code);
+ kernelmode_fixup_or_oops(regs, error_code, address, pkey,
si_code, 0);
return;
}
@@ -914,7 +943,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code,
unsigned long address,
{
/* Kernel mode? Handle exceptions or die: */
if (!user_mode(regs)) {
- kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS,
BUS_ADRERR);
+ kernelmode_fixup_or_oops(regs, error_code, address, SIGBUS,
BUS_ADRERR, fault);
return;
}
@@ -929,22 +958,11 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code,
unsigned long address,
set_signal_archinfo(address, error_code);
-#ifdef CONFIG_MEMORY_FAILURE
- if (fault & (VM_FAULT_HWPOISON|VM_FAULT_HWPOISON_LARGE)) {
- struct task_struct *tsk = current;
- unsigned lsb = 0;
-
- pr_err(
- "MCE: Killing %s:%d due to hardware memory corruption fault at %lx\n",
- tsk->comm, tsk->pid, address);
- if (fault & VM_FAULT_HWPOISON_LARGE)
- lsb = hstate_index_to_shift(VM_FAULT_GET_HINDEX(fault));
- if (fault & VM_FAULT_HWPOISON)
- lsb = PAGE_SHIFT;
- force_sig_mceerr(BUS_MCEERR_AR, (void __user *)address, lsb);
+ /* Sending MCERR Sigbus for page fault error from hwpoison */
+ if (IS_ENABLED(CONFIG_MEMORY_FAILURE)
+ &&
