[PATCH 0/4] Remove problematic include in <asm/set_memory.h>

[PATCH 0/4] Remove problematic include in <asm/set_memory.h>

[Kevin Brodsky]

The need for this series arose from a completely unrelated series [1].
Long story short, that series causes <linux/mm.h> to include
<linux/set_memory.h>, which doesn't feel too unreasonable.

That works fine on arm64 and probably most other architectures, but not
on x86 [2]: <asm/set_memory.h> itself includes <linux/mm.h>, creating a
circular dependency.

set_memory.h doesn't really need <linux/mm.h>, so removing that include
seems like the right thing to do. That turned out to be a little more
involved than expected, hence this series:

* Patch 1-2 ensure that code doesn't rely on <asm/set_memory.h>
  including <linux/mm.h>. The errors that these patches fix are included
  at the end of this email.

* Patch 3 removes an unused function whose declaration relied on
  <linux/mm.h> being included.
 
* Patch 4 actually remove the include.

I've build-tested this series with x86_64_defconfig and allyesconfig. 

- Kevin

[1] https://lore.kernel.org/linux-hardening/20241206101110.1646108-1-kevin.brodsky@arm.com/
[2] https://lore.kernel.org/oe-kbuild-all/202412062006.C23V9ESs-lkp@intel.com/

Cc: bp@alien8.de
Cc: dan.j.williams@intel.com
Cc: dave.hansen@linux.intel.com
Cc: david@redhat.com
Cc: jane.chu@oracle.com
Cc: osalvador@suse.de
Cc: tglx@linutronix.de
---
Build errors caused by patch 4 and fixed by patch 1-2:

  CC      drivers/gpu/drm/i915/gt/intel_ggtt.o
In file included from arch/x86/include/asm/smp.h:9,
                 from drivers/gpu/drm/i915/gt/intel_ggtt.c:7:
arch/x86/include/asm/thread_info.h: In function 'arch_within_stack_frames':
arch/x86/include/asm/thread_info.h:212:16: error: 'NOT_STACK' undeclared (first use in this function)
  212 |         return NOT_STACK;
      |                ^~~~~~~~~
arch/x86/include/asm/thread_info.h:212:16: note: each undeclared identifier is reported only once for each function it appears in

  CC      drivers/virt/coco/sev-guest/sev-guest.o
drivers/virt/coco/sev-guest/sev-guest.c: In function 'free_shared_pages':
drivers/virt/coco/sev-guest/sev-guest.c:621:31: error: implicit declaration of function 'PAGE_ALIGN'; did you mean 'PFN_ALIGN'? [-Wimplicit-function-declaration]
  621 |         unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
      |                               ^~~~~~~~~~
      |                               PFN_ALIGN
  CC      drivers/infiniband/hw/mlx5/ib_rep.o
drivers/virt/coco/sev-guest/sev-guest.c: In function 'alloc_shared_pages':
drivers/virt/coco/sev-guest/sev-guest.c:646:51: error: implicit declaration of function 'page_address'; did you mean 'pbe_address'? [-Wimplicit-function-declaration]
  646 |         ret = set_memory_decrypted((unsigned long)page_address(page), npages);
      |                                                   ^~~~~~~~~~~~
      |                                                   pbe_address
drivers/virt/coco/sev-guest/sev-guest.c:653:16: error: returning 'int' from a function with return type 'void *' makes pointer from integer without a cast [-Wint-conversion]
  653 |         return page_address(page);
      |                ^~~~~~~~~~~~~~~~~~

---
Kevin Brodsky (4):
  x86/smp: Explicitly include <linux/thread_info.h>
  x86/sev: Explicitly include <linux/mm.h>
  x86/mm: Remove unused __set_memory_prot()
  x86/mm: Remove unnecessary include in set_memory.h

 arch/x86/include/asm/set_memory.h       |  2 --
 arch/x86/include/asm/smp.h              |  2 +-
 arch/x86/mm/pat/set_memory.c            | 13 -------------
 drivers/virt/coco/sev-guest/sev-guest.c |  1 +
 4 files changed, 2 insertions(+), 16 deletions(-)


base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
-- 
2.47.0

[PATCH 1/4] x86/smp: Explicitly include <linux/thread_info.h>

[Kevin Brodsky]

Including <asm/thread_info.h> directly relies on
<linux/thread_info.h> having already been included, because the
former needs the BAD_STACK/NOT_STACK constants defined in the
latter.

A subsequent patch will break that assumption in a file that
includes asm/smp.h. Include the full <linux/thread_info.h> to avoid
getting into troubles.

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 arch/x86/include/asm/smp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
index ca073f40698f..88e72b414bfa 100644
--- a/arch/x86/include/asm/smp.h
+++ b/arch/x86/include/asm/smp.h
@@ -6,7 +6,7 @@
 
 #include <asm/cpumask.h>
 #include <asm/current.h>
-#include <asm/thread_info.h>
+#include <linux/thread_info.h>
 
 DECLARE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_map);
 DECLARE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_map);
-- 
2.47.0

Re: [PATCH 1/4] x86/smp: Explicitly include <linux/thread_info.h>

[Borislav Petkov]

On Tue, Dec 10, 2024 at 06:46:07PM +0000, Kevin Brodsky wrote:
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index ca073f40698f..88e72b414bfa 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -6,7 +6,7 @@
>  
>  #include <asm/cpumask.h>
>  #include <asm/current.h>
> -#include <asm/thread_info.h>
> +#include <linux/thread_info.h>

linux/ namespace headers come before asm/ ones, I'd say.

But, more importantly, why is this 4 patches instead of 2:

1. Remove unused __set_memory_prot
2. Fixup include hell

?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/4] x86/smp: Explicitly include <linux/thread_info.h>

[Kevin Brodsky]

On 10/12/2024 22:36, Borislav Petkov wrote:
> On Tue, Dec 10, 2024 at 06:46:07PM +0000, Kevin Brodsky wrote:
>> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
>> index ca073f40698f..88e72b414bfa 100644
>> --- a/arch/x86/include/asm/smp.h
>> +++ b/arch/x86/include/asm/smp.h
>> @@ -6,7 +6,7 @@
>>  
>>  #include <asm/cpumask.h>
>>  #include <asm/current.h>
>> -#include <asm/thread_info.h>
>> +#include <linux/thread_info.h>
> linux/ namespace headers come before asm/ ones, I'd say.

Oops, meant to move it but forgot, thanks!

> But, more importantly, why is this 4 patches instead of 2:
>
> 1. Remove unused __set_memory_prot
> 2. Fixup include hell

Totally fine by me. I wasn't sure that touching
drivers/virt/coco/sev-guest in the same patch as arch/x86 was ok, but
sounds like it is.

- Kevin

[PATCH 2/4] x86/sev: Explicitly include <linux/mm.h>

[Kevin Brodsky]

sev-guest.c relies on <asm/set_memory.h> including <linux/mm.h>,
but that include is about to go away. Explicitly include
<linux/mm.h> not to run into troubles.

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index b699771be029..e134bee818fa 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -23,6 +23,7 @@
 #include <linux/cleanup.h>
 #include <linux/uuid.h>
 #include <linux/configfs.h>
+#include <linux/mm.h>
 #include <uapi/linux/sev-guest.h>
 #include <uapi/linux/psp-sev.h>
 
-- 
2.47.0

[PATCH 3/4] x86/mm: Remove unused __set_memory_prot()

[Kevin Brodsky]

__set_memory_prot() is unused since commit 5c11f00b09c1 ("x86:
remove memory hotplug support on X86_32"). Let's remove it.

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 arch/x86/include/asm/set_memory.h |  1 -
 arch/x86/mm/pat/set_memory.c      | 13 -------------
 2 files changed, 14 deletions(-)

diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
index cc62ef70ccc0..6586d533fe3a 100644
--- a/arch/x86/include/asm/set_memory.h
+++ b/arch/x86/include/asm/set_memory.h
@@ -38,7 +38,6 @@ int set_memory_rox(unsigned long addr, int numpages);
  * The caller is required to take care of these.
  */
 
-int __set_memory_prot(unsigned long addr, int numpages, pgprot_t prot);
 int _set_memory_uc(unsigned long addr, int numpages);
 int _set_memory_wc(unsigned long addr, int numpages);
 int _set_memory_wt(unsigned long addr, int numpages);
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 95bc50a8541c..2dc1145cfdb8 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -1944,19 +1944,6 @@ static inline int cpa_clear_pages_array(struct page **pages, int numpages,
 		CPA_PAGES_ARRAY, pages);
 }
 
-/*
- * __set_memory_prot is an internal helper for callers that have been passed
- * a pgprot_t value from upper layers and a reservation has already been taken.
- * If you want to set the pgprot to a specific page protocol, use the
- * set_memory_xx() functions.
- */
-int __set_memory_prot(unsigned long addr, int numpages, pgprot_t prot)
-{
-	return change_page_attr_set_clr(&addr, numpages, prot,
-					__pgprot(~pgprot_val(prot)), 0, 0,
-					NULL);
-}
-
 int _set_memory_uc(unsigned long addr, int numpages)
 {
 	/*
-- 
2.47.0

[PATCH 4/4] x86/mm: Remove unnecessary include in set_memory.h

[Kevin Brodsky]

Commit 03b122da74b2 ("x86/sgx: Hook arch_memory_failure() into
mainline code") included <linux/mm.h> in asm/set_memory.h to provide
some helper. However commit b3fdf9398a16 ("x86/mce: relocate
set{clear}_mce_nospec() functions") moved the inline definitions
someplace else, and now set_memory.h just declares a bunch of
functions.

No need for the whole linux/mm.h for declaring functions; just
remove that include. This helps avoid circular dependency headaches
(e.g. if linux/mm.h ends up including <linux/set_memory.h>).

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 arch/x86/include/asm/set_memory.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
index 6586d533fe3a..8d9f1c9aaa4c 100644
--- a/arch/x86/include/asm/set_memory.h
+++ b/arch/x86/include/asm/set_memory.h
@@ -2,7 +2,6 @@
 #ifndef _ASM_X86_SET_MEMORY_H
 #define _ASM_X86_SET_MEMORY_H
 
-#include <linux/mm.h>
 #include <asm/page.h>
 #include <asm-generic/set_memory.h>
 
-- 
2.47.0

Re: [PATCH 0/4] Remove problematic include in <asm/set_memory.h>

[David Hildenbrand]

On 10.12.24 19:46, Kevin Brodsky wrote:
> The need for this series arose from a completely unrelated series [1].
> Long story short, that series causes <linux/mm.h> to include
> <linux/set_memory.h>, which doesn't feel too unreasonable.
> 
> That works fine on arm64 and probably most other architectures, but not
> on x86 [2]: <asm/set_memory.h> itself includes <linux/mm.h>, creating a
> circular dependency.
> 
> set_memory.h doesn't really need <linux/mm.h>, so removing that include
> seems like the right thing to do. That turned out to be a little more
> involved than expected, hence this series:
> 
> * Patch 1-2 ensure that code doesn't rely on <asm/set_memory.h>
>    including <linux/mm.h>. The errors that these patches fix are included
>    at the end of this email.
> 
> * Patch 3 removes an unused function whose declaration relied on
>    <linux/mm.h> being included.
>   
> * Patch 4 actually remove the include.
> 
> I've build-tested this series with x86_64_defconfig and allyesconfig.
> 
> - Kevin
> 
> [1] https://lore.kernel.org/linux-hardening/20241206101110.1646108-1-kevin.brodsky@arm.com/
> [2] https://lore.kernel.org/oe-kbuild-all/202412062006.C23V9ESs-lkp@intel.com/
> 
> Cc: bp@alien8.de
> Cc: dan.j.williams@intel.com
> Cc: dave.hansen@linux.intel.com
> Cc: david@redhat.com
> Cc: jane.chu@oracle.com
> Cc: osalvador@suse.de
> Cc: tglx@linutronix.de
> ---

LGTM

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH 0/4] Remove problematic include in <asm/set_memory.h>

[Christoph Hellwig]

On Tue, Dec 10, 2024 at 06:46:06PM +0000, Kevin Brodsky wrote:
> The need for this series arose from a completely unrelated series [1].
> Long story short, that series causes <linux/mm.h> to include
> <linux/set_memory.h>, which doesn't feel too unreasonable.

It is entirely unreasoable. <linux/mm.h> is inclued just about
everywhere and should not grow more fringe includes.

That btw doesn't invalidate any reason to not also reduce the amount
of crap included in <asm/set_memory.h>, as reducing include hell is
always good.

Re: [PATCH 0/4] Remove problematic include in <asm/set_memory.h>

[Kevin Brodsky]

On 11/12/2024 06:04, Christoph Hellwig wrote:
> On Tue, Dec 10, 2024 at 06:46:06PM +0000, Kevin Brodsky wrote:
>> The need for this series arose from a completely unrelated series [1].
>> Long story short, that series causes <linux/mm.h> to include
>> <linux/set_memory.h>, which doesn't feel too unreasonable.
> It is entirely unreasoable. <linux/mm.h> is inclued just about
> everywhere and should not grow more fringe includes.

Understood, I did wonder about that. Then I suppose the best course of
action for the problematic patch in that other series [3] is to move
pagetable_{alloc,free} out of linux/mm.h.

- Kevin

[3]
https://lore.kernel.org/linux-hardening/20241206101110.1646108-11-kevin.brodsky@arm.com/

Re: [PATCH 0/4] Remove problematic include in <asm/set_memory.h>

[Christoph Hellwig]

On Wed, Dec 11, 2024 at 09:14:07AM +0100, Kevin Brodsky wrote:
> On 11/12/2024 06:04, Christoph Hellwig wrote:
> > On Tue, Dec 10, 2024 at 06:46:06PM +0000, Kevin Brodsky wrote:
> >> The need for this series arose from a completely unrelated series [1].
> >> Long story short, that series causes <linux/mm.h> to include
> >> <linux/set_memory.h>, which doesn't feel too unreasonable.
> > It is entirely unreasoable. <linux/mm.h> is inclued just about
> > everywhere and should not grow more fringe includes.
> 
> Understood, I did wonder about that. Then I suppose the best course of
> action for the problematic patch in that other series [3] is to move
> pagetable_{alloc,free} out of linux/mm.h.

Yes, adding a new header for the page table helpers only used by arch
code is probably a good idea. Or maybe they can actually fit into
asm-generic/pgalloc.h?