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

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

[Richard Hansen]

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.

Without this change, specifying neither is the same as specifying
flags=MS_ASYNC because nothing in msync() is conditioned on the
MS_ASYNC flag.  This has not always been true, and there's no good
reason to believe that this behavior would have persisted
indefinitely.

The msync(2) man page (as currently written in man-pages.git) is
silent on the behavior if both flags are unset, so this change should
not break an application written by somone who carefully reads the
Linux man pages or the POSIX spec.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/msync.html

Signed-off-by: Richard Hansen <rhansen@bbn.com>
Reported-by: Greg Troxel <gdt@ir.bbn.com>
Reviewed-by: Greg Troxel <gdt@ir.bbn.com>
---

This is a resend of:
http://article.gmane.org/gmane.linux.kernel/1554416
I didn't get any feedback from that submission, so I'm resending it
without changes.

 mm/msync.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/msync.c b/mm/msync.c
index 632df45..472ad3e 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -42,6 +42,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t,
len, int, flags)
 		goto out;
 	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
 		goto out;
+	if (!(flags & (MS_ASYNC | MS_SYNC)))
+		goto out;
 	error = -ENOMEM;
 	len = (len + ~PAGE_MASK) & PAGE_MASK;
 	end = start + len;
-- 
1.8.4

--
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>

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

[Michael Kerrisk (man-pages)]

Richard,

On 04/01/2014 08:25 PM, 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.
> 
> Without this change, specifying neither is the same as specifying
> flags=MS_ASYNC because nothing in msync() is conditioned on the
> MS_ASYNC flag.  This has not always been true, 

I am curious (since such things should be documented)--when was
it not true?

> and there's no good
> reason to believe that this behavior would have persisted
> indefinitely.
> 
> The msync(2) man page (as currently written in man-pages.git) is
> silent on the behavior if both flags are unset, so this change should
> not break an application written by somone who carefully reads the
> Linux man pages or the POSIX spec.

Sadly, people do not always carefully read man pages, so there
remains the chance that a change like this will break applications.
Aside from standards conformance, what do you see as the benefit
of the change?

Thanks,

Michael


> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/msync.html
> 
> Signed-off-by: Richard Hansen <rhansen-A08e6c8yq/Q@public.gmane.org>
> Reported-by: Greg Troxel <gdt-2FjktZCtrC/QT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Greg Troxel <gdt-2FjktZCtrC/QT0dZR+AlfA@public.gmane.org>
> ---
> 
> This is a resend of:
> http://article.gmane.org/gmane.linux.kernel/1554416
> I didn't get any feedback from that submission, so I'm resending it
> without changes.
> 
>  mm/msync.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/msync.c b/mm/msync.c
> index 632df45..472ad3e 100644
> --- a/mm/msync.c
> +++ b/mm/msync.c
> @@ -42,6 +42,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t,
> len, int, flags)
>  		goto out;
>  	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
>  		goto out;
> +	if (!(flags & (MS_ASYNC | MS_SYNC)))
> +		goto out;
>  	error = -ENOMEM;
>  	len = (len + ~PAGE_MASK) & PAGE_MASK;
>  	end = start + len;
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

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

[Richard Hansen]

On 2014-04-01 15:32, Michael Kerrisk (man-pages) wrote:
> Richard,
> 
> On 04/01/2014 08:25 PM, 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.
>>
>> Without this change, specifying neither is the same as specifying
>> flags=MS_ASYNC because nothing in msync() is conditioned on the
>> MS_ASYNC flag.  This has not always been true, 
> 
> I am curious (since such things should be documented)--when was
> it not true?

Before commit 204ec84 [1] (in v2.6.19), specifying MS_ASYNC could
potentially follow a different code path than specifying neither
MS_ASYNC nor MS_SYNC.  I'm not familiar enough with the internals to
know what the behavioral implications were at the time.

[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=204ec841fbea3e5138168edbc3a76d46747cc987

> 
>> and there's no good
>> reason to believe that this behavior would have persisted
>> indefinitely.
>>
>> The msync(2) man page (as currently written in man-pages.git) is
>> silent on the behavior if both flags are unset, so this change should
>> not break an application written by somone who carefully reads the
>> Linux man pages or the POSIX spec.
> 
> Sadly, people do not always carefully read man pages, so there
> remains the chance that a change like this will break applications.

True.  Mitigating factors:  (1) It'll only break applications that only
care about Linux, and (2) any app that does flags=0 is arguably buggy
anyway given the unspecified behavior.

> Aside from standards conformance,

Technically this change isn't required for standards conformance.  The
POSIX standard is OK with implementation extensions, so this issue could
be resolved by simply documenting that if neither MS_ASYNC nor MS_SYNC
are set then MS_ASYNC is implied.  This would preclude us from using
flags=0 for a different purpose in the future, so I'm a bit reluctant to
go this route.

(If we do go this route I'd like to see msync() modified to explicitly
set the MS_ASYNC flag if neither are set to be defensive and to
communicate intention to anyone reading the code.)

> what do you see as the benefit of the change?

  * Clarify intentions.  Looking at the 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.

  * Eliminate unclear semantics.  (What does it mean for msync() to be
    neither synchronous nor asynchronous?)

  * Force 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 the alternatives to this patch are to stay vague
    or to commit to defaulting to MS_ASYNC, neither of which I like as
    much.

    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

Note that in addition to this patch I'd like to update the msync(2) man
page to say that one of the two flags must be specified, but this commit
should go in first.

Thanks,
Richard


> 
> Thanks,
> 
> Michael
> 
> 
>> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/msync.html
>>
>> Signed-off-by: Richard Hansen <rhansen@bbn.com>
>> Reported-by: Greg Troxel <gdt@ir.bbn.com>
>> Reviewed-by: Greg Troxel <gdt@ir.bbn.com>
>> ---
>>
>> This is a resend of:
>> http://article.gmane.org/gmane.linux.kernel/1554416
>> I didn't get any feedback from that submission, so I'm resending it
>> without changes.
>>
>>  mm/msync.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/msync.c b/mm/msync.c
>> index 632df45..472ad3e 100644
>> --- a/mm/msync.c
>> +++ b/mm/msync.c
>> @@ -42,6 +42,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t,
>> len, int, flags)
>>  		goto out;
>>  	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
>>  		goto out;
>> +	if (!(flags & (MS_ASYNC | MS_SYNC)))
>> +		goto out;
>>  	error = -ENOMEM;
>>  	len = (len + ~PAGE_MASK) & PAGE_MASK;
>>  	end = start + len;
>>
> 
> 

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

[chrubis-AlSwsSmVLrQ]

Hi!
> > and there's no good
> > reason to believe that this behavior would have persisted
> > indefinitely.
> > 
> > The msync(2) man page (as currently written in man-pages.git) is
> > silent on the behavior if both flags are unset, so this change should
> > not break an application written by somone who carefully reads the
> > Linux man pages or the POSIX spec.
> 
> Sadly, people do not always carefully read man pages, so there
> remains the chance that a change like this will break applications.
> Aside from standards conformance, what do you see as the benefit
> of the change?

I've looked around Linux Test Project and this change will break a few
testcases, but nothing that couldn't be easily fixed.

The rest of the world may be more problematic though.

-- 
Cyril Hrubis
chrubis-AlSwsSmVLrQ@public.gmane.org

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

[Christoph Hellwig]

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 for no gain.

NAK.

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

[Steven Whitehouse]

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 for no gain.
> 
> 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,

Steve.

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

[Richard Hansen]

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.

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

[Michael Kerrisk (man-pages)]

[CC += Peter Zijlstra]
[CC += bug-readline@gnu.org -- maintainers, it _may_ be desirable to
fix your msync() call]

Richard,

On Thu, Apr 3, 2014 at 1:44 AM, Richard Hansen <rhansen@bbn.com> wrote:
> 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.

Maybe. You earlier asserted that the semantics when flags==0 may have
been different, prior to Peter Zijstra's patch,
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=204ec841fbea3e5138168edbc3a76d46747cc987
.
It's not clear to me that that is the case. But, it would be wise to
CC the developer, in case he has an insight.

>   * 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.

There is no doubt that the situation on Linux is an unfortunate mess
from history (and is far from the only one, see
https://lwn.net/Articles/588444/).

And I think everyone would agree that all of the above would be nice
to have, if there was no cost to having them. But, there is a major
cost: the pain of breaking those sloppy user-space applications. And
in fact some casual grepping suggests that many applications would
break, since, for example, libreadline contains (in histfile.c) an
msync() call that omits both MS_SYNC and MS_ASYNC (I have not looked
into the details of what that piece of code does).

But, even if you could find and fix every application that misuses
msync(), new kernels with your proposed changes would still break old
binaries. Linus has made it clear on numerous occasions that kernel
changes must not break user space. So, the change you suggest is never
going to fly (and Christoph's NAK at least saves Linus yelling at you
;-).)

>     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.

I think the only reasonable solution is to better document existing
behavior and what the programmer should do. With that in mind, I've
drafted the following text for the msync(2) man page:

    NOTES
       According to POSIX, exactly one of MS_SYNC and MS_ASYNC  must  be
       specified  in  flags.   However,  Linux permits a call to msync()
       that specifies neither of these flags, with  semantics  that  are
       (currently)  equivalent  to  specifying  MS_ASYNC.   (Since Linux
       2.6.19, MS_ASYNC is in fact a no-op, since  the  kernel  properly
       tracks  dirty  pages  and  flushes them to storage as necessary.)
       Notwithstanding the Linux behavior, portable, future-proof appli‐
       cations  should  ensure  that they specify exactly one of MS_SYNC
       and MS_ASYNC in flags.

Comments on this draft welcome.

Cheers,

Michael


> 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.

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
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>

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

[Christopher Covington]

On 04/03/2014 04:25 AM, Michael Kerrisk (man-pages) wrote:

> I think the only reasonable solution is to better document existing
> behavior and what the programmer should do. With that in mind, I've
> drafted the following text for the msync(2) man page:
> 
>     NOTES
>        According to POSIX, exactly one of MS_SYNC and MS_ASYNC  must  be
>        specified  in  flags.   However,  Linux permits a call to msync()
>        that specifies neither of these flags, with  semantics  that  are
>        (currently)  equivalent  to  specifying  MS_ASYNC.   (Since Linux
>        2.6.19, MS_ASYNC is in fact a no-op, since  the  kernel  properly
>        tracks  dirty  pages  and  flushes them to storage as necessary.)
>        Notwithstanding the Linux behavior, portable, future-proof appli‐
>        cations  should  ensure  that they specify exactly one of MS_SYNC
>        and MS_ASYNC in flags.

Nit: MS_SYNC or MS_ASYNC

Christopher

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by the Linux Foundation.

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

[Michael Kerrisk (man-pages)]

On 04/03/2014 01:51 PM, Christopher Covington wrote:
> On 04/03/2014 04:25 AM, Michael Kerrisk (man-pages) wrote:
> 
>> I think the only reasonable solution is to better document existing
>> behavior and what the programmer should do. With that in mind, I've
>> drafted the following text for the msync(2) man page:
>>
>>     NOTES
>>        According to POSIX, exactly one of MS_SYNC and MS_ASYNC  must  be
>>        specified  in  flags.   However,  Linux permits a call to msync()
>>        that specifies neither of these flags, with  semantics  that  are
>>        (currently)  equivalent  to  specifying  MS_ASYNC.   (Since Linux
>>        2.6.19, MS_ASYNC is in fact a no-op, since  the  kernel  properly
>>        tracks  dirty  pages  and  flushes them to storage as necessary.)
>>        Notwithstanding the Linux behavior, portable, future-proof appli‐
>>        cations  should  ensure  that they specify exactly one of MS_SYNC
>>        and MS_ASYNC in flags.
> 
> Nit: MS_SYNC or MS_ASYNC

Thanks. Reworded.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
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>

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

[Greg Troxel]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]


"Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:

> I think the only reasonable solution is to better document existing
> behavior and what the programmer should do. With that in mind, I've
> drafted the following text for the msync(2) man page:
>
>     NOTES
>        According to POSIX, exactly one of MS_SYNC and MS_ASYNC  must  be
>        specified  in  flags.   However,  Linux permits a call to msync()
>        that specifies neither of these flags, with  semantics  that  are
>        (currently)  equivalent  to  specifying  MS_ASYNC.   (Since Linux
>        2.6.19, MS_ASYNC is in fact a no-op, since  the  kernel  properly
>        tracks  dirty  pages  and  flushes them to storage as necessary.)
>        Notwithstanding the Linux behavior, portable, future-proof appli‐
>        cations  should  ensure  that they specify exactly one of MS_SYNC
>        and MS_ASYNC in flags.
>
> Comments on this draft welcome.

I think it's a step backwards to document unspecified behavior.  If
anything, the man page should make it clear that providing neither flag
results in undefined behavior and will lead to failure on systems on
than Linux.  While I can see the point of not changing the previous
behavior to protect buggy code, there's no need to document it in the
man page and further enshrine it.

There's a larger point, which is that people write code for Linux when
they should be writing code for POSIX.  Therefore, Linux has an
obligation to the larger free software community to avoid encouraging
non-portable code.  This is somewhat similar (except for the key point
that it's unintentional) to bash's allowing "==" in test, which is a
gratuitous extension to the standard that has led to large amounts of
nonportable code.  To mitigate this, it would be reasonable to syslog a
warning the first time a process makes a call with flags that POSIX says
leads to undefined behavior.  That would meet the
portability-citizenzhip goals and not break existing systems.

[-- Attachment #2: Type: application/pgp-signature, Size: 180 bytes --]

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

[Michael Kerrisk (man-pages)]

Hi Greg,

On 04/03/2014 02:57 PM, Greg Troxel wrote:
> 
> "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> writes:
> 
>> I think the only reasonable solution is to better document existing
>> behavior and what the programmer should do. With that in mind, I've
>> drafted the following text for the msync(2) man page:
>>
>>     NOTES
>>        According to POSIX, exactly one of MS_SYNC and MS_ASYNC  must  be
>>        specified  in  flags.   However,  Linux permits a call to msync()
>>        that specifies neither of these flags, with  semantics  that  are
>>        (currently)  equivalent  to  specifying  MS_ASYNC.   (Since Linux
>>        2.6.19, MS_ASYNC is in fact a no-op, since  the  kernel  properly
>>        tracks  dirty  pages  and  flushes them to storage as necessary.)
>>        Notwithstanding the Linux behavior, portable, future-proof appli‐
>>        cations  should  ensure  that they specify exactly one of MS_SYNC
>>        and MS_ASYNC in flags.
>>
>> Comments on this draft welcome.
> 
> I think it's a step backwards to document unspecified behavior.  If
> anything, the man page should make it clear that providing neither flag
> results in undefined behavior and will lead to failure on systems on
> than Linux.  While I can see the point of not changing the previous
> behavior to protect buggy code, there's no need to document it in the
> man page and further enshrine it.

The Linux behavior is what it is. For the reasons I mentioned already,
it's unlikely to change. And, when the man pages omit to explain what
Linux actually does, there will one day come a request to actually
document the behavior. So, I don't think it's quite enough to say the 
behavior is undefined. On the other hand, it does not hurt to further
expand the portability warning. I made the text now:

    NOTES
       According to POSIX, either MS_SYNC or MS_ASYNC must be  specified
       in  flags, and  indeed failure to include one of these flags will
       cause msync() to fail on some systems.  However, Linux permits  a
       call  to  msync()  that  specifies  neither  of these flags, with
       semantics that are (currently) equivalent to specifying MS_ASYNC.
       (Since  Linux 2.6.19, MS_ASYNC is in fact a no-op, since the ker‐
       nel properly tracks dirty pages and flushes them  to  storage  as
       necessary.)    Notwithstanding   the  Linux  behavior,  portable,
       future-proof applications should ensure that they specify  either
       MS_SYNC or MS_ASYNC in flags.




-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
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>

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

[Richard Hansen]

On 2014-04-03 04:25, Michael Kerrisk (man-pages) wrote:
> [CC += Peter Zijlstra]
> [CC += bug-readline@gnu.org -- maintainers, it _may_ be desirable to
> fix your msync() call]

I didn't see bug-readline@gnu.org in the CC list -- did you forget to
add them, or were they BCC'd?

>>   * 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.
> 
> Maybe. You earlier asserted that the semantics when flags==0 may have
> been different, prior to Peter Zijstra's patch,
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=204ec841fbea3e5138168edbc3a76d46747cc987
> .
> It's not clear to me that that is the case. But, it would be wise to
> CC the developer, in case he has an insight.

Good idea, thanks.

> But, even if you could find and fix every application that misuses
> msync(), new kernels with your proposed changes would still break old
> binaries. Linus has made it clear on numerous occasions that kernel
> changes must not break user space. So, the change you suggest is never
> going to fly (and Christoph's NAK at least saves Linus yelling at you
> ;-).)

OK -- that's a good enough reason for me.

> I think the only reasonable solution is to better document existing
> behavior and what the programmer should do.

Greg mentioned the possibility of syslogging a warning the first time a
process uses msync() with neither flag set.  Another alternative would
be to do this in userspace: modify the {g,u}libc shims to log a warning
to stderr.

And there's yet another alternative that's probably a bad idea but I'll
toss it out anyway:  I'm not very familiar with the Linux kernel, but
the NetBSD kernel defines multiple versions of some syscalls for
backward-compatibility reasons.  A new non-backward-compatible version
of an existing syscall gets a new syscall number.  Programs compiled
against the latest headers use the new version of the syscall but old
binaries still get the old behavior.  I imagine folks would frown upon
doing something like this in Linux for msync() (create a new version
that EINVALs if neither flag is specified), but it would be a way to
migrate toward a portability-friendly behavior while maintaining
compatibility with existing binaries.  (Sloppy userspace programs would
still need to be fixed, so this would still "break userspace".)

> With that in mind, I've
> drafted the following text for the msync(2) man page:
> 
>     NOTES
>        According to POSIX, exactly one of MS_SYNC and MS_ASYNC  must  be
>        specified  in  flags.   However,  Linux permits a call to msync()
>        that specifies neither of these flags, with  semantics  that  are
>        (currently)  equivalent  to  specifying  MS_ASYNC.   (Since Linux
>        2.6.19, MS_ASYNC is in fact a no-op, since  the  kernel  properly
>        tracks  dirty  pages  and  flushes them to storage as necessary.)
>        Notwithstanding the Linux behavior, portable, future-proof appli‐
>        cations  should  ensure  that they specify exactly one of MS_SYNC
>        and MS_ASYNC in flags.
> 
> Comments on this draft welcome.

I agree with Greg's reply to this note.  How about this text instead:

    Exactly one of MS_SYNC and MS_ASYNC must be specified in flags.
    If neither flag is set, the behavior is unspecified.

I'll follow up with a new patch that explicitly defaults to MS_ASYNC (to
document the desire to maintain compaitibility and to prevent unexpected
problems if msync() is ever overhauled again).

Thanks,
Richard

--
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>

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

[Christoph Hellwig]

Guys, I don't really see why you get so worked up about this.  There is
lots and lots of precedent of Linux allowing non-Posix (or non-standard
in general) arguments to system calls.  Even ones that don't have
symbolic names defined for them (the magic 3 open argument for device
files).

Given that we historicaly allowed the 0 argument to msync we'll have to
keep supporting it to not break existing userspace, and adding warnings
triggered by userspace that the person running the system usually can't
fix for something that is entirely harmless at runtime isn't going to
win you friends either.

A "strictly Posix" environment that catches all this sounds fine to me,
but it's something that should in the userspace c runtime, not the
kernel.  The kernel has never been about strict Posix implementations,
it sometimes doesn't even make it easy to implement the semantics in
user land, which is a bit unfortunate.

--
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>

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

[Michael Kerrisk (man-pages)]

[Resending this message from yesterday, since, as Richard Hansen
pointed out, I failed to CC bug-readline@] 

[CC += Peter Zijlstra]
[CC += bug-readline@gnu.org -- maintainers, it _may_ be desirable to
fix your msync() call]

Richard,

On Thu, Apr 3, 2014 at 1:44 AM, Richard Hansen <rhansen@bbn.com> wrote:
> 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.

Maybe. You earlier asserted that the semantics when flags==0 may have
been different, prior to Peter Zijstra's patch,
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=204ec841fbea3e5138168edbc3a76d46747cc987
.
It's not clear to me that that is the case. But, it would be wise to
CC the developer, in case he has an insight.

>   * 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.

There is no doubt that the situation on Linux is an unfortunate mess
from history (and is far from the only one, see
https://lwn.net/Articles/588444/).

And I think everyone would agree that all of the above would be nice
to have, if there was no cost to having them. But, there is a major
cost: the pain of breaking those sloppy user-space applications. And
in fact some casual grepping suggests that many applications would
break, since, for example, libreadline contains (in histfile.c) an
msync() call that omits both MS_SYNC and MS_ASYNC (I have not looked
into the details of what that piece of code does).

But, even if you could find and fix every application that misuses
msync(), new kernels with your proposed changes would still break old
binaries. Linus has made it clear on numerous occasions that kernel
changes must not break user space. So, the change you suggest is never
going to fly (and Christoph's NAK at least saves Linus yelling at you
;-).)

>     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.

I think the only reasonable solution is to better document existing
behavior and what the programmer should do. With that in mind, I've
drafted the following text for the msync(2) man page:

    NOTES
       According to POSIX, exactly one of MS_SYNC and MS_ASYNC  must  be
       specified  in  flags.   However,  Linux permits a call to msync()
       that specifies neither of these flags, with  semantics  that  are
       (currently)  equivalent  to  specifying  MS_ASYNC.   (Since Linux
       2.6.19, MS_ASYNC is in fact a no-op, since  the  kernel  properly
       tracks  dirty  pages  and  flushes them to storage as necessary.)
       Notwithstanding the Linux behavior, portable, future-proof appli‐
       cations  should  ensure  that they specify exactly one of MS_SYNC
       and MS_ASYNC in flags.

Comments on this draft welcome.

Cheers,

Michael


> 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.

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

--
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>

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

[Peter Zijlstra]

On Fri, Apr 04, 2014 at 09:12:58AM +0200, Michael Kerrisk (man-pages) wrote:
> >   * 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.
> 
> Maybe. You earlier asserted that the semantics when flags==0 may have
> been different, prior to Peter Zijstra's patch,
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=204ec841fbea3e5138168edbc3a76d46747cc987
> .
> It's not clear to me that that is the case. But, it would be wise to
> CC the developer, in case he has an insight.

Right; so before that patch there appears to have been a difference.
The code looked like:

  if (flags & MS_ASYNC) {
  	balance_dirty_pages_ratelimited();
  } else if (flags & MS_SYNC) {
  	do_fsync()
  } else {
  	/* do nothing */
  }

Which would give the following semantics:

  msync(.flags = 0) -- scan PTEs and update dirty page accounting
  msync(.flags = MS_ASYNC) -- scan PTEs and dirty throttle
  msync(.flags = MS_SYNC) -- scan PTEs and flush dirty pages

However with the introduction of accurate dirty page accounting in
.19 we always had an accurate dirty page count and both .flags=0 and
.flags=MS_ASYNC turn into the same NO-OP.

Yielding todays state, where 0 and MS_ASYNC don't do anything much and
MS_SYNC issues the fsync() -- although I understand Willy recently
posted a patch to do a data-range-sync instead of the full fsync.


--
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>

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

[Richard Hansen]

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.

Without this change, specifying neither is the same as specifying
flags=MS_ASYNC because nothing in msync() is conditioned on the
MS_ASYNC flag.  This has not always been true, and there's no good
reason to believe that this behavior would have persisted
indefinitely.

The msync(2) man page (as currently written in man-pages.git) is
silent on the behavior if both flags are unset, so this change should
not break an application written by somone who carefully reads the
Linux man pages or the POSIX spec.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/msync.html

Signed-off-by: Richard Hansen <rhansen@bbn.com>
Reported-by: Greg Troxel <gdt@ir.bbn.com>
Reviewed-by: Greg Troxel <gdt@ir.bbn.com>
---
 mm/msync.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/msync.c b/mm/msync.c
index 632df45..472ad3e 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -42,6 +42,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
 		goto out;
 	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
 		goto out;
+	if (!(flags & (MS_ASYNC | MS_SYNC)))
+		goto out;
 	error = -ENOMEM;
 	len = (len + ~PAGE_MASK) & PAGE_MASK;
 	end = start + len;
-- 
1.8.4

--
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>