| Message ID | 20221018235051.152548-4-zokeefe@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add MADV_COLLAPSE documentation | expand |
On Tue, Oct 18, 2022 at 4:51 PM Zach OKeefe <zokeefe@google.com> wrote: > > From: Zach O'Keefe <zokeefe@google.com> > > The initial commit of process_madvise(2) to man-pages project included > an error, indicating that CAP_SYS_ADMIN capability was required when, in > fact, CAP_SYS_NICE was the required capability. > > The initial commit of process_madvise(2) to Linux, commit ecb8ac8b1f14 > ("mm/madvise: introduce process_madvise() syscall: an external memory > hinting API"), relied on PTRACE_MODE_ATTACH_FSCREDS (see ptrace(2)), > but was amended by commit 96cfe2c0fd23 ("mm/madvise: replace ptrace > attach requirement for process_madvise") which replaced this with a > combination of PTRACE_MODE_READ and CAP_SYS_NICE (PTRACE_MODE_READ to > prevent leaking ASLR metadata and CAP_SYS_NICE for influencing process > performance). > > Correct this in the man-page for process_madvise(2). Thanks for fixing my mistake! > > Fixes: a144f458b ("process_madvise.2: Document process_madvise(2)") > Cc: Suren Baghdasaryan <surenb@google.com> > Cc: Minchan Kim <minchan@kernel.org> > Signed-off-by: Zach O'Keefe <zokeefe@google.com> Reviewed-by: Suren Baghdasaryan <surenb@google.com> > --- > man2/process_madvise.2 | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/man2/process_madvise.2 b/man2/process_madvise.2 > index 6208206e4..7bee1a098 100644 > --- a/man2/process_madvise.2 > +++ b/man2/process_madvise.2 > @@ -113,7 +113,8 @@ check (see > in addition, > because of the performance implications of applying the advice, > the caller must have the > -.B CAP_SYS_ADMIN > +.\" commit 96cfe2c0fd23ea7c2368d14f769d287e7ae1082e > +.B CAP_SYS_NICE > capability. > .SH RETURN VALUE > On success, > -- > 2.38.0.413.g74048e4d9e-goog >
Hi Zach! Thanks for the work! :-) On 10/19/22 01:50, Zach OKeefe wrote: > From: Zach O'Keefe <zokeefe@google.com> > > The initial commit of process_madvise(2) to man-pages project included > an error, indicating that CAP_SYS_ADMIN capability was required when, in > fact, CAP_SYS_NICE was the required capability. > > The initial commit of process_madvise(2) to Linux, commit ecb8ac8b1f14 > ("mm/madvise: introduce process_madvise() syscall: an external memory > hinting API"), relied on PTRACE_MODE_ATTACH_FSCREDS (see ptrace(2)), > but was amended by commit 96cfe2c0fd23 ("mm/madvise: replace ptrace > attach requirement for process_madvise") which replaced this with a > combination of PTRACE_MODE_READ and CAP_SYS_NICE (PTRACE_MODE_READ to > prevent leaking ASLR metadata and CAP_SYS_NICE for influencing process > performance). Those two commits are several versions apart: alx@asus5775:~/src/linux/linux$ git describe --contains ecb8ac8b1f14 v5.10-rc1~87^2~14 alx@asus5775:~/src/linux/linux$ git describe --contains 96cfe2c0fd23 v5.12-rc3~12^2~9 If I understand the paragraph above, from 5.10 to 5.12 the capability required was CAP_SYS_ADMIN? Cheers, Alex > > Correct this in the man-page for process_madvise(2). > > Fixes: a144f458b ("process_madvise.2: Document process_madvise(2)") > Cc: Suren Baghdasaryan <surenb@google.com> > Cc: Minchan Kim <minchan@kernel.org> > Signed-off-by: Zach O'Keefe <zokeefe@google.com> > --- > man2/process_madvise.2 | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/man2/process_madvise.2 b/man2/process_madvise.2 > index 6208206e4..7bee1a098 100644 > --- a/man2/process_madvise.2 > +++ b/man2/process_madvise.2 > @@ -113,7 +113,8 @@ check (see > in addition, > because of the performance implications of applying the advice, > the caller must have the > -.B CAP_SYS_ADMIN > +.\" commit 96cfe2c0fd23ea7c2368d14f769d287e7ae1082e > +.B CAP_SYS_NICE > capability. > .SH RETURN VALUE > On success,
On 10/21/22 14:37, Alejandro Colomar wrote: > On 10/19/22 01:50, Zach OKeefe wrote: >> From: Zach O'Keefe <zokeefe@google.com> >> >> The initial commit of process_madvise(2) to man-pages project included >> an error, indicating that CAP_SYS_ADMIN capability was required when, in >> fact, CAP_SYS_NICE was the required capability. >> >> The initial commit of process_madvise(2) to Linux, commit ecb8ac8b1f14 >> ("mm/madvise: introduce process_madvise() syscall: an external memory >> hinting API"), relied on PTRACE_MODE_ATTACH_FSCREDS (see ptrace(2)), >> but was amended by commit 96cfe2c0fd23 ("mm/madvise: replace ptrace >> attach requirement for process_madvise") which replaced this with a >> combination of PTRACE_MODE_READ and CAP_SYS_NICE (PTRACE_MODE_READ to >> prevent leaking ASLR metadata and CAP_SYS_NICE for influencing process >> performance). [...] > If I understand the paragraph above, from 5.10 to 5.12 the capability required > was CAP_SYS_ADMIN? Or was it CAP_SYS_PTRACE?
Hey Alex! Thanks for taking the time to review! On Fri, Oct 21, 2022 at 5:41 AM Alejandro Colomar <alx.manpages@gmail.com> wrote: > > On 10/21/22 14:37, Alejandro Colomar wrote: > > On 10/19/22 01:50, Zach OKeefe wrote: > >> From: Zach O'Keefe <zokeefe@google.com> > >> > >> The initial commit of process_madvise(2) to man-pages project included > >> an error, indicating that CAP_SYS_ADMIN capability was required when, in > >> fact, CAP_SYS_NICE was the required capability. > >> > >> The initial commit of process_madvise(2) to Linux, commit ecb8ac8b1f14 > >> ("mm/madvise: introduce process_madvise() syscall: an external memory > >> hinting API"), relied on PTRACE_MODE_ATTACH_FSCREDS (see ptrace(2)), > >> but was amended by commit 96cfe2c0fd23 ("mm/madvise: replace ptrace > >> attach requirement for process_madvise") which replaced this with a > >> combination of PTRACE_MODE_READ and CAP_SYS_NICE (PTRACE_MODE_READ to > >> prevent leaking ASLR metadata and CAP_SYS_NICE for influencing process > >> performance). > > [...] > > > If I understand the paragraph above, from 5.10 to 5.12 the capability required > > was CAP_SYS_ADMIN? > > Or was it CAP_SYS_PTRACE? Starting in 5.10, there was no CAP_* capability requirement - only PTRACE_MODE_ATTACH_FSCREDS (aka PTRACE_MODE_ATTACH | PTRACE_MODE_REALCREDS). Now, my understanding of the algorithm employed for ptrace access mode checking isn't to be trusted, but AFAIK, a caller having CAP_SYS_PTRACE in the target's user namespace (directly or transitively) isn't required to pass this (though it makes it easier). ptrace(2) has an overview of the algorithm. Starting in 5.12, CAP_SYS_NICE was added as a requirement, and the ptrace algorithm used changed to PTRACE_MODE_READ. If you think recording the differences in kernel versions in the man-page is important, let me know and I can amend this patch. Thanks, Zcah > -- > <http://www.alejandro-colomar.es/>
Hey Zach, On 10/21/22 18:16, Zach O'Keefe wrote: > Hey Alex! > > Thanks for taking the time to review! > > On Fri, Oct 21, 2022 at 5:41 AM Alejandro Colomar > <alx.manpages@gmail.com> wrote: >> >> On 10/21/22 14:37, Alejandro Colomar wrote: >>> On 10/19/22 01:50, Zach OKeefe wrote: >>>> From: Zach O'Keefe <zokeefe@google.com> >>>> >>>> The initial commit of process_madvise(2) to man-pages project included >>>> an error, indicating that CAP_SYS_ADMIN capability was required when, in >>>> fact, CAP_SYS_NICE was the required capability. >>>> >>>> The initial commit of process_madvise(2) to Linux, commit ecb8ac8b1f14 >>>> ("mm/madvise: introduce process_madvise() syscall: an external memory >>>> hinting API"), relied on PTRACE_MODE_ATTACH_FSCREDS (see ptrace(2)), >>>> but was amended by commit 96cfe2c0fd23 ("mm/madvise: replace ptrace >>>> attach requirement for process_madvise") which replaced this with a >>>> combination of PTRACE_MODE_READ and CAP_SYS_NICE (PTRACE_MODE_READ to >>>> prevent leaking ASLR metadata and CAP_SYS_NICE for influencing process >>>> performance). >> >> [...] >> >>> If I understand the paragraph above, from 5.10 to 5.12 the capability required >>> was CAP_SYS_ADMIN? >> >> Or was it CAP_SYS_PTRACE? > > Starting in 5.10, there was no CAP_* capability requirement - only > PTRACE_MODE_ATTACH_FSCREDS (aka PTRACE_MODE_ATTACH | > PTRACE_MODE_REALCREDS). Now, my understanding of the algorithm > employed for ptrace access mode checking isn't to be trusted, but > AFAIK, a caller having CAP_SYS_PTRACE in the target's user namespace > (directly or transitively) isn't required to pass this (though it > makes it easier). ptrace(2) has an overview of the algorithm. > > Starting in 5.12, CAP_SYS_NICE was added as a requirement, and the > ptrace algorithm used changed to PTRACE_MODE_READ. Understood. > > If you think recording the differences in kernel versions in the > man-page is important, let me know and I can amend this patch. Yes; since it was live during 2 versions, I think we should at least mention it. A couple of lines in NOTES might be enough. Thanks, Alex > > Thanks, > Zcah > >> -- >> <http://www.alejandro-colomar.es/>
diff --git a/man2/process_madvise.2 b/man2/process_madvise.2 index 6208206e4..7bee1a098 100644 --- a/man2/process_madvise.2 +++ b/man2/process_madvise.2 @@ -113,7 +113,8 @@ check (see in addition, because of the performance implications of applying the advice, the caller must have the -.B CAP_SYS_ADMIN +.\" commit 96cfe2c0fd23ea7c2368d14f769d287e7ae1082e +.B CAP_SYS_NICE capability. .SH RETURN VALUE On success,