Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754818AbaA1Mql (ORCPT ); Tue, 28 Jan 2014 07:46:41 -0500 Received: from relay.parallels.com ([195.214.232.42]:52074 "EHLO relay.parallels.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750816AbaA1Mqk (ORCPT ); Tue, 28 Jan 2014 07:46:40 -0500 Message-ID: <52E7A6AA.2050300@parallels.com> Date: Tue, 28 Jan 2014 16:46:34 +0400 From: Stanislav Kinsbursky User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: "Michael Kerrisk (man-pages)" CC: "Eric W. Biederman" , Serge Hallyn , Pavel Emelyanov , Al Viro , KOSAKI Motohiro , lkml Subject: Re: [PATCH] ipc: Fix 2 bugs in msgrcv() MSG_COPY implementation References: <52E11F9D.7040205@gmail.com> In-Reply-To: <52E11F9D.7040205@gmail.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.30.24.12] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Michael. Thanks you for your careful explanation of the problem. All is true and I like your solution. Acked-by: Stanislav Kinsbursky 3.01.2014 17:56, Michael Kerrisk (man-pages) пишет: > Hello Stanislav, Pavel, > > While documenting the msgrcv() MSG_COPY flag that you (Stanislaw) added > in commit 4a674f34ba04a002244edaf891b5da7fc1473ae8 (==> kernel 3.8), > I've come across a couple of bugs in the implementation. > > Could you please review/ack/nack my patch to resolve these bugs, and > then I'll resubmit, probably also tagging the patch for -stable. > > The two bugs concern MSG_COPY interactions with other flags, namely: > > (A) MSG_COPY + MSG_EXCEPT > (B) MSG_COPY + !IPC_NOWAIT > > The bugs are distinct (and the fix for the first one is obvious), > however my proposed fix for both is a single-line patch, which > is why I'm combining them in a single mail, rather than writing > two mails+patches. (If the fix for the second problem should be > something other than I propose, then two patches might be needed...) > > ===== (A) MSG_COPY + MSG_EXCEPT ===== > > With the addition of the MSG_COPY flag, there are now two msgrcv() flags, > MSG_COPY and MSG_EXCEPT, that modify the meaning of the 'msgtyp' argument > in unrelated ways. Specifying both in the same call is a logical error > that is currently permitted, with the effect that MSG_COPY has priority > and MSG_EXCEPT is ignored. The call should give an error for this case. > The patch below implements that behavior. > > ===== (B) (B) MSG_COPY + !IPC_NOWAIT ===== > > The test code that was submitted in commit > 3a665531a3b7c2ad2c87903b24646be6916340e4 shows MSG_COPY being used in > conjunction with IPC_NOWAIT. In other words, if there is no message > at the position 'msgtyp'. return immediately with the error in ENOMSG. > > What was not (fully) tested is the behavior if MSG_COPY is specified > *without* IPC_NOWAIT, and there is an odd behavior. If the queue contains > less than 'msgtyp' messages, then the call blocks until the next message > is written to the queue. At that point, the msgrcv() call returns a copy > of the newly added message, regardless of whether that message is at the > ordinal position 'msgtyp'. This is clearly bogus, and problematic for > applications that might want to make use of the MSG_COPY flag. > > I see the following possible solutions to this problem: > > (1) Force the call to block until a message *does* appear at the position > 'msgtyp'. > > (2) If the MSG_COPY flag is specified, the kernel should implicitly add > IPC_NOWAIT, so that the call fails with ENOMSG for this case. > > (3) If the MSG_COPY flag is specified, but IPC_NOWAIT is not, generate an > error (probably, EINVAL is the right one). > > I do not know if any application would really want to have the functionality > of solution (1), especially since an application can determine in advance > the number of messages in the queue using msgctl() IPC_STAT. Obviously, this > solution would be the most work to implement. > > Solution (2) would have the effect of silently fixing any applications that > tried to employ broken behavior. However, it would mean that if we later > decided to implement solution (1), then user-space could not easily detect > what the kernel supports (but, since I'm somewhat doubtful that solution (1) > is needed, I'm not sure that this is much of a problem). > > Solution (3) would have the effect of informing broken applications that > they are doing something broken. The downside is that this would cause a > ABI breakage for any applications that are currently employing the broken > behavior. However: > > a) Those applications are almost certainly not getting the results they > expect. > b) Probably, those applications don't even exist, because MSG_COPY is > currently hidden behind CONFIG_CHECKPOINT_RESTORE. > > The upside of solution (3) is that if we later decided to implement > solution (1), user-space could determine what the kernel supports, > via the error return. > > I think solution (3) is mildly preferable to solution (2), and > solution (1) could still be done later if anyone really cares. > The patch below implements solution (3). > > Cheers, > > Michael > > PS For anyone out there still listening, it's the usual story: > documenting an API (and the thinking about, and the testing of the API, > that documentation entails) is the one of the single best ways of > finding bugs in the API, as I've learned from a lot of experience. > Best to do that documentation before releasing the API. > > Signed-off-by: Michael Kerrisk > > --- > > diff -ruNp linux-3.13/ipc/msg.c linux-3.13-msg_copy-fix/ipc/msg.c > --- linux-3.13/ipc/msg.c 2014-01-23 14:13:22.989383573 +0100 > +++ linux-3.13-msg_copy-fix/ipc/msg.c 2014-01-23 13:03:09.600538370 +0100 > @@ -885,6 +885,8 @@ long do_msgrcv(int msqid, void __user *b > return -EINVAL; > > if (msgflg & MSG_COPY) { > + if ((msgflg & MSG_EXCEPT) || !(msgflg & IPC_NOWAIT)) > + return -EINVAL; > copy = prepare_copy(buf, min_t(size_t, bufsz, ns->msg_ctlmax)); > if (IS_ERR(copy)) > return PTR_ERR(copy); > > -- Best regards, Stanislav Kinsbursky -- 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/