| Message ID | 60052659-7b37-cb69-bf9f-1683caa46219@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 27, 2018 at 6:45 PM Eric Sandeen <sandeen@redhat.com> wrote: > > Thus the invalid flag combination of (MAP_SHARED|MAP_PRIVATE) now > passes without error, which is a regression. It's not a regression, it's just new behavior. "regression" doesn't mean "things changed". It means "something broke". What broke? Because if it's some manual page breakage, just fix the manual. That's what "new behavior" is all about. There is nothing that says that "MAP_SHARED_VALIDATE" can't work with just the legacy flags. Because I'd be worried about your patch breaking some actual new user of MAP_SHARED_VALIDATE. Because it's actual *users* of behavior we care about, not some test-suite or manual pages. Linus
On 6/27/18 9:10 PM, Linus Torvalds wrote: > On Wed, Jun 27, 2018 at 6:45 PM Eric Sandeen <sandeen@redhat.com> wrote: >> >> Thus the invalid flag combination of (MAP_SHARED|MAP_PRIVATE) now >> passes without error, which is a regression. > > It's not a regression, it's just new behavior. > > "regression" doesn't mean "things changed". It means "something broke". > > What broke? My commit log perhaps was not clear enough. What broke is that mmap(MAP_SHARED|MAP_PRIVATE) now succeeds without error, whereas before it rightly returned -EINVAL. What behavior should a user expect from a successful mmap(MAP_SHARED|MAP_PRIVATE)? -Eric > Because if it's some manual page breakage, just fix the manual. That's > what "new behavior" is all about. > > There is nothing that says that "MAP_SHARED_VALIDATE" can't work with > just the legacy flags. > > Because I'd be worried about your patch breaking some actual new user > of MAP_SHARED_VALIDATE. > > Because it's actual *users* of behavior we care about, not some > test-suite or manual pages. > > Linus
On Wed, Jun 27, 2018 at 7:17 PM Eric Sandeen <sandeen@sandeen.net> wrote: > > What broke is that mmap(MAP_SHARED|MAP_PRIVATE) now succeeds without error, > whereas before it rightly returned -EINVAL. You're still confusing *behavior* with breakage. Yes. New *behavior* is that MAP_SHARED|MAP_PRIVATE is now a valid thing. It means "MAP_SHARED_VALIDATE". Behavior changed. That's normal. Every single time we add a system call, behavior changes: a system call that used to return -ENOSYS now returns something else. That's not breakage, that's just intentional new behavior. > What behavior should a user expect from a successful mmap(MAP_SHARED|MAP_PRIVATE)? MAP_SHARED|MAP_PRIVATE makes no sense and nobody uses it (because it has always returned an error and never done anything interesting). Nobody uses it, and it used to return an error is *exactly* why it was defined to be MAP_SHARED_VALIDATE. So you should expect MAP_SHARED_VALIDATE behavior - which is MAP_SHARED together with "validate that all the flags are things that we support". Actual BREAKAGE is if some application or user workflow no longer works. Did LibreOffice stop working? That is breakage. And by application, I mean exactly that: a real program. Not some manual-page, and not some test-program that people don't actually rely on, and that just reports on some particular behavior. Because I can write a test program that verifies that system call #335 doesn't exist: #define _GNU_SOURCE #include <unistd.h> #include <sys/syscall.h> #include <errno.h> #include <assert.h> int main(int argc, char **argv) { assert(syscall(335, 0) == -1 && errno == ENOSYS); return 0; } and the next system call we add will break that test program on x86-64. And that's still not a "regression" - it's just a change in behavior. But if firefox no longer runs, because it depended on that system call not existing (or it depended on that MAP_SHARED_VALIDATE returning EINVAL) then it's a regression. See the difference? One case is "we added new behavior". The other case is "we have a regression". Linus
On 6/27/18 9:37 PM, Linus Torvalds wrote: > On Wed, Jun 27, 2018 at 7:17 PM Eric Sandeen <sandeen@sandeen.net> wrote: >> >> What broke is that mmap(MAP_SHARED|MAP_PRIVATE) now succeeds without error, >> whereas before it rightly returned -EINVAL. > > You're still confusing *behavior* with breakage. > > Yes. New *behavior* is that MAP_SHARED|MAP_PRIVATE is now a valid > thing. It means "MAP_SHARED_VALIDATE". > > Behavior changed. That's normal. Every single time we add a system > call, behavior changes: a system call that used to return -ENOSYS now > returns something else. > > That's not breakage, that's just intentional new behavior. *shrug* semantics aside, the new behavior is out there in a public API, so I guess there's nothing to do at this point other than to document the change more clearly. It's true that my patch could possibly break existing users. The man page is clearly wrong at this point, both in terms of the error code section, and the claim that MAP_SHARED and MAP_PRIVATE behave as described in POSIX (because POSIX states that these two flags may not be specified together.) -Eric
diff --git a/mm/mmap.c b/mm/mmap.c index d1eb87ef4b1a..b1dc84466365 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1440,6 +1440,16 @@ unsigned long do_mmap(struct file *file, unsigned long addr, if (!file_mmap_ok(file, inode, pgoff, len)) return -EOVERFLOW; + /* + * MAP_SHARED_VALIDATE is indistinguishable from + * (MAP_SHARED|MAP_PRIVATE) which must return -EINVAL. + * If the flags contain MAP_SHARED_VALIDATE and none of the + * non-legacy flags, the user gets EINVAL. + */ + if (((flags & MAP_SHARED_VALIDATE) == MAP_SHARED_VALIDATE) && + !(flags & ~LEGACY_MAP_MASK)) { + return -EINVAL; + } flags_mask = LEGACY_MAP_MASK | file->f_op->mmap_supported_flags;
mmap(2) says the syscall will return EINVAL if "flags contained neither MAP_PRIVATE or MAP_SHARED, or contained both of these values." ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ However, commit 1c972597 ("mm: introduce MAP_SHARED_VALIDATE ...") introduced a new flag, MAP_SHARED_VALIDATE, with a value of 0x3, which is indistinguishable from (MAP_SHARED|MAP_PRIVATE). Thus the invalid flag combination of (MAP_SHARED|MAP_PRIVATE) now passes without error, which is a regression. I'm not sure of the best way out of this, other than to change the API description to say that MAP_SHARED_VALIDATE is only allowed in combination with "new" flags, and reject it if it's used only with flags contained in LEGACY_MAP_MASK. This will require the mmap(2) manpage to enumerate which flags don't require validation, as well, so the user knows when to use the VALIDATE flag. I'm not super happy with this, because it also means that code which explicitly asks for mmap(MAP_SHARED|MAP_PRIVATE|MAP_SYNC) will also pass, but I'm not sure there's anything to do about that. Reported-by: Zhibin Li <zhibli@redhat.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> ---