On 2014-04-02 07:45, Steven Whitehouse wrote:
> Hi,
>
> On Wed, 2014-04-02 at 04:10 -0700, Christoph Hellwig wrote:
>> On Tue, Apr 01, 2014 at 02:25:45PM -0400, Richard Hansen wrote:
>>> For the flags parameter, POSIX says "Either MS_ASYNC or MS_SYNC shall
>>> be specified, but not both." [1] There was already a test for the
>>> "both" condition. Add a test to ensure that the caller specified one
>>> of the flags; fail with EINVAL if neither are specified.
>>
>> This breaks various (sloppy) existing userspace
Agreed, but this shouldn't be a strong consideration. The kernel should
let userspace apps worry about their own bugs, not provide crutches.
>> for no gain.
I disagree. Here is what we gain from this patch (expanded from my
previous email):
* Clearer intentions. Looking at the existing code and the code
history, the fact that flags=0 behaves like flags=MS_ASYNC appears
to be a coincidence, not the result of an intentional choice.
* Clearer semantics. What does it mean for msync() to be neither
synchronous nor asynchronous?
* Met expectations. An average reader of the POSIX spec or the
Linux man page would expect msync() to fail if neither flag is
specified.
* Defense against potential future security vulnerabilities. By
explicitly requiring one of the flags, a future change to msync()
is less likely to expose an unintended code path to userspace.
* flags=0 is reserved. By making it illegal to omit both flags
we have the option of making it legal in the future for some
expanded purpose. (Unlikely, but still.)
* Forced app portability. Other operating systems (e.g., NetBSD)
enforce POSIX, so an app developer using Linux might not notice the
non-conformance. This is really the app developer's problem, not
the kernel's, but it's worth considering given msync()'s behavior
is currently unspecified.
Here is a link to a discussion on the bup mailing list about
msync() portability. This is the conversation that motivated this
patch.
http://article.gmane.org/gmane.comp.sysutils.backup.bup/3005
Alternatives:
* Do nothing. Leave the behavior of flags=0 unspecified and let
sloppy userspace continue to be sloppy. Easiest, but the intended
behavior remains unclear and it risks unintended behavior changes
the next time msync() is overhauled.
* Leave msync()'s current behavior alone, but document that MS_ASYNC
is the default if neither is specified. This is backward-
compatible with sloppy userspace, but encourages non-portable uses
of msync() and would preclude using flags=0 for some other future
purpose.
* Change the default to MS_SYNC and document this. This is perhaps
the most conservative option, but it alters the behavior of existing
sloppy userspace and also has the disadvantages of the previous
alternative.
Overall, I believe the advantages of this patch outweigh the
disadvantages, given the alternatives.
Perhaps I should include the above bullets in the commit message.
>>
>> NAK.
>>
> Agreed. It might be better to have something like:
>
> if (flags == 0)
> flags = MS_SYNC;
>
> That way applications which don't set the flags (and possibly also don't
> check the return value, so will not notice an error return) will get the
> sync they desire. Not that either of those things is desirable, but at
> least we can make the best of the situation. Probably better to be slow
> than to potentially lose someone's data in this case,
This is a conservative alternative, but I'd rather not condone flags=0.
Other than compatibility with broken apps, there is little value in
supporting flags=0. Portable apps will have to specify one of the flags
anyway, and the behavior of flags=0 is already accessible via other means.
Thanks,
Richard
>
> Steve.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>