Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1624717 > unrolled thread

Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to  user-space Dave Hansen <dave.hansen@linux.intel.com> - 2017-04-17 17:40 +0200
    Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to  user-space Joerg Roedel <jroedel@suse.de> - 2017-04-20 14:10 +0200
      Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to  user-space Dave Hansen <dave.hansen@linux.intel.com> - 2017-04-20 17:50 +0200
        Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to  user-space Joerg Roedel <jroedel@suse.de> - 2017-04-21 14:20 +0200
          Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to  user-space Dave Hansen <dave.hansen@linux.intel.com> - 2017-04-21 16:40 +0200

#1624717 — Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

Hi Joerg,

> When this function fails it just sends a SIGSEGV signal to
> user-space using force_sig(). This signal is missing
> essential information about the cause, e.g. the trap_nr or
> an error code.
>
> Fix this by propagating the error to the only caller of
> mpx_handle_bd_fault(), do_bounds(), which sends the correct
> SIGSEGV signal to the process.

Just to be clear, the thing you're calling "correct" is this do_trap(),
right?

        do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, NULL);

> Fixes: fe3d197f84319 ('x86, mpx: On-demand kernel allocation of bounds
tables')
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/mm/mpx.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> index cd44ae7..1c34b76 100644
> --- a/arch/x86/mm/mpx.c
> +++ b/arch/x86/mm/mpx.c
> @@ -526,15 +526,7 @@ int mpx_handle_bd_fault(void)
>  	if (!kernel_managing_mpx_tables(current->mm))
>  		return -EINVAL;
>
> -	if (do_mpx_bt_fault()) {
> -		force_sig(SIGSEGV, current);
> -		/*
> -		 * The force_sig() is essentially "handling" this
> -		 * exception, so we do not pass up the error
> -		 * from do_mpx_bt_fault().
> -		 */
> -	}
> -	return 0;
> +	return do_mpx_bt_fault();
>  }

do_mpx_bt_fault() can fail for a bunch of reasons:
 * unexpected or invalid value in BNDCSR
 * out of memory (physical or virtual)
 * unresolvable fault walking/filling bounds tables
 * !valid and non-empty bad entry in the bounds tables

This will end up sending a signal that *looks* like a X86_TRAP_BR for
all of those, including those that are not really bounds-related, like
unresolvable faults.  We also don't populate enough information in the
siginfo that gets delivered for userspace to resolve the fault.

I'm not sure this patch is the right thing.

[toc] | [next] | [standalone]

#1627399

Hi Dave,

On Mon, Apr 17, 2017 at 08:38:03AM -0700, Dave Hansen wrote:
> Just to be clear, the thing you're calling "correct" is this do_trap(),
> right?
> 
>         do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, NULL);

Yes, because it signals the right trap_nr and error_code to user-space.

> do_mpx_bt_fault() can fail for a bunch of reasons:
>  * unexpected or invalid value in BNDCSR
>  * out of memory (physical or virtual)
>  * unresolvable fault walking/filling bounds tables
>  * !valid and non-empty bad entry in the bounds tables
> 
> This will end up sending a signal that *looks* like a X86_TRAP_BR for
> all of those, including those that are not really bounds-related, like
> unresolvable faults.  We also don't populate enough information in the
> siginfo that gets delivered for userspace to resolve the fault.
> 
> I'm not sure this patch is the right thing.

The problem is, without this patch the trap_nr reported to user-space is
0, which maps to divide-by-zero. I think this is wrong, and since all
failure cases from do_mpx_bt_fault() can only happen in the #BR
exception handler, I think that reporting X86_TRAP_BR for all failure
cases is the right thing to do.

I don't know whether user-space (with this patch) already gets enough
information from do_trap() to handle all of the above cases, but it is a
step in the right direction.


	Joerg

[toc] | [prev] | [next] | [standalone]

#1627605

On 04/20/2017 05:08 AM, Joerg Roedel wrote:
>> do_mpx_bt_fault() can fail for a bunch of reasons:
>>  * unexpected or invalid value in BNDCSR
>>  * out of memory (physical or virtual)
>>  * unresolvable fault walking/filling bounds tables
>>  * !valid and non-empty bad entry in the bounds tables
>>
>> This will end up sending a signal that *looks* like a X86_TRAP_BR for
>> all of those, including those that are not really bounds-related, like
>> unresolvable faults.  We also don't populate enough information in the
>> siginfo that gets delivered for userspace to resolve the fault.
>>
>> I'm not sure this patch is the right thing.
> 
> The problem is, without this patch the trap_nr reported to user-space is
> 0, which maps to divide-by-zero. I think this is wrong, and since all
> failure cases from do_mpx_bt_fault() can only happen in the #BR
> exception handler, I think that reporting X86_TRAP_BR for all failure
> cases is the right thing to do.

Urg, that does sound bogus.

How about doing X86_TRAP_PF?  That would at least be consistent with
SIGBUS, which is probably the closest thing to a generic error code that
we have.

[toc] | [prev] | [next] | [standalone]

#1628193

On Thu, Apr 20, 2017 at 08:45:28AM -0700, Dave Hansen wrote:
> How about doing X86_TRAP_PF?  That would at least be consistent with
> SIGBUS, which is probably the closest thing to a generic error code that
> we have.

Correct me if I am wrong, but for SIGBUS this only happens in the
page-fault path, right? And this path is indeed entered on a #PF
exception.

I see no reason to lie to user-space about the trap_nr that caused the
SIGSEGV, especially since user-space software needs to be modified to
make use of MPX, including the signal handler. So there is no risk of
introducing any incompatibility or regression, no?


	Joerg

[toc] | [prev] | [next] | [standalone]

#1628349

On 04/21/2017 05:19 AM, Joerg Roedel wrote:
> On Thu, Apr 20, 2017 at 08:45:28AM -0700, Dave Hansen wrote:
>> How about doing X86_TRAP_PF?  That would at least be consistent with
>> SIGBUS, which is probably the closest thing to a generic error code that
>> we have.
> Correct me if I am wrong, but for SIGBUS this only happens in the
> page-fault path, right? And this path is indeed entered on a #PF
> exception.

It can happen to programs for tons of reasons.  It definitely happens
outside page faults.

> I see no reason to lie to user-space about the trap_nr that caused the
> SIGSEGV, especially since user-space software needs to be modified to
> make use of MPX, including the signal handler. So there is no risk of
> introducing any incompatibility or regression, no?

I think it's pretty safe to change.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web