[PATCH 0/3] Get rid of BUG()

[PATCH 0/3] Get rid of BUG()

[Jarkko Sakkinen]

The use of BUG() is not favored for any new kernel code. This patch set
implements more reasonable error recovery. In all cases the recovery is not
ideal but it is an improvement to the current situation.

In addition, sgx_free_page() has been simplified by removing the parameter for
skipping EREMOVE. EREMOVE can be successfully executed for an unused EPC page.

Jarkko Sakkinen (3):
  intel_sgx: do not use BUG() in sgx_free_page()
  intel_sgx: remove flags parameter from sgx_free_page()
  intel_sgx: fix error paths for EBLOCK and ETRACK

 drivers/platform/x86/intel_sgx.h            |  4 +-
 drivers/platform/x86/intel_sgx_ioctl.c      | 12 ++--
 drivers/platform/x86/intel_sgx_page_cache.c | 89 +++++++++++++++--------------
 drivers/platform/x86/intel_sgx_util.c       |  6 +-
 drivers/platform/x86/intel_sgx_vma.c        |  6 +-
 5 files changed, 55 insertions(+), 62 deletions(-)

-- 
2.9.3

[PATCH 1/3] intel_sgx: do not use BUG() in sgx_free_page()

[Jarkko Sakkinen]

EREMOVE fails on non-EPC page or when a SECS page with children is to be
removed. These do not happen if the driver is working correctly. Log the
error but do not crash the driver.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/platform/x86/intel_sgx_page_cache.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
index d073057..7f73ac7 100644
--- a/drivers/platform/x86/intel_sgx_page_cache.c
+++ b/drivers/platform/x86/intel_sgx_page_cache.c
@@ -551,10 +551,8 @@ void sgx_free_page(struct sgx_epc_page *entry,
 		ret = __eremove(epc);
 		sgx_put_epc_page(epc);
 
-		if (ret) {
-			pr_err("EREMOVE returned %d\n", ret);
-			BUG();
-		}
+		if (ret)
+			sgx_err(encl, "EREMOVE returned %d\n", ret);
 	}
 
 	spin_lock(&sgx_free_list_lock);
-- 
2.9.3

Re: [PATCH 1/3] intel_sgx: do not use BUG() in sgx_free_page()

[Andy Shevchenko]

On Thu, Jan 26, 2017 at 11:20 PM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> EREMOVE fails on non-EPC page or when a SECS page with children is to be
> removed. These do not happen if the driver is working correctly. Log the
> error but do not crash the driver.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/platform/x86/intel_sgx_page_cache.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> index d073057..7f73ac7 100644
> --- a/drivers/platform/x86/intel_sgx_page_cache.c
> +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> @@ -551,10 +551,8 @@ void sgx_free_page(struct sgx_epc_page *entry,
>                 ret = __eremove(epc);
>                 sgx_put_epc_page(epc);
>
> -               if (ret) {
> -                       pr_err("EREMOVE returned %d\n", ret);
> -                       BUG();
> -               }
> +               if (ret)
> +                       sgx_err(encl, "EREMOVE returned %d\n", ret);

Do you have something like critical level? For me seems reasonable to
increase the level of message if BUG() was somehow related to actual
situation.

>         }
>
>         spin_lock(&sgx_free_list_lock);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Get rid of BUG()

[Andy Shevchenko]

On Thu, Jan 26, 2017 at 11:20 PM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> The use of BUG() is not favored for any new kernel code. This patch set
> implements more reasonable error recovery. In all cases the recovery is not
> ideal but it is an improvement to the current situation.
>
> In addition, sgx_free_page() has been simplified by removing the parameter for
> skipping EREMOVE. EREMOVE can be successfully executed for an unused EPC page.
>
> Jarkko Sakkinen (3):

By some reason I only see first patch.
What's going on?

>   intel_sgx: do not use BUG() in sgx_free_page()
>   intel_sgx: remove flags parameter from sgx_free_page()
>   intel_sgx: fix error paths for EBLOCK and ETRACK
>
>  drivers/platform/x86/intel_sgx.h            |  4 +-
>  drivers/platform/x86/intel_sgx_ioctl.c      | 12 ++--
>  drivers/platform/x86/intel_sgx_page_cache.c | 89 +++++++++++++++--------------
>  drivers/platform/x86/intel_sgx_util.c       |  6 +-
>  drivers/platform/x86/intel_sgx_vma.c        |  6 +-
>  5 files changed, 55 insertions(+), 62 deletions(-)
>
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/3] intel_sgx: do not use BUG() in sgx_free_page()

[Jarkko Sakkinen]

On Fri, Jan 27, 2017 at 05:45:03PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 26, 2017 at 11:20 PM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > EREMOVE fails on non-EPC page or when a SECS page with children is to be
> > removed. These do not happen if the driver is working correctly. Log the
> > error but do not crash the driver.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/platform/x86/intel_sgx_page_cache.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_sgx_page_cache.c b/drivers/platform/x86/intel_sgx_page_cache.c
> > index d073057..7f73ac7 100644
> > --- a/drivers/platform/x86/intel_sgx_page_cache.c
> > +++ b/drivers/platform/x86/intel_sgx_page_cache.c
> > @@ -551,10 +551,8 @@ void sgx_free_page(struct sgx_epc_page *entry,
> >                 ret = __eremove(epc);
> >                 sgx_put_epc_page(epc);
> >
> > -               if (ret) {
> > -                       pr_err("EREMOVE returned %d\n", ret);
> > -                       BUG();
> > -               }
> > +               if (ret)
> > +                       sgx_err(encl, "EREMOVE returned %d\n", ret);
> 
> Do you have something like critical level? For me seems reasonable to
> increase the level of message if BUG() was somehow related to actual
> situation.

Hmm... I think that would make sense. This could only happen when
the driver implementation is working incorrectly.

/Jarkko