Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161148AbaDCAWf (ORCPT ); Wed, 2 Apr 2014 20:22:35 -0400 Received: from smtp.bbn.com ([128.33.1.81]:60486 "EHLO smtp.bbn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161102AbaDCAWe (ORCPT ); Wed, 2 Apr 2014 20:22:34 -0400 X-Greylist: delayed 2241 seconds by postgrey-1.27 at vger.kernel.org; Wed, 02 Apr 2014 20:22:33 EDT X-Submitted: to socket.bbn.com (Postfix) with ESMTPSA id C8F1840050 Message-ID: <533CA0F6.2070100@bbn.com> Date: Wed, 02 Apr 2014 19:44:54 -0400 From: Richard Hansen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Steven Whitehouse , Christoph Hellwig , mtk.manpages@gmail.com CC: linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, Greg Troxel Subject: Re: [PATCH] mm: msync: require either MS_ASYNC or MS_SYNC References: <533B04A9.6090405@bbn.com> <20140402111032.GA27551@infradead.org> <1396439119.2726.29.camel@menhir> In-Reply-To: <1396439119.2726.29.camel@menhir> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/