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

Groups > linux.kernel > #1669947 > unrolled thread

[PATCH 3/4] randstruct: Disable randomization of ACPICA structs

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

  [PATCH 3/4] randstruct: Disable randomization of ACPICA structs Kees Cook <keescook@chromium.org> - 2017-06-19 23:00 +0200
    Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs Christoph Hellwig <hch@infradead.org> - 2017-06-20 09:00 +0200
      Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs Kees Cook <keescook@chromium.org> - 2017-06-20 21:30 +0200
        Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs Christoph Hellwig <hch@infradead.org> - 2017-06-20 22:40 +0200
          Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs "Rafael J. Wysocki" <rafael@kernel.org> - 2017-06-20 23:00 +0200
          Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs "Rafael J. Wysocki" <rafael@kernel.org> - 2017-06-20 23:40 +0200
            Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs Kees Cook <keescook@chromium.org> - 2017-06-23 02:00 +0200
              Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-23 02:10 +0200
                Re: [PATCH 3/4] randstruct: Disable randomization of ACPICA structs Kees Cook <keescook@chromium.org> - 2017-06-23 02:30 +0200

#1669947 — [PATCH 3/4] randstruct: Disable randomization of ACPICA structs

Since the ACPICA source is maintained externally to the kernel, we can
neither switch it to designated initializers nor mark it
__no_randomize_layout. Until ACPICA-upstream changes[1] land to handle the
designated initialization, explicitly skip it in the plugin.

[1] https://github.com/acpica/acpica/pull/248

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/randomize_layout_plugin.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index f777ead58ba8..e6e02a40d522 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -346,6 +346,10 @@ static int relayout_struct(tree type)
 	    !strcmp((const char *)ORIG_TYPE_NAME(type), "RAWPCIFACTORY"))
 		return 0;
 
+	/* Skip ACPICA structs until refreshed with designated_init. */
+	if (!strcmp((const char *)ORIG_TYPE_NAME(type), "acpi_sleep_functions"))
+		return 0;
+
 	/* throw out any structs in uapi */
 	xloc = expand_location(DECL_SOURCE_LOCATION(TYPE_FIELDS(type)));
 
-- 
2.7.4

[toc] | [next] | [standalone]

#1670344

On Mon, Jun 19, 2017 at 01:56:40PM -0700, Kees Cook wrote:
> Since the ACPICA source is maintained externally to the kernel, we can
> neither switch it to designated initializers nor mark it
> __no_randomize_layout. Until ACPICA-upstream changes[1] land to handle the
> designated initialization, explicitly skip it in the plugin.

NAK.  ACPI has no business rejecting kernel changes to start with, but
independent of that your patch was complete garbage anyway.

There is no need for function pointers here, please include the patch
below instead:

---
From e8046f6507c2ed60bc501a0c0caa5a3f15f5e3e4 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Sun, 28 May 2017 09:53:45 +0300
Subject: acpi: get rid of acpi_sleep_dispatch

No need for the array of structs of function pointers when we can just
call the handfull of functions directly.

This could be further cleaned up if acpi_gbl_reduced_hardware was defined
true in the ACPI_REDUCED_HARDWARE case, but that's material for the next
round.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/acpi/acpica/hwxfsleep.c | 89 +++++++++--------------------------------
 include/acpi/actypes.h          |  9 -----
 2 files changed, 18 insertions(+), 80 deletions(-)

diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
index 5733b1167e46..66fa3ebddd57 100644
--- a/drivers/acpi/acpica/hwxfsleep.c
+++ b/drivers/acpi/acpica/hwxfsleep.c
@@ -57,26 +57,6 @@ acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs,
 				   acpi_physical_address physical_address64);
 #endif
 
-static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
-
-/*
- * Dispatch table used to efficiently branch to the various sleep
- * functions.
- */
-#define ACPI_SLEEP_FUNCTION_ID         0
-#define ACPI_WAKE_PREP_FUNCTION_ID     1
-#define ACPI_WAKE_FUNCTION_ID          2
-
-/* Legacy functions are optional, based upon ACPI_REDUCED_HARDWARE */
-
-static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
-	 acpi_hw_extended_sleep},
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
-	 acpi_hw_extended_wake_prep},
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake), acpi_hw_extended_wake}
-};
-
 /*
  * These functions are removed for the ACPI_REDUCED_HARDWARE case:
  *      acpi_set_firmware_waking_vector
@@ -236,53 +216,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
 
 ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_s4bios)
 #endif				/* !ACPI_REDUCED_HARDWARE */
-/*******************************************************************************
- *
- * FUNCTION:    acpi_hw_sleep_dispatch
- *
- * PARAMETERS:  sleep_state         - Which sleep state to enter/exit
- *              function_id         - Sleep, wake_prep, or Wake
- *
- * RETURN:      Status from the invoked sleep handling function.
- *
- * DESCRIPTION: Dispatch a sleep/wake request to the appropriate handling
- *              function.
- *
- ******************************************************************************/
-static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id)
-{
-	acpi_status status;
-	struct acpi_sleep_functions *sleep_functions =
-	    &acpi_sleep_dispatch[function_id];
-
-#if (!ACPI_REDUCED_HARDWARE)
-	/*
-	 * If the Hardware Reduced flag is set (from the FADT), we must
-	 * use the extended sleep registers (FADT). Note: As per the ACPI
-	 * specification, these extended registers are to be used for HW-reduced
-	 * platforms only. They are not general-purpose replacements for the
-	 * legacy PM register sleep support.
-	 */
-	if (acpi_gbl_reduced_hardware) {
-		status = sleep_functions->extended_function(sleep_state);
-	} else {
-		/* Legacy sleep */
-
-		status = sleep_functions->legacy_function(sleep_state);
-	}
-
-	return (status);
-
-#else
-	/*
-	 * For the case where reduced-hardware-only code is being generated,
-	 * we know that only the extended sleep registers are available
-	 */
-	status = sleep_functions->extended_function(sleep_state);
-	return (status);
-
-#endif				/* !ACPI_REDUCED_HARDWARE */
-}
 
 /*******************************************************************************
  *
@@ -389,7 +322,12 @@ acpi_status acpi_enter_sleep_state(u8 sleep_state)
 		return_ACPI_STATUS(AE_AML_OPERAND_VALUE);
 	}
 
-	status = acpi_hw_sleep_dispatch(sleep_state, ACPI_SLEEP_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_sleep(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_sleep(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
@@ -415,8 +353,12 @@ acpi_status acpi_leave_sleep_state_prep(u8 sleep_state)
 
 	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state_prep);
 
-	status =
-	    acpi_hw_sleep_dispatch(sleep_state, ACPI_WAKE_PREP_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_wake_prep(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_wake_prep(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
@@ -440,7 +382,12 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
 
 	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state);
 
-	status = acpi_hw_sleep_dispatch(sleep_state, ACPI_WAKE_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_wake(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_wake(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index d549e31c6d18..cfcb3abc65b9 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -894,15 +894,6 @@ typedef u8 acpi_adr_space_type;
 #define ACPI_ENABLE_EVENT                       1
 #define ACPI_DISABLE_EVENT                      0
 
-/* Sleep function dispatch */
-
-typedef acpi_status (*acpi_sleep_function) (u8 sleep_state);
-
-struct acpi_sleep_functions {
-	acpi_sleep_function legacy_function;
-	acpi_sleep_function extended_function;
-};
-
 /*
  * External ACPI object definition
  */
-- 
2.11.0

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

#1671034

On Mon, Jun 19, 2017 at 11:56 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jun 19, 2017 at 01:56:40PM -0700, Kees Cook wrote:
>> Since the ACPICA source is maintained externally to the kernel, we can
>> neither switch it to designated initializers nor mark it
>> __no_randomize_layout. Until ACPICA-upstream changes[1] land to handle the
>> designated initialization, explicitly skip it in the plugin.
>
> NAK.  ACPI has no business rejecting kernel changes to start with, but
> independent of that your patch was complete garbage anyway.

While I don't disagree with your opinion about ACPICA's inclusion in
the kernel, that isn't a battle I want to have. The ACPI maintainers
have a certain way of doing things, and what I need to change is tiny
compared to that.

> There is no need for function pointers here, please include the patch
> below instead:

Can you send the patch to https://github.com/acpica/acpica ? My change
was finally accepted, so this whole issue will go away on the next
refresh. Until then, I don't want to block the entire automatic
structure selection logic of randstruct on a three-function table. :)

Given that it's a tiny exclusion for randstruct, and there is already
a path in motion to fix it, I think this patch is trivial and
sufficient.

-Kees

-- 
Kees Cook
Pixel Security

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

#1671094

On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
> Can you send the patch to https://github.com/acpica/acpica ? My change
> was finally accepted, so this whole issue will go away on the next
> refresh. Until then, I don't want to block the entire automatic
> structure selection logic of randstruct on a three-function table. :)

I do not have a github account and no such thing is required for kernel
development.

I've already CCed the ACPI maintainer the last time I sent the patch,
and I would much prefer if they'd just include it to play silly games.
Ccing them and Linus once again to sort this process mess out.

> Given that it's a tiny exclusion for randstruct, and there is already
> a path in motion to fix it, I think this patch is trivial and
> sufficient.

But it's pointless - just do the right thing.

---
From e8046f6507c2ed60bc501a0c0caa5a3f15f5e3e4 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Sun, 28 May 2017 09:53:45 +0300
Subject: acpi: get rid of acpi_sleep_dispatch

No need for the array of structs of function pointers when we can just
call the handfull of functions directly.

This could be further cleaned up if acpi_gbl_reduced_hardware was defined
true in the ACPI_REDUCED_HARDWARE case, but that's material for the next
round.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/acpi/acpica/hwxfsleep.c | 89 +++++++++--------------------------------
 include/acpi/actypes.h          |  9 -----
 2 files changed, 18 insertions(+), 80 deletions(-)

diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
index 5733b1167e46..66fa3ebddd57 100644
--- a/drivers/acpi/acpica/hwxfsleep.c
+++ b/drivers/acpi/acpica/hwxfsleep.c
@@ -57,26 +57,6 @@ acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs,
 				   acpi_physical_address physical_address64);
 #endif
 
-static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
-
-/*
- * Dispatch table used to efficiently branch to the various sleep
- * functions.
- */
-#define ACPI_SLEEP_FUNCTION_ID         0
-#define ACPI_WAKE_PREP_FUNCTION_ID     1
-#define ACPI_WAKE_FUNCTION_ID          2
-
-/* Legacy functions are optional, based upon ACPI_REDUCED_HARDWARE */
-
-static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
-	 acpi_hw_extended_sleep},
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
-	 acpi_hw_extended_wake_prep},
-	{ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake), acpi_hw_extended_wake}
-};
-
 /*
  * These functions are removed for the ACPI_REDUCED_HARDWARE case:
  *      acpi_set_firmware_waking_vector
@@ -236,53 +216,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
 
 ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_s4bios)
 #endif				/* !ACPI_REDUCED_HARDWARE */
-/*******************************************************************************
- *
- * FUNCTION:    acpi_hw_sleep_dispatch
- *
- * PARAMETERS:  sleep_state         - Which sleep state to enter/exit
- *              function_id         - Sleep, wake_prep, or Wake
- *
- * RETURN:      Status from the invoked sleep handling function.
- *
- * DESCRIPTION: Dispatch a sleep/wake request to the appropriate handling
- *              function.
- *
- ******************************************************************************/
-static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id)
-{
-	acpi_status status;
-	struct acpi_sleep_functions *sleep_functions =
-	    &acpi_sleep_dispatch[function_id];
-
-#if (!ACPI_REDUCED_HARDWARE)
-	/*
-	 * If the Hardware Reduced flag is set (from the FADT), we must
-	 * use the extended sleep registers (FADT). Note: As per the ACPI
-	 * specification, these extended registers are to be used for HW-reduced
-	 * platforms only. They are not general-purpose replacements for the
-	 * legacy PM register sleep support.
-	 */
-	if (acpi_gbl_reduced_hardware) {
-		status = sleep_functions->extended_function(sleep_state);
-	} else {
-		/* Legacy sleep */
-
-		status = sleep_functions->legacy_function(sleep_state);
-	}
-
-	return (status);
-
-#else
-	/*
-	 * For the case where reduced-hardware-only code is being generated,
-	 * we know that only the extended sleep registers are available
-	 */
-	status = sleep_functions->extended_function(sleep_state);
-	return (status);
-
-#endif				/* !ACPI_REDUCED_HARDWARE */
-}
 
 /*******************************************************************************
  *
@@ -389,7 +322,12 @@ acpi_status acpi_enter_sleep_state(u8 sleep_state)
 		return_ACPI_STATUS(AE_AML_OPERAND_VALUE);
 	}
 
-	status = acpi_hw_sleep_dispatch(sleep_state, ACPI_SLEEP_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_sleep(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_sleep(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
@@ -415,8 +353,12 @@ acpi_status acpi_leave_sleep_state_prep(u8 sleep_state)
 
 	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state_prep);
 
-	status =
-	    acpi_hw_sleep_dispatch(sleep_state, ACPI_WAKE_PREP_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_wake_prep(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_wake_prep(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
@@ -440,7 +382,12 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
 
 	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state);
 
-	status = acpi_hw_sleep_dispatch(sleep_state, ACPI_WAKE_FUNCTION_ID);
+#if !ACPI_REDUCED_HARDWARE
+	if (!acpi_gbl_reduced_hardware)
+		status = acpi_hw_legacy_wake(sleep_state);
+	else
+#endif
+		status = acpi_hw_extended_wake(sleep_state);
 	return_ACPI_STATUS(status);
 }
 
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index d549e31c6d18..cfcb3abc65b9 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -894,15 +894,6 @@ typedef u8 acpi_adr_space_type;
 #define ACPI_ENABLE_EVENT                       1
 #define ACPI_DISABLE_EVENT                      0
 
-/* Sleep function dispatch */
-
-typedef acpi_status (*acpi_sleep_function) (u8 sleep_state);
-
-struct acpi_sleep_functions {
-	acpi_sleep_function legacy_function;
-	acpi_sleep_function extended_function;
-};
-
 /*
  * External ACPI object definition
  */
-- 
2.11.0

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

#1671115

On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>> Can you send the patch to https://github.com/acpica/acpica ? My change
>> was finally accepted, so this whole issue will go away on the next
>> refresh. Until then, I don't want to block the entire automatic
>> structure selection logic of randstruct on a three-function table. :)
>
> I do not have a github account and no such thing is required for kernel
> development.
>
> I've already CCed the ACPI maintainer the last time I sent the patch,
> and I would much prefer if they'd just include it to play silly games.
> Ccing them and Linus once again to sort this process mess out.
>
>> Given that it's a tiny exclusion for randstruct, and there is already
>> a path in motion to fix it, I think this patch is trivial and
>> sufficient.
>
> But it's pointless - just do the right thing.
>
> ---
> From e8046f6507c2ed60bc501a0c0caa5a3f15f5e3e4 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Sun, 28 May 2017 09:53:45 +0300
> Subject: acpi: get rid of acpi_sleep_dispatch
>
> No need for the array of structs of function pointers when we can just
> call the handfull of functions directly.
>
> This could be further cleaned up if acpi_gbl_reduced_hardware was defined
> true in the ACPI_REDUCED_HARDWARE case, but that's material for the next
> round.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/acpi/acpica/hwxfsleep.c | 89 +++++++++--------------------------------
>  include/acpi/actypes.h          |  9 -----
>  2 files changed, 18 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
> index 5733b1167e46..66fa3ebddd57 100644
> --- a/drivers/acpi/acpica/hwxfsleep.c
> +++ b/drivers/acpi/acpica/hwxfsleep.c
> @@ -57,26 +57,6 @@ acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs,
>                                    acpi_physical_address physical_address64);
>  #endif
>
> -static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
> -
> -/*
> - * Dispatch table used to efficiently branch to the various sleep
> - * functions.
> - */
> -#define ACPI_SLEEP_FUNCTION_ID         0
> -#define ACPI_WAKE_PREP_FUNCTION_ID     1
> -#define ACPI_WAKE_FUNCTION_ID          2
> -
> -/* Legacy functions are optional, based upon ACPI_REDUCED_HARDWARE */
> -
> -static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
> -       {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_sleep),
> -        acpi_hw_extended_sleep},
> -       {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake_prep),
> -        acpi_hw_extended_wake_prep},
> -       {ACPI_HW_OPTIONAL_FUNCTION(acpi_hw_legacy_wake), acpi_hw_extended_wake}
> -};
> -
>  /*
>   * These functions are removed for the ACPI_REDUCED_HARDWARE case:
>   *      acpi_set_firmware_waking_vector
> @@ -236,53 +216,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
>
>  ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_s4bios)
>  #endif                         /* !ACPI_REDUCED_HARDWARE */
> -/*******************************************************************************
> - *
> - * FUNCTION:    acpi_hw_sleep_dispatch
> - *
> - * PARAMETERS:  sleep_state         - Which sleep state to enter/exit
> - *              function_id         - Sleep, wake_prep, or Wake
> - *
> - * RETURN:      Status from the invoked sleep handling function.
> - *
> - * DESCRIPTION: Dispatch a sleep/wake request to the appropriate handling
> - *              function.
> - *
> - ******************************************************************************/
> -static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id)
> -{
> -       acpi_status status;
> -       struct acpi_sleep_functions *sleep_functions =
> -           &acpi_sleep_dispatch[function_id];
> -
> -#if (!ACPI_REDUCED_HARDWARE)
> -       /*
> -        * If the Hardware Reduced flag is set (from the FADT), we must
> -        * use the extended sleep registers (FADT). Note: As per the ACPI
> -        * specification, these extended registers are to be used for HW-reduced
> -        * platforms only. They are not general-purpose replacements for the
> -        * legacy PM register sleep support.
> -        */
> -       if (acpi_gbl_reduced_hardware) {
> -               status = sleep_functions->extended_function(sleep_state);
> -       } else {
> -               /* Legacy sleep */
> -
> -               status = sleep_functions->legacy_function(sleep_state);
> -       }
> -
> -       return (status);
> -
> -#else
> -       /*
> -        * For the case where reduced-hardware-only code is being generated,
> -        * we know that only the extended sleep registers are available
> -        */
> -       status = sleep_functions->extended_function(sleep_state);
> -       return (status);
> -
> -#endif                         /* !ACPI_REDUCED_HARDWARE */
> -}
>
>  /*******************************************************************************
>   *
> @@ -389,7 +322,12 @@ acpi_status acpi_enter_sleep_state(u8 sleep_state)
>                 return_ACPI_STATUS(AE_AML_OPERAND_VALUE);
>         }
>
> -       status = acpi_hw_sleep_dispatch(sleep_state, ACPI_SLEEP_FUNCTION_ID);
> +#if !ACPI_REDUCED_HARDWARE
> +       if (!acpi_gbl_reduced_hardware)
> +               status = acpi_hw_legacy_sleep(sleep_state);
> +       else
> +#endif
> +               status = acpi_hw_extended_sleep(sleep_state);
>         return_ACPI_STATUS(status);
>  }
>

I guess you can avoid these #ifs in function bodies?

Thanks,
Rafael

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

#1671155

On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>> Can you send the patch to https://github.com/acpica/acpica ? My change
>> was finally accepted, so this whole issue will go away on the next
>> refresh. Until then, I don't want to block the entire automatic
>> structure selection logic of randstruct on a three-function table. :)
>
> I do not have a github account and no such thing is required for kernel
> development.

It isn't required for the ACPICA material either.

You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
your ACPICA patches.  They pick up stuff that looks good to them.

And we tend to prefer routing ACPICA changes through the upstream,
because failing to do so usually turns out to be painful in the long
term.  I don't think it is unreasonable to ask for cooperation in that
respect.

Thanks,
Rafael

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

#1673140

On Tue, Jun 20, 2017 at 2:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>>> Can you send the patch to https://github.com/acpica/acpica ? My change
>>> was finally accepted, so this whole issue will go away on the next
>>> refresh. Until then, I don't want to block the entire automatic
>>> structure selection logic of randstruct on a three-function table. :)
>>
>> I do not have a github account and no such thing is required for kernel
>> development.
>
> It isn't required for the ACPICA material either.
>
> You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
> your ACPICA patches.  They pick up stuff that looks good to them.
>
> And we tend to prefer routing ACPICA changes through the upstream,
> because failing to do so usually turns out to be painful in the long
> term.  I don't think it is unreasonable to ask for cooperation in that
> respect.

I'd like to unblock randstruct, so what's the easiest way to move
this? My version of changes have already landed upstream in ACPICA,
but I don't know how frequently they get flushed back into the kernel.

I can't turn on randstruct auto-selection in -next without either
ACPICA using (or not needing) designated initializers or just
blacklisting it in the randstruct plugin itself. I would much prefer
the latter as the problem is solved in ACPICA upstream already but
just isn't in the kernel yet, and I want to get testing of the
auto-selection ASAP. Once it's in the kernel I can drop the blacklist.

Christoph: how about a middle ground where randstruct blacklists
ACPICA in -next and if ACPICA is fixed by the time the merge window
opens, I'll drop the blacklist. That gets the testing coverage without
what you see as an ugly hack right now. I just really don't want to
waste any more time on this since there are SO many other randomized
structures I'd like to be sure are getting testing.

Alternatively, if the ACPICA folks Ack Christoph's patch, I can carry
that in the randstruct tree for -next instead?

Thanks,

-Kees

-- 
Kees Cook
Pixel Security

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

#1673142

On Thursday, June 22, 2017 04:57:39 PM Kees Cook wrote:
> On Tue, Jun 20, 2017 at 2:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
> >>> Can you send the patch to https://github.com/acpica/acpica ? My change
> >>> was finally accepted, so this whole issue will go away on the next
> >>> refresh. Until then, I don't want to block the entire automatic
> >>> structure selection logic of randstruct on a three-function table. :)
> >>
> >> I do not have a github account and no such thing is required for kernel
> >> development.
> >
> > It isn't required for the ACPICA material either.
> >
> > You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
> > your ACPICA patches.  They pick up stuff that looks good to them.
> >
> > And we tend to prefer routing ACPICA changes through the upstream,
> > because failing to do so usually turns out to be painful in the long
> > term.  I don't think it is unreasonable to ask for cooperation in that
> > respect.
> 
> I'd like to unblock randstruct, so what's the easiest way to move
> this? My version of changes have already landed upstream in ACPICA,
> but I don't know how frequently they get flushed back into the kernel.

Usually, when there's a new ACPICA release, but occasionally that happens
faster.

Which commit in upstream ACPICA is this?

> I can't turn on randstruct auto-selection in -next without either
> ACPICA using (or not needing) designated initializers or just
> blacklisting it in the randstruct plugin itself. I would much prefer
> the latter as the problem is solved in ACPICA upstream already but
> just isn't in the kernel yet, and I want to get testing of the
> auto-selection ASAP. Once it's in the kernel I can drop the blacklist.
> 
> Christoph: how about a middle ground where randstruct blacklists
> ACPICA in -next and if ACPICA is fixed by the time the merge window
> opens, I'll drop the blacklist. That gets the testing coverage without
> what you see as an ugly hack right now. I just really don't want to
> waste any more time on this since there are SO many other randomized
> structures I'd like to be sure are getting testing.
> 
> Alternatively, if the ACPICA folks Ack Christoph's patch, I can carry
> that in the randstruct tree for -next instead?

Maybe we can simply forward port the ACPICA commit right away.

Lv, can you take care of this, please?

Thanks,
Rafael

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

#1673150

On Thu, Jun 22, 2017 at 4:59 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, June 22, 2017 04:57:39 PM Kees Cook wrote:
>> On Tue, Jun 20, 2017 at 2:34 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Tue, Jun 20, 2017 at 10:35 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> >> On Tue, Jun 20, 2017 at 12:25:53PM -0700, Kees Cook wrote:
>> >>> Can you send the patch to https://github.com/acpica/acpica ? My change
>> >>> was finally accepted, so this whole issue will go away on the next
>> >>> refresh. Until then, I don't want to block the entire automatic
>> >>> structure selection logic of randstruct on a three-function table. :)
>> >>
>> >> I do not have a github account and no such thing is required for kernel
>> >> development.
>> >
>> > It isn't required for the ACPICA material either.
>> >
>> > You just need to CC the ACPICA maintainers, as per MAINTAINERS, on
>> > your ACPICA patches.  They pick up stuff that looks good to them.
>> >
>> > And we tend to prefer routing ACPICA changes through the upstream,
>> > because failing to do so usually turns out to be painful in the long
>> > term.  I don't think it is unreasonable to ask for cooperation in that
>> > respect.
>>
>> I'd like to unblock randstruct, so what's the easiest way to move
>> this? My version of changes have already landed upstream in ACPICA,
>> but I don't know how frequently they get flushed back into the kernel.
>
> Usually, when there's a new ACPICA release, but occasionally that happens
> faster.
>
> Which commit in upstream ACPICA is this?

https://github.com/acpica/acpica/commit/2058b3bf5deecb9644d676703bd97d1bce5e612a

>> I can't turn on randstruct auto-selection in -next without either
>> ACPICA using (or not needing) designated initializers or just
>> blacklisting it in the randstruct plugin itself. I would much prefer
>> the latter as the problem is solved in ACPICA upstream already but
>> just isn't in the kernel yet, and I want to get testing of the
>> auto-selection ASAP. Once it's in the kernel I can drop the blacklist.
>>
>> Christoph: how about a middle ground where randstruct blacklists
>> ACPICA in -next and if ACPICA is fixed by the time the merge window
>> opens, I'll drop the blacklist. That gets the testing coverage without
>> what you see as an ugly hack right now. I just really don't want to
>> waste any more time on this since there are SO many other randomized
>> structures I'd like to be sure are getting testing.
>>
>> Alternatively, if the ACPICA folks Ack Christoph's patch, I can carry
>> that in the randstruct tree for -next instead?
>
> Maybe we can simply forward port the ACPICA commit right away.
>
> Lv, can you take care of this, please?

That would be great, thanks!

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web