From: Suren Baghdasaryan <firstname.lastname@example.org> To: "Michael Kerrisk (man-pages)" <email@example.com> Cc: linux-man <firstname.lastname@example.org>, "Andrew Morton" <email@example.com>, "Jann Horn" <firstname.lastname@example.org>, "Kees Cook" <email@example.com>, "Jeffrey Vander Stoep" <firstname.lastname@example.org>, "Minchan Kim" <email@example.com>, "Michal Hocko" <firstname.lastname@example.org>, "Shakeel Butt" <email@example.com>, "David Rientjes" <firstname.lastname@example.org>, "Edgar Arriaga García" <email@example.com>, "Tim Murray" <firstname.lastname@example.org>, Linux-MM <email@example.com>, "SElinux list" <firstname.lastname@example.org>, linux-security-module <email@example.com>, "Linux API" <firstname.lastname@example.org>, lkml <email@example.com>, "Android Kernel Team" <firstname.lastname@example.org> Subject: Re: [PATCH 1/1] process_madvise.2: Add process_madvise man page Date: Thu, 28 Jan 2021 23:15:04 -0800 [thread overview] Message-ID: <CAJuCfpE+g96MW+x9A9M0PT_a6-FDtJNFnx6mk9cW3JkZ-SDjvw@mail.gmail.com> (raw) In-Reply-To: <email@example.com> On Thu, Jan 28, 2021 at 12:31 PM Michael Kerrisk (man-pages) <firstname.lastname@example.org> wrote: > > Hello Suren, > > On 1/28/21 7:40 PM, Suren Baghdasaryan wrote: > > On Thu, Jan 28, 2021 at 4:24 AM Michael Kerrisk (man-pages) > > <email@example.com> wrote: > >> > >> Hello Suren, > >> > >> Thank you for writing this page! Some comments below. > > > > Thanks for the review! > > Couple questions below and I'll respin the new version once they are clarified. > > Okay. See below. > > >> On Wed, 20 Jan 2021 at 21:36, Suren Baghdasaryan <firstname.lastname@example.org> wrote: > >>> > > [...] > > Thanks for all the acks. That let's me know that you saw what I said. > > >>> RETURN VALUE > >>> On success, process_madvise() returns the number of bytes advised. This > >>> return value may be less than the total number of requested bytes, if an > >>> error occurred. The caller should check return value to determine whether > >>> a partial advice occurred. > >> > >> So there are three return values possible, > > > > Ok, I think I see your point. How about this instead: > > Well, I'm glad you saw it, because I forgot to finish it. But yes, > you understood what I forgot to say. > > > RETURN VALUE > > On success, process_madvise() returns the number of bytes advised. This > > return value may be less than the total number of requested bytes, if an > > error occurred after some iovec elements were already processed. The caller > > should check the return value to determine whether a partial > > advice occurred. > > > > On error, -1 is returned and errno is set appropriately. > > We recently standardized some wording here: > s/appropriately/to indicate the error/. > > > >>> +.PP > >>> +The pointer > >>> +.I iovec > >>> +points to an array of iovec structures, defined in > >> > >> "iovec" should be formatted as > >> > >> .I iovec > > > > I think it is formatted that way above. What am I missing? > > But also in "an array of iovec structures"... > > > BTW, where should I be using .I vs .IR? I was looking for an answer > > but could not find it. > > .B / .I == bold/italic this line > .BR / .IR == alternate bold/italic with normal (Roman) font. > > So: > .I iovec > .I iovec , # so that comma is not italic > .BR process_madvise () > etc. > > [...] > > >>> +.I iovec > >>> +if one of its elements points to an invalid memory > >>> +region in the remote process. No further elements will be > >>> +processed beyond that point. > >>> +.PP > >>> +Permission to provide a hint to external process is governed by a > >>> +ptrace access mode > >>> +.B PTRACE_MODE_READ_REALCREDS > >>> +check; see > >>> +.BR ptrace (2) > >>> +and > >>> +.B CAP_SYS_ADMIN > >>> +capability that caller should have in order to affect performance > >>> +of an external process. > >> > >> The preceding sentence is garbled. Missing words? > > > > Maybe I worded it incorrectly. What I need to say here is that the > > caller should have both PTRACE_MODE_READ_REALCREDS credentials and > > CAP_SYS_ADMIN capability. The first part I shamelessly copy/pasted > > from https://man7.org/linux/man-pages/man2/process_vm_readv.2.html and > > tried adding the second one to it, obviously unsuccessfully. Any > > advice on how to fix that? > > I think you already got pretty close. How about: > > [[ > Permission to provide a hint to another process is governed by a > ptrace access mode > .B PTRACE_MODE_READ_REALCREDS > check (see > BR ptrace (2)); > in addition, the caller must have the > .B CAP_SYS_ADMIN > capability. In V2 I explanded a bit this part to explain why CAP_SYS_ADMIN is needed. There were questions about that during my patch review which adds this requirement (https://lore.kernel.org/patchwork/patch/1363605), so I thought a short explanation would be useful. > ]] > > [...] > > >>> +.TP > >>> +.B ESRCH > >>> +No process with ID > >>> +.I pidfd > >>> +exists. > >> > >> Should this maybe be: > >> [[ > >> The target process does not exist (i.e., it has terminated and > >> been waited on). > >> ]] > >> > >> See pidfd_send_signal(2). > > > > I "borrowed" mine from > > https://man7.org/linux/man-pages/man2/process_vm_readv.2.html but > > either one sounds good to me. Maybe for pidfd_send_signal the wording > > about termination is more important. Anyway, it's up to you. Just let > > me know which one to use. > > I think the pidfd_send_signal(2) wording fits better. > > [...] > > Thanks, > > Michael > > -- > Michael Kerrisk > Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ > Linux/UNIX System Programming Training: http://man7.org/training/
prev parent reply other threads:[~2021-01-29 7:15 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-20 20:23 Suren Baghdasaryan 2021-01-25 13:19 ` Michal Hocko 2021-01-26 0:14 ` Suren Baghdasaryan 2021-01-28 12:24 ` Michael Kerrisk (man-pages) 2021-01-28 18:40 ` Suren Baghdasaryan 2021-01-28 20:31 ` Michael Kerrisk (man-pages) 2021-01-28 20:37 ` Suren Baghdasaryan 2021-01-29 7:15 ` Suren Baghdasaryan [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAJuCfpE+g96MW+x9A9M0PT_a6-FDtJNFnx6mk9cW3JkZ-SDjvw@mail.gmail.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 1/1] process_madvise.2: Add process_madvise man page' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).