Re: [PATCH] ipc: Fix 2 bugs in msgrcv() MSG_COPY implementation

[Stanislav Kinsbursky <skinsbursky@parallels.com>]

Hello Michael.

Thanks you for your careful explanation of the problem.
All is true and I like your solution.

Acked-by: Stanislav Kinsbursky <skinsbursky@parallels.com>


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 <mtk.manpages@gmail.com>
>
> ---
>
> 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