On 16.11.22 19:16, Linus Torvalds wrote:
On Wed, Nov 16, 2022 at 2:30 AM David Hildenbrand <david@xxxxxxxxxx> 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
|