Re: [PATCH] mm: msync: require either MS_ASYNC or MS_SYNC

[Richard Hansen <rhansen-A08e6c8yq/Q@public.gmane.org>]

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.