Skip to contentSkip to conversation

FAILED: patch "[PATCH] mm: refactor map_deny_write_exec()" failed to apply to 6.6-stable tree

PATCH In review [email protected] 2 participants 8 messages Nov 11, 2024 – Nov 15, 2024

Subsystems (3)
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 6.6-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.6.y
git checkout FETCH_HEAD
git cherry-pick -x 0fb4a7ad270b3b209e510eb9dc5b07bf02b7edaf
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2024111110-dubbed-hydration-c1be@gregkh' --subject-prefix 'PATCH 6.6.y' HEAD^..

Possible dependencies:

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0fb4a7ad270b3b209e510eb9dc5b07bf02b7edaf Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <[email protected]>
Date: Tue, 29 Oct 2024 18:11:46 +0000
Subject: [PATCH] mm: refactor map_deny_write_exec()

Refactor the map_deny_write_exec() to not unnecessarily require a VMA parameter but rather to accept VMA flags parameters, which allows us to use this function early in mmap_region() in a subsequent commit.

While we're here, we refactor the function to be more readable and add some additional documentation.

Fixes deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
Cc Andreas Larsson <[email protected]>
Cc Catalin Marinas <[email protected]>
Cc David S. Miller <[email protected]>
Cc Helge Deller <[email protected]>
Cc James E.J. Bottomley <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Mark Brown <[email protected]>
Cc Peter Xu <[email protected]>
Cc Will Deacon <[email protected]>
include/linux/mman.h +18 -3
index bcb201ab7a41..8ddca62d6460 100644
@@ -188,16 +188,31 @@ static inline bool arch_memory_deny_write_exec_supported(void)
  *
  *	d)	mmap(PROT_READ | PROT_EXEC)
  *		mmap(PROT_READ | PROT_EXEC | PROT_BTI)
+ *
+ * This is only applicable if the user has set the Memory-Deny-Write-Execute
+ * (MDWE) protection mask for the current process.
+ *
+ * @old specifies the VMA flags the VMA originally possessed, and @new the ones
+ * we propose to set.
+ *
+ * Return: false if proposed change is OK, true if not ok and should be denied.
  */
-static inline bool map_deny_write_exec(struct vm_area_struct *vma,  unsigned long vm_flags)
+static inline bool map_deny_write_exec(unsigned long old, unsigned long new)
 {
+	/* If MDWE is disabled, we have nothing to deny. */
 	if (!test_bit(MMF_HAS_MDWE, &current->mm->flags))
 		return false;
 
-	if ((vm_flags & VM_EXEC) && (vm_flags & VM_WRITE))
+	/* If the new VMA is not executable, we have nothing to deny. */
+	if (!(new & VM_EXEC))
+		return false;
+
+	/* Under MDWE we do not accept newly writably executable VMAs... */
+	if (new & VM_WRITE)
 		return true;
 
-	if (!(vma->vm_flags & VM_EXEC) && (vm_flags & VM_EXEC))
+	/* ...nor previously non-executable VMAs becoming executable. */
+	if (!(old & VM_EXEC))
 		return true;
 
 	return false;
mm/mmap.c +1 -1
index ac0604f146f6..ab71d4c3464c 100644
@@ -1505,7 +1505,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 		vma_set_anonymous(vma);
 	}
 
-	if (map_deny_write_exec(vma, vma->vm_flags)) {
+	if (map_deny_write_exec(vma->vm_flags, vma->vm_flags)) {
 		error = -EACCES;
 		goto close_and_free_vma;
 	}
mm/mprotect.c +1 -1
index 0c5d6d06107d..6f450af3252e 100644
@@ -810,7 +810,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
 			break;
 		}
 
-		if (map_deny_write_exec(vma, newflags)) {
+		if (map_deny_write_exec(vma->vm_flags, newflags)) {
 			error = -EACCES;
 			break;
 		}
mm/vma.h +1 -1
index 75558b5e9c8c..d58068c0ff2e 100644
@@ -42,7 +42,7 @@ struct vma_munmap_struct {
 	int vma_count;                  /* Number of vmas that will be removed */
 	bool unlock;                    /* Unlock after the munmap */
 	bool clear_ptes;                /* If there are outstanding PTE to be cleared */
-	/* 1 byte hole */
+	/* 2 byte hole */
 	unsigned long nr_pages;         /* Number of pages being removed */
 	unsigned long locked_vm;        /* Number of locked pages */
 	unsigned long nr_accounted;     /* Number of VM_ACCOUNT pages */
#1 lore raw

PATCH 1/1 mm: refactor map_deny_write_exec()

Refactor the map_deny_write_exec() to not unnecessarily require a VMA parameter but rather to accept VMA flags parameters, which allows us to use this function early in mmap_region() in a subsequent commit.

While we're here, we refactor the function to be more readable and add some additional documentation.

Fixes deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
Cc stable <[email protected]>
---
 include/linux/mman.h | 21 ++++++++++++++++++---
 mm/mmap.c            |  2 +-
 mm/mprotect.c        |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)
include/linux/mman.h +18 -3
index db4741007bef..651705c2bf47 100644
@@ -187,16 +187,31 @@ static inline bool arch_memory_deny_write_exec_supported(void)
  *
  *	d)	mmap(PROT_READ | PROT_EXEC)
  *		mmap(PROT_READ | PROT_EXEC | PROT_BTI)
+ *
+ * This is only applicable if the user has set the Memory-Deny-Write-Execute
+ * (MDWE) protection mask for the current process.
+ *
+ * @old specifies the VMA flags the VMA originally possessed, and @new the ones
+ * we propose to set.
+ *
+ * Return: false if proposed change is OK, true if not ok and should be denied.
  */
-static inline bool map_deny_write_exec(struct vm_area_struct *vma,  unsigned long vm_flags)
+static inline bool map_deny_write_exec(unsigned long old, unsigned long new)
 {
+	/* If MDWE is disabled, we have nothing to deny. */
 	if (!test_bit(MMF_HAS_MDWE, &current->mm->flags))
 		return false;
 
-	if ((vm_flags & VM_EXEC) && (vm_flags & VM_WRITE))
+	/* If the new VMA is not executable, we have nothing to deny. */
+	if (!(new & VM_EXEC))
+		return false;
+
+	/* Under MDWE we do not accept newly writably executable VMAs... */
+	if (new & VM_WRITE)
 		return true;
 
-	if (!(vma->vm_flags & VM_EXEC) && (vm_flags & VM_EXEC))
+	/* ...nor previously non-executable VMAs becoming executable. */
+	if (!(old & VM_EXEC))
 		return true;
 
 	return false;
mm/mmap.c +1 -1
index 9fefd13640d1..d71ac65563b2 100644
@@ -2826,7 +2826,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 		vma_set_anonymous(vma);
 	}
 
-	if (map_deny_write_exec(vma, vma->vm_flags)) {
+	if (map_deny_write_exec(vma->vm_flags, vma->vm_flags)) {
 		error = -EACCES;
 		goto close_and_free_vma;
 	}
mm/mprotect.c +1 -1
index b94fbb45d5c7..7e870a8c9402 100644
@@ -791,7 +791,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
 			break;
 		}
 
-		if (map_deny_write_exec(vma, newflags)) {
+		if (map_deny_write_exec(vma->vm_flags, newflags)) {
 			error = -EACCES;
 			break;
 		}
Signature · 1 line
2.47.0
author maintainer
#2 lore raw

Re: [PATCH 6.6.y] mm: refactor map_deny_write_exec()

Quoted 14 lines · Lorenzo Stoakes

Refactor the map_deny_write_exec() to not unnecessarily require a VMA parameter but rather to accept VMA flags parameters, which allows us to use this function early in mmap_region() in a subsequent commit.

While we're here, we refactor the function to be more readable and add some additional documentation.

Fixes deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
Cc stable <[email protected]>
Quoted 5 lines
---
 include/linux/mman.h | 21 ++++++++++++++++++---
 mm/mmap.c            |  2 +-
 mm/mprotect.c        |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)

There's no clue here as to what the upstream git id is :(

Also, you sent lots of patches for each branch, but not as a series, so we have no idea what order these go in :(

Can you resend all of these, with the upstream git id in it, and as a patch series, so we know to apply them correctly?

thanks,

greg k-h

#3 lore raw

Re: [PATCH 6.6.y] mm: refactor map_deny_write_exec()

Quoted 21 lines · Greg KH
Quoted 14 lines · Lorenzo Stoakes

Refactor the map_deny_write_exec() to not unnecessarily require a VMA parameter but rather to accept VMA flags parameters, which allows us to use this function early in mmap_region() in a subsequent commit.

While we're here, we refactor the function to be more readable and add some additional documentation.

Fixes deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
Cc stable <[email protected]>
Quoted 5 lines
---
 include/linux/mman.h | 21 ++++++++++++++++++---
 mm/mmap.c            |  2 +-
 mm/mprotect.c        |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)
Quoted 1 line

There's no clue here as to what the upstream git id is :(

It's in-reply-to a mail that literally contains the upstream git id, following the instructions you explicitly gave.

Quoted 3 lines

Also, you sent lots of patches for each branch, but not as a series, so we have no idea what order these go in :(

I did wonder how you'd sort out ordering, but again, I was following your explicit instructions.

Quoted 3 lines

Can you resend all of these, with the upstream git id in it, and as a patch series, so we know to apply them correctly?

I'll do this, but... I do have to say, Greg, each of these patches are in reply to a mail stating something like, for instance this one:

The patch below does not apply to the 6.6-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

(I note the above hand waves mention of including original git commit, but it's unwise to then immediately list explicit commands none of which mention this...)

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.6.y
git checkout FETCH_HEAD
git cherry-pick -x 0fb4a7ad270b3b209e510eb9dc5b07bf02b7edaf
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2024111110-dubbed-hydration-c1be@gregkh' --subject-prefix 'PATCH 6.6.y' HEAD^..

Might I politely suggest changing this or no longer telling people a series of commands to follow that result in 'please redo everything over again'?

Something like prefixing this with 'IF YOU NEED ONLY FIXUP A SINGLE COMMIT YOU CAN DO THE FOLLOWING:'?

Because right now it reads as 'you _must_ follow these instructions' to resolve the issue.

Quoted 4 lines

thanks,

greg k-h

A side note but... I didn't actually want to do these backports this way (as per our conversation prior to submission of original series), but my upstream patches got changed to cc: stable @ vger.kernel.org which triggered the bots, and is why I tried the follow these instructions.

I would otherwise have sent these as series in the first instance. But c'est la vie. Murphy's law dictated this series of events happen instead :(

Thanks, Lorenzo

author maintainer
#4 lore raw

Re: [PATCH 6.6.y] mm: refactor map_deny_write_exec()

Quoted 22 lines · Lorenzo Stoakes
Quoted 21 lines · Greg KH
Quoted 19 lines · Lorenzo Stoakes

Refactor the map_deny_write_exec() to not unnecessarily require a VMA parameter but rather to accept VMA flags parameters, which allows us to use this function early in mmap_region() in a subsequent commit.

While we're here, we refactor the function to be more readable and add some additional documentation.

Fixes deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
Cc stable <[email protected]>
---
 include/linux/mman.h | 21 ++++++++++++++++++---
 mm/mmap.c            |  2 +-
 mm/mprotect.c        |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)
Quoted 4 lines
Quoted 1 line

There's no clue here as to what the upstream git id is :(

It's in-reply-to a mail that literally contains the upstream git id, following the instructions you explicitly gave.

The instructions explicitly give you commands that say to use 'git cherry-pick -x' which adds the commit id :)

Quoted 22 lines
Quoted 2 lines

Also, you sent lots of patches for each branch, but not as a series, so we have no idea what order these go in :(

I did wonder how you'd sort out ordering, but again, I was following your explicit instructions.

Quoted 3 lines

Can you resend all of these, with the upstream git id in it, and as a patch series, so we know to apply them correctly?

I'll do this, but... I do have to say, Greg, each of these patches are in reply to a mail stating something like, for instance this one:

The patch below does not apply to the 6.6-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

(I note the above hand waves mention of including original git commit, but it's unwise to then immediately list explicit commands none of which mention this...)

Quoted 5 lines

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.6.y git checkout FETCH_HEAD
git cherry-pick -x 0fb4a7ad270b3b209e510eb9dc5b07bf02b7edaf

See, -x, I think you forgot that :)

Anyway, this normally works just fine, as whole series of commits that fail are odd and rare. I can guess at ordering, like I do when I take them from Linus's tree (going by original commit dates), but for when you resend a bunch of them, it's much tricker as the original "FAILED" message doesn't show that order.

thanks,

greg k-h

#5 lore raw

Re: [PATCH 6.6.y] mm: refactor map_deny_write_exec()

Quoted 23 lines · Greg KH
Quoted 22 lines · Lorenzo Stoakes
Quoted 21 lines · Greg KH
Quoted 19 lines · Lorenzo Stoakes

Refactor the map_deny_write_exec() to not unnecessarily require a VMA parameter but rather to accept VMA flags parameters, which allows us to use this function early in mmap_region() in a subsequent commit.

While we're here, we refactor the function to be more readable and add some additional documentation.

Fixes deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
Cc stable <[email protected]>
---
 include/linux/mman.h | 21 ++++++++++++++++++---
 mm/mmap.c            |  2 +-
 mm/mprotect.c        |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

There's no clue here as to what the upstream git id is :(

It's in-reply-to a mail that literally contains the upstream git id, following the instructions you explicitly gave.

The instructions explicitly give you commands that say to use 'git cherry-pick -x' which adds the commit id :)

Right yes, missed that, these had to be hand fixed up which is part of it.

Quoted 29 lines
Quoted 23 lines
Quoted 3 lines

Also, you sent lots of patches for each branch, but not as a series, so we have no idea what order these go in :(

I did wonder how you'd sort out ordering, but again, I was following your explicit instructions.

Quoted 3 lines

Can you resend all of these, with the upstream git id in it, and as a patch series, so we know to apply them correctly?

I'll do this, but... I do have to say, Greg, each of these patches are in reply to a mail stating something like, for instance this one:

The patch below does not apply to the 6.6-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

(I note the above hand waves mention of including original git commit, but it's unwise to then immediately list explicit commands none of which mention this...)

Quoted 5 lines

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.6.y git checkout FETCH_HEAD
git cherry-pick -x 0fb4a7ad270b3b209e510eb9dc5b07bf02b7edaf

Quoted 7 lines

See, -x, I think you forgot that :)

Anyway, this normally works just fine, as whole series of commits that fail are odd and rare. I can guess at ordering, like I do when I take them from Linus's tree (going by original commit dates), but for when you resend a bunch of them, it's much tricker as the original "FAILED" message doesn't show that order.

My point stands about rewording this, because I mean - I did what you asked (modulo mistakenly not getting the cherry-picked-by thing) - and it seems you are still unable to apply these.

I would add something about 'if there are a series to be applied, see <link to stable process option 3> or whatever it will be.

Because presumably, even if I had got the upstream commit ID bit right, you'd still have had no clue on ordering right? And we'd be in the same position.

Quoted 4 lines

thanks,

greg k-h

author maintainer
#6 lore raw

Re: [PATCH 6.6.y] mm: refactor map_deny_write_exec()

Quoted 71 lines · Lorenzo Stoakes
Quoted 23 lines · Greg KH
Quoted 22 lines · Lorenzo Stoakes
Quoted 21 lines · Greg KH
Quoted 19 lines · Lorenzo Stoakes

Refactor the map_deny_write_exec() to not unnecessarily require a VMA parameter but rather to accept VMA flags parameters, which allows us to use this function early in mmap_region() in a subsequent commit.

While we're here, we refactor the function to be more readable and add some additional documentation.

Fixes deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails")
Cc stable <[email protected]>
---
 include/linux/mman.h | 21 ++++++++++++++++++---
 mm/mmap.c            |  2 +-
 mm/mprotect.c        |  2 +-
 3 files changed, 20 insertions(+), 5 deletions(-)
Quoted 7 lines
Quoted 4 lines
Quoted 1 line

There's no clue here as to what the upstream git id is :(

It's in-reply-to a mail that literally contains the upstream git id, following the instructions you explicitly gave.

The instructions explicitly give you commands that say to use 'git cherry-pick -x' which adds the commit id :)

Right yes, missed that, these had to be hand fixed up which is part of it.

Quoted 29 lines
Quoted 23 lines
Quoted 3 lines

Also, you sent lots of patches for each branch, but not as a series, so we have no idea what order these go in :(

I did wonder how you'd sort out ordering, but again, I was following your explicit instructions.

Quoted 3 lines

Can you resend all of these, with the upstream git id in it, and as a patch series, so we know to apply them correctly?

I'll do this, but... I do have to say, Greg, each of these patches are in reply to a mail stating something like, for instance this one:

The patch below does not apply to the 6.6-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

(I note the above hand waves mention of including original git commit, but it's unwise to then immediately list explicit commands none of which mention this...)

Quoted 5 lines

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-6.6.y git checkout FETCH_HEAD
git cherry-pick -x 0fb4a7ad270b3b209e510eb9dc5b07bf02b7edaf

Quoted 7 lines

See, -x, I think you forgot that :)

Anyway, this normally works just fine, as whole series of commits that fail are odd and rare. I can guess at ordering, like I do when I take them from Linus's tree (going by original commit dates), but for when you resend a bunch of them, it's much tricker as the original "FAILED" message doesn't show that order.

Quoted 3 lines

My point stands about rewording this, because I mean - I did what you asked (modulo mistakenly not getting the cherry-picked-by thing) - and it seems you are still unable to apply these.

If I had the git ids, I'd take the time to figure it out. As-is, these were going to have to be redone anyway, hence me asking that they be sent in a series. That's all, just trying to make my life easier.

thanks,

greg k-h

#7 lore raw

Re: [PATCH 6.6.y] mm: refactor map_deny_write_exec()

Please ignore all of my series for 5.10.y, 5.15.y, 6.1.y, 6.6.y that I sent yesterday then, and I"ll resend _all_ just to make sure they're (ostensibly) correct.

Some have cherry picked upstream git id's some don't depending on whether I had to (painfully) manually fix up or (painfully) resolve merge conflicts, as I erroneously missed that -x added the (cherry picked...) comment.

This way I can make sure to cc- everybody consistently too rather than rely on the git send-email auto-cc.

Thanks.