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

From: Stanislav Kinsbursky
Date: Tue Jan 28 2014 - 07:46:47 EST


Hello Michael.

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

Acked-by: Stanislav Kinsbursky <skinsbursky@xxxxxxxxxxxxx>


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

---

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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/