From: Linus Torvalds <torvalds@linux-foundation.org> To: Lorenzo Stoakes <lstoakes@gmail.com> Cc: Rik van Riel <riel@redhat.com>, Hugh Dickins <hughd@google.com>, linux-mm <linux-mm@kvack.org>, Mel Gorman <mgorman@techsingularity.net>, tbsaunde@tbsaunde.org, robert@ocallahan.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org> Subject: Re: [PATCH] mm: check VMA flags to avoid invalid PROT_NONE NUMA balancing Date: Fri, 7 Oct 2016 08:34:15 -0700 [thread overview] Message-ID: <CA+55aFzOYk_1Jcr8CSKyqfkXaOApZvCkX0_27mZk7PvGSE4xSw@mail.gmail.com> (raw) In-Reply-To: <20161007100720.GA14859@lucifer> On Fri, Oct 7, 2016 at 3:07 AM, Lorenzo Stoakes <lstoakes@gmail.com> wrote: > > So I've experimented with this a little locally, removing FOLL_FORCE altogether > and tracking places where it is used (it seems to be a fair few places > actually.) I'm actually a bit worried that it is used too much simply because it's so easy to use. In paticular, there's a class of functions that (for legacy reasons) get the "int force" and "int write" arguments, and they both tend to just use a very non-descript 0/1 in the callers. Then at some point you have code like if (write) flags |= FOLL_WRITE; if (force) flags |= FOLL_FORCE; (I'm paraphrasing from memory), and then subsequent calls use that FOLL_FORCE. And the problem with that it's that it's *really* hard to see where people actually use FOLL_FORCE, and it's also really easy to use it without thinking (because it doesn't say "FOLL_FORCE", it just randomly says "1" in some random argument list). So the *first* step I'd do is to actually get rid of all the "write" and "force" arguments, and just pass in "flags" instead, and use FOLL_FORCE and FOLL_WRITE explicitly in the caller. That way it suddenly becomes *much* easier to grep for FOLL_FORCE and see who it is that actually really wants it. (In fact, I think some functions get *both* "flags" and the separate write/force argument). Would you be willing to look at doing that kind of purely syntactic, non-semantic cleanup first? > I've rather naively replaced the FOLL_FORCE check in check_vma_flags() with a > check against 'tsk && tsk->ptrace && tsk->parent == current', I'm not sure how > valid or sane this is, however, but a quick check against gdb proves that it is > able to do its thing in this configuration. Is this a viable path, or is this > way off the mark here? I think that if we end up having the FOLL_FORCE semantics, we're actually better off having an explicit FOLL_FORCE flag, and *not* do some kind of implicit "under these magical circumstances we'll force it anyway". The implicit thing is what we used to do long long ago, we definitely don't want to. So I'd rather see all the callers that actually use FOLL_FORCE, and then we can start looking at whether it's really a requirement. Some of them may not strictly *need* it in actual use, they just have it because of historical reasons (ie exactly those implicit users that just got mindlessly converted to maintain same semantics). Linus
WARNING: multiple messages have this Message-ID (diff)
From: Linus Torvalds <torvalds@linux-foundation.org> To: Lorenzo Stoakes <lstoakes@gmail.com> Cc: Rik van Riel <riel@redhat.com>, Hugh Dickins <hughd@google.com>, linux-mm <linux-mm@kvack.org>, Mel Gorman <mgorman@techsingularity.net>, tbsaunde@tbsaunde.org, robert@ocallahan.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org> Subject: Re: [PATCH] mm: check VMA flags to avoid invalid PROT_NONE NUMA balancing Date: Fri, 7 Oct 2016 08:34:15 -0700 [thread overview] Message-ID: <CA+55aFzOYk_1Jcr8CSKyqfkXaOApZvCkX0_27mZk7PvGSE4xSw@mail.gmail.com> (raw) In-Reply-To: <20161007100720.GA14859@lucifer> On Fri, Oct 7, 2016 at 3:07 AM, Lorenzo Stoakes <lstoakes@gmail.com> wrote: > > So I've experimented with this a little locally, removing FOLL_FORCE altogether > and tracking places where it is used (it seems to be a fair few places > actually.) I'm actually a bit worried that it is used too much simply because it's so easy to use. In paticular, there's a class of functions that (for legacy reasons) get the "int force" and "int write" arguments, and they both tend to just use a very non-descript 0/1 in the callers. Then at some point you have code like if (write) flags |= FOLL_WRITE; if (force) flags |= FOLL_FORCE; (I'm paraphrasing from memory), and then subsequent calls use that FOLL_FORCE. And the problem with that it's that it's *really* hard to see where people actually use FOLL_FORCE, and it's also really easy to use it without thinking (because it doesn't say "FOLL_FORCE", it just randomly says "1" in some random argument list). So the *first* step I'd do is to actually get rid of all the "write" and "force" arguments, and just pass in "flags" instead, and use FOLL_FORCE and FOLL_WRITE explicitly in the caller. That way it suddenly becomes *much* easier to grep for FOLL_FORCE and see who it is that actually really wants it. (In fact, I think some functions get *both* "flags" and the separate write/force argument). Would you be willing to look at doing that kind of purely syntactic, non-semantic cleanup first? > I've rather naively replaced the FOLL_FORCE check in check_vma_flags() with a > check against 'tsk && tsk->ptrace && tsk->parent == current', I'm not sure how > valid or sane this is, however, but a quick check against gdb proves that it is > able to do its thing in this configuration. Is this a viable path, or is this > way off the mark here? I think that if we end up having the FOLL_FORCE semantics, we're actually better off having an explicit FOLL_FORCE flag, and *not* do some kind of implicit "under these magical circumstances we'll force it anyway". The implicit thing is what we used to do long long ago, we definitely don't want to. So I'd rather see all the callers that actually use FOLL_FORCE, and then we can start looking at whether it's really a requirement. Some of them may not strictly *need* it in actual use, they just have it because of historical reasons (ie exactly those implicit users that just got mindlessly converted to maintain same semantics). Linus -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2016-10-07 15:34 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-09-11 22:54 [PATCH] mm: check VMA flags to avoid invalid PROT_NONE NUMA balancing Lorenzo Stoakes 2016-09-11 22:59 ` Lorenzo Stoakes 2016-09-11 22:59 ` Lorenzo Stoakes 2016-09-25 18:47 ` Lorenzo Stoakes 2016-09-25 18:47 ` Lorenzo Stoakes 2016-09-25 20:52 ` Linus Torvalds 2016-09-25 20:52 ` Linus Torvalds 2016-09-25 22:24 ` Linus Torvalds 2016-09-25 22:24 ` Linus Torvalds 2016-09-25 22:34 ` Rik van Riel 2016-09-25 22:50 ` Linus Torvalds 2016-09-25 22:50 ` Linus Torvalds 2016-09-25 23:28 ` Hugh Dickins 2016-09-25 23:28 ` Hugh Dickins 2016-09-26 0:49 ` Rik van Riel 2016-09-26 1:05 ` Linus Torvalds 2016-09-26 1:05 ` Linus Torvalds 2016-10-07 10:07 ` Lorenzo Stoakes 2016-10-07 10:07 ` Lorenzo Stoakes 2016-10-07 15:34 ` Linus Torvalds [this message] 2016-10-07 15:34 ` Linus Torvalds 2016-10-07 16:22 ` Lorenzo Stoakes 2016-10-07 16:22 ` Lorenzo Stoakes 2016-10-07 18:16 ` Hugh Dickins 2016-10-07 18:16 ` Hugh Dickins 2016-10-07 18:26 ` Lorenzo Stoakes 2016-10-07 18:26 ` Lorenzo Stoakes 2016-10-10 7:47 ` Jan Kara 2016-10-10 7:47 ` Jan Kara 2016-10-10 8:28 ` Lorenzo Stoakes 2016-10-10 8:28 ` Lorenzo Stoakes 2016-10-10 16:37 ` Jan Kara 2016-10-10 16:37 ` Jan Kara 2016-09-26 8:19 ` Mel Gorman
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=CA+55aFzOYk_1Jcr8CSKyqfkXaOApZvCkX0_27mZk7PvGSE4xSw@mail.gmail.com \ --to=torvalds@linux-foundation.org \ --cc=akpm@linux-foundation.org \ --cc=hughd@google.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=lstoakes@gmail.com \ --cc=mgorman@techsingularity.net \ --cc=riel@redhat.com \ --cc=robert@ocallahan.org \ --cc=tbsaunde@tbsaunde.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.