Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Fri, Nov 18, 2022 at 12:09:02PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 16, 2022 at 10:16:34AM -0800, Linus Torvalds wrote:
> > Following the history of it is a big of a mess, because there's a
> > number of renamings and re-organizations, but it seems to go back to
> > 2007 and commit b6a2fea39318 ("mm: variable length argument support").
>
> I went back and read parts of the discussions with Ollie, and the
> .force=1 thing just magically appeared one day when we were sending
> work-in-progress patches back and forth without mention of where it came
> from :-/
>
> And I certainly can't remember now..
>
> Looking at it now, I have the same reaction as both you and Kees had, it
> seems entirely superflous. So I'm all for trying to remove it.
Thanks for digging through the history! I've pushed the change to -next:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/execve&id=cd57e443831d8eeb083c7165bce195d886e216d4
--
Kees Cook
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Wed, Nov 16, 2022 at 10:16:34AM -0800, Linus Torvalds wrote:
> Following the history of it is a big of a mess, because there's a
> number of renamings and re-organizations, but it seems to go back to
> 2007 and commit b6a2fea39318 ("mm: variable length argument support").
I went back and read parts of the discussions with Ollie, and the
.force=1 thing just magically appeared one day when we were sending
work-in-progress patches back and forth without mention of where it came
from :-/
And I certainly can't remember now..
Looking at it now, I have the same reaction as both you and Kees had, it
seems entirely superflous. So I'm all for trying to remove it.
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Thu, Nov 17, 2022 at 03:20:01PM -0800, Linus Torvalds wrote: > On Thu, Nov 17, 2022 at 2:58 PM Kees Cook wrote: > > > > Oh, er, why does get_arg_page() even need FOLL_FORCE? This is writing the > > new stack contents to the nascent brpm->vma, which was newly allocated > > with VM_STACK_FLAGS, which an arch can override, but they all appear to > > include > > VM_WRITE | VM_MAYWRITE. > > Yeah, it does seem entirely superfluous. > > It's been there since the very beginning (although in that original > commit b6a2fea39318 it was there as a '1' to the 'force' argument to > get_user_pages()). > > I *think* it can be just removed. But as long as it exists, it should > most definitely not be renamed to FOLL_PTRACE. > > There's a slight worry that it currently hides some other setup issue > that makes it matter, since it's been that way so long, but I can't > see what it is. My test system boots happily with it removed. I'll throw it into -next and see if anything melts... -- Kees Cook
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Thu, Nov 17, 2022 at 2:58 PM Kees Cook wrote: > > Oh, er, why does get_arg_page() even need FOLL_FORCE? This is writing the > new stack contents to the nascent brpm->vma, which was newly allocated > with VM_STACK_FLAGS, which an arch can override, but they all appear to > include > VM_WRITE | VM_MAYWRITE. Yeah, it does seem entirely superfluous. It's been there since the very beginning (although in that original commit b6a2fea39318 it was there as a '1' to the 'force' argument to get_user_pages()). I *think* it can be just removed. But as long as it exists, it should most definitely not be renamed to FOLL_PTRACE. There's a slight worry that it currently hides some other setup issue that makes it matter, since it's been that way so long, but I can't see what it is. Linus
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Wed, Nov 16, 2022 at 10:16:34AM -0800, Linus Torvalds wrote: > There _are_ also small random cases too, like get_cmdline(). Maybe > that counts as ptrace, but the execve() case most definitely does not. Oh, er, why does get_arg_page() even need FOLL_FORCE? This is writing the new stack contents to the nascent brpm->vma, which was newly allocated with VM_STACK_FLAGS, which an arch can override, but they all appear to include VM_WRITE | VM_MAYWRITE. -- Kees Cook
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On 16.11.22 19:16, Linus Torvalds wrote:
On Wed, Nov 16, 2022 at 2:30 AM David Hildenbrand wrote:
Let's make it clearer that functionality provided by FOLL_FORCE is
really only for ptrace access.
I'm not super-happy about this one.
I do understand the "let's rename the bit so that no new user shows up".
And it's true that the main traditional use is ptrace.
But from the patch itself it becomes obvious that no, it's not *just*
ptrace. At least not yet.
It's used for get_arg_page(), which uses it to basically look up (and
install) pages in the newly created VM.
Now, I'm not entirely sure why it even uses FOLL_FORCE, - I think it
might be historical, because the target should always be the new stack
vma.
Following the history of it is a big of a mess, because there's a
number of renamings and re-organizations, but it seems to go back to
2007 and commit b6a2fea39318 ("mm: variable length argument support").
Right.
Before that commit, we kept our own array of "this is the set of pages
that I will install in the new VM". That commit basically just inserts
the pages directly into the VM instead, getting rid of the array size
limitation.
So at a minimum, I think that FOLL_FORCE would need to be removed
before any renaming to FOLL_PTRACE, because that's not some kind of
small random case.
It *might* be as simple as just removing it, but maybe there's some
reason for having it that I don't immediately see.
Right, I have the same feeling. It might just be a copy-and-paste legacy
leftover.
There _are_ also small random cases too, like get_cmdline(). Maybe
that counts as ptrace, but the execve() case most definitely does not.
I agree. I'd suggest moving forward without this (last) patch for now
and figuring out how to further cleanup FOLL_FORCE usage on top.
@Andrew, if you intend to put this into mm-unstable, please drop the
last patch for now.
--
Thanks,
David / dhildenb
Re: [PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
On Wed, Nov 16, 2022 at 2:30 AM David Hildenbrand wrote:
>
> Let's make it clearer that functionality provided by FOLL_FORCE is
> really only for ptrace access.
I'm not super-happy about this one.
I do understand the "let's rename the bit so that no new user shows up".
And it's true that the main traditional use is ptrace.
But from the patch itself it becomes obvious that no, it's not *just*
ptrace. At least not yet.
It's used for get_arg_page(), which uses it to basically look up (and
install) pages in the newly created VM.
Now, I'm not entirely sure why it even uses FOLL_FORCE, - I think it
might be historical, because the target should always be the new stack
vma.
Following the history of it is a big of a mess, because there's a
number of renamings and re-organizations, but it seems to go back to
2007 and commit b6a2fea39318 ("mm: variable length argument support").
Before that commit, we kept our own array of "this is the set of pages
that I will install in the new VM". That commit basically just inserts
the pages directly into the VM instead, getting rid of the array size
limitation.
So at a minimum, I think that FOLL_FORCE would need to be removed
before any renaming to FOLL_PTRACE, because that's not some kind of
small random case.
It *might* be as simple as just removing it, but maybe there's some
reason for having it that I don't immediately see.
There _are_ also small random cases too, like get_cmdline(). Maybe
that counts as ptrace, but the execve() case most definitely does not.
Linus
[PATCH mm-unstable v1 20/20] mm: rename FOLL_FORCE to FOLL_PTRACE
Let's make it clearer that functionality provided by FOLL_FORCE is
really only for ptrace access. Prevent accidental re-use in drivers
by renaming FOLL_FORCE to FOLL_PTRACE:
git grep -l 'FOLL_FORCE' | xargs sed -i 's/FOLL_FORCE/FOLL_PTRACE/g'
In the future, we might want to use a separate set of flags for the
access_vm interface: most FOLL_* flags don't apply and we mostly only
want to pass FOLL_PTRACE and FOLL_WRITE.
Suggested-by: Christoph Hellwig
Cc: Oleg Nesterov
Cc: Richard Henderson
Cc: Ivan Kokshaysky
Cc: Matt Turner
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Thomas Bogendoerfer
Cc: Michael Ellerman
Cc: Nicholas Piggin
Cc: Christophe Leroy
Cc: "David S. Miller"
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc: Dave Hansen
Cc: "H. Peter Anvin"
Cc: Richard Weinberger
Cc: Anton Ivanov
Cc: Johannes Berg
Cc: Eric Biederman
Cc: Kees Cook
Cc: Alexander Viro
Cc: Peter Zijlstra
Cc: Arnaldo Carvalho de Melo
Cc: Mark Rutland
Cc: Alexander Shishkin
Cc: Jiri Olsa
Cc: Namhyung Kim
Cc: Mike Kravetz
Cc: Muchun Song
Cc: Kentaro Takeda
Cc: Tetsuo Handa
Cc: Paul Moore
Cc: James Morris
Cc: "Serge E. Hallyn"
Signed-off-by: David Hildenbrand
---
arch/alpha/kernel/ptrace.c| 6 +++---
arch/arm64/kernel/mte.c | 2 +-
arch/ia64/kernel/ptrace.c | 10 +-
arch/mips/kernel/ptrace32.c | 4 ++--
arch/mips/math-emu/dsemul.c | 2 +-
arch/powerpc/kernel/ptrace/ptrace32.c | 4 ++--
arch/sparc/kernel/ptrace_32.c | 4 ++--
arch/sparc/kernel/ptrace_64.c | 8
arch/x86/kernel/step.c| 2 +-
arch/x86/um/ptrace_32.c | 2 +-
arch/x86/um/ptrace_64.c | 2 +-
fs/exec.c | 2 +-
fs/proc/base.c| 2 +-
include/linux/mm.h| 8
kernel/events/uprobes.c | 4 ++--
kernel/ptrace.c | 12 ++--
mm/gup.c | 28 +--
mm/huge_memory.c | 8
mm/hugetlb.c | 2 +-
mm/memory.c | 4 ++--
mm/util.c | 4 ++--
security/tomoyo/domain.c | 2 +-
22 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/arch/alpha/kernel/ptrace.c b/arch/alpha/kernel/ptrace.c
index a1a239ea002d..55def6479ff2 100644
--- a/arch/alpha/kernel/ptrace.c
+++ b/arch/alpha/kernel/ptrace.c
@@ -158,7 +158,7 @@ static inline int
read_int(struct task_struct *task, unsigned long addr, int * data)
{
int copied = access_process_vm(task, addr, data, sizeof(int),
- FOLL_FORCE);
+ FOLL_PTRACE);
return (copied == sizeof(int)) ? 0 : -EIO;
}
@@ -166,7 +166,7 @@ static inline int
write_int(struct task_struct *task, unsigned long addr, int data)
{
int copied = access_process_vm(task, addr, &data, sizeof(int),
- FOLL_FORCE | FOLL_WRITE);
+ FOLL_PTRACE | FOLL_WRITE);
return (copied == sizeof(int)) ? 0 : -EIO;
}
@@ -284,7 +284,7 @@ long arch_ptrace(struct task_struct *child, long request,
case PTRACE_PEEKTEXT: /* read word at location addr. */
case PTRACE_PEEKDATA:
copied = ptrace_access_vm(child, addr, &tmp, sizeof(tmp),
- FOLL_FORCE);
+ FOLL_PTRACE);
ret = -EIO;
if (copied != sizeof(tmp))
break;
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 7467217c1eaf..fa29fecaedbc 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -525,7 +525,7 @@ int mte_ptrace_copy_tags(struct task_struct *child, long
request,
int ret;
struct iovec kiov;
struct iovec __user *uiov = (void __user *)data;
- unsigned int gup_flags = FOLL_FORCE;
+ unsigned int gup_flags = FOLL_PTRACE;
if (!system_supports_mte())
return -EIO;
diff --git a/arch/ia64/kernel/ptrace.c b/arch/ia64/kernel/ptrace.c
index ab8aeb34d1d9..3781db1f506c 100644
--- a/arch/ia64/kernel/ptrace.c
+++ b/arch/ia64/kernel/ptrace.c
@@ -452,7 +452,7 @@ ia64_peek (struct task_struct *child, struct switch_stack
*child_stack,
return 0;
}
}
- copied = access_process_vm(child, addr, &ret, sizeof(ret), FOLL_FORCE);
+ copied = access_process_vm(child, addr, &ret, sizeof(ret), FOLL_PTRACE);
if (copied != sizeof(ret))
return -EIO;
*val = ret;
@@ -489,7 +489,7 @@ ia64_poke (struct task_struct *child, struct switch_stack
*child_stack,
}
}
} else if (access_process_vm(child, addr, &val, sizeof(val),
- FOLL_FORCE | FOLL_WRITE)
+ F
