* [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
@ 2012-03-02 7:42 Akira Takeuchi
2012-03-14 21:46 ` Andrew Morton
0 siblings, 1 reply; 10+ messages in thread
From: Akira Takeuchi @ 2012-03-02 7:42 UTC (permalink / raw)
To: linux-kernel; +Cc: torvalds
This patch fixes up the regression problem of mq_timed{send,receive} syscall.
When a message of mqueue can be performed immediately,
the validity of abs_timeout parameter should not be checked.
According to the manpage of mq_timedreceive:
Under no circumstance shall the operation fail with a timeout
if a message can be removed from the message queue immediately.
The validity of the abstime parameter need not be checked
if a message can be removed from the message queue immediately.
On 2.6.35+ kernel, mq_timed{send,receive} returns EINVAL incorrectly,
in this situation.
I found this problem during the OPTS testcase
"conformance/interfaces/mq_timedreceive/10-2":
# ./10-2.test
FAIL: the validity of abs_timeout is checked
Test FAILED
Signed-off-by: Akira Takeuchi <takeuchi.akr@jp.panasonic.com>
Signed-off-by: Kiyoshi Owada <owada.kiyoshi@jp.panasonic.com>
---
ipc/mqueue.c | 31 +++++++++++++++++++++++++++----
1 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 86ee272..7cd4411 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -861,14 +861,22 @@ SYSCALL_DEFINE5(mq_timedsend, mqd_t, mqdes, const char __user *, u_msg_ptr,
struct msg_msg *msg_ptr;
struct mqueue_inode_info *info;
ktime_t expires, *timeout = NULL;
+ int timeout_param_error = 0;
struct timespec ts;
int ret;
if (u_abs_timeout) {
int res = prepare_timeout(u_abs_timeout, &expires, &ts);
if (res)
- return res;
- timeout = &expires;
+ /*
+ * The validity of the abs_timeout parameter need not be
+ * checked when there is sufficient room in the queue.
+ * So, do not return here, even if the parameter is
+ * invalid.
+ */
+ timeout_param_error = res;
+ else
+ timeout = &expires;
}
if (unlikely(msg_prio >= (unsigned long) MQ_PRIO_MAX))
@@ -916,6 +924,9 @@ SYSCALL_DEFINE5(mq_timedsend, mqd_t, mqdes, const char __user *, u_msg_ptr,
if (filp->f_flags & O_NONBLOCK) {
spin_unlock(&info->lock);
ret = -EAGAIN;
+ } else if (unlikely(timeout_param_error)) {
+ spin_unlock(&info->lock);
+ ret = timeout_param_error;
} else {
wait.task = current;
wait.msg = (void *) msg_ptr;
@@ -955,13 +966,21 @@ SYSCALL_DEFINE5(mq_timedreceive, mqd_t, mqdes, char __user *, u_msg_ptr,
struct mqueue_inode_info *info;
struct ext_wait_queue wait;
ktime_t expires, *timeout = NULL;
+ int timeout_param_error = 0;
struct timespec ts;
if (u_abs_timeout) {
int res = prepare_timeout(u_abs_timeout, &expires, &ts);
if (res)
- return res;
- timeout = &expires;
+ /*
+ * The validity of the abs_timeout parameter need not be
+ * checked if a message can be removed from the message
+ * queue immediately. So, do not return here, even if
+ * the parameter is invalid.
+ */
+ timeout_param_error = res;
+ else
+ timeout = &expires;
}
audit_mq_sendrecv(mqdes, msg_len, 0, timeout ? &ts : NULL);
@@ -996,6 +1015,10 @@ SYSCALL_DEFINE5(mq_timedreceive, mqd_t, mqdes, char __user *, u_msg_ptr,
if (filp->f_flags & O_NONBLOCK) {
spin_unlock(&info->lock);
ret = -EAGAIN;
+ } else if (unlikely(timeout_param_error)) {
+ spin_unlock(&info->lock);
+ ret = timeout_param_error;
+ msg_ptr = NULL; /* just for shutting up warning */
} else {
wait.task = current;
wait.state = STATE_NONE;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
2012-03-02 7:42 [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately Akira Takeuchi
@ 2012-03-14 21:46 ` Andrew Morton
2012-03-14 22:08 ` Thomas Gleixner
2012-03-14 23:02 ` Thomas Gleixner
0 siblings, 2 replies; 10+ messages in thread
From: Andrew Morton @ 2012-03-14 21:46 UTC (permalink / raw)
To: Akira Takeuchi
Cc: linux-kernel, torvalds, Carsten Emde, Thomas Gleixner,
Manfred Spraul
On Fri, 02 Mar 2012 16:42:35 +0900
Akira Takeuchi <takeuchi.akr@jp.panasonic.com> wrote:
> This patch fixes up the regression problem of mq_timed{send,receive} syscall.
>
> When a message of mqueue can be performed immediately,
> the validity of abs_timeout parameter should not be checked.
>
> According to the manpage of mq_timedreceive:
> Under no circumstance shall the operation fail with a timeout
> if a message can be removed from the message queue immediately.
> The validity of the abstime parameter need not be checked
> if a message can be removed from the message queue immediately.
>
> On 2.6.35+ kernel, mq_timed{send,receive} returns EINVAL incorrectly,
> in this situation.
>
> I found this problem during the OPTS testcase
> "conformance/interfaces/mq_timedreceive/10-2":
>
> # ./10-2.test
> FAIL: the validity of abs_timeout is checked
> Test FAILED
Are you able to identify the commit which caused this regression? I'm
guessing
commit 9ca7d8e6834c40a99622bbe4a88aaf64313ae43c
Author: Carsten Emde <C.Emde@osadl.org>
AuthorDate: Fri Apr 2 22:40:20 2010 +0200
Commit: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue Apr 6 21:50:03 2010 +0200
mqueue: Convert message queue timeout to use hrtimers
> Signed-off-by: Akira Takeuchi <takeuchi.akr@jp.panasonic.com>
> Signed-off-by: Kiyoshi Owada <owada.kiyoshi@jp.panasonic.com>
> ---
> ipc/mqueue.c | 31 +++++++++++++++++++++++++++----
> 1 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 86ee272..7cd4411 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -861,14 +861,22 @@ SYSCALL_DEFINE5(mq_timedsend, mqd_t, mqdes, const char __user *, u_msg_ptr,
> struct msg_msg *msg_ptr;
> struct mqueue_inode_info *info;
> ktime_t expires, *timeout = NULL;
> + int timeout_param_error = 0;
> struct timespec ts;
> int ret;
>
> if (u_abs_timeout) {
> int res = prepare_timeout(u_abs_timeout, &expires, &ts);
> if (res)
> - return res;
> - timeout = &expires;
> + /*
> + * The validity of the abs_timeout parameter need not be
> + * checked when there is sufficient room in the queue.
> + * So, do not return here, even if the parameter is
> + * invalid.
> + */
> + timeout_param_error = res;
> + else
> + timeout = &expires;
> }
>
> if (unlikely(msg_prio >= (unsigned long) MQ_PRIO_MAX))
> @@ -916,6 +924,9 @@ SYSCALL_DEFINE5(mq_timedsend, mqd_t, mqdes, const char __user *, u_msg_ptr,
> if (filp->f_flags & O_NONBLOCK) {
> spin_unlock(&info->lock);
> ret = -EAGAIN;
> + } else if (unlikely(timeout_param_error)) {
> + spin_unlock(&info->lock);
> + ret = timeout_param_error;
> } else {
> wait.task = current;
> wait.msg = (void *) msg_ptr;
> @@ -955,13 +966,21 @@ SYSCALL_DEFINE5(mq_timedreceive, mqd_t, mqdes, char __user *, u_msg_ptr,
> struct mqueue_inode_info *info;
> struct ext_wait_queue wait;
> ktime_t expires, *timeout = NULL;
> + int timeout_param_error = 0;
> struct timespec ts;
>
> if (u_abs_timeout) {
> int res = prepare_timeout(u_abs_timeout, &expires, &ts);
> if (res)
> - return res;
> - timeout = &expires;
> + /*
> + * The validity of the abs_timeout parameter need not be
> + * checked if a message can be removed from the message
> + * queue immediately. So, do not return here, even if
> + * the parameter is invalid.
> + */
> + timeout_param_error = res;
> + else
> + timeout = &expires;
> }
>
> audit_mq_sendrecv(mqdes, msg_len, 0, timeout ? &ts : NULL);
> @@ -996,6 +1015,10 @@ SYSCALL_DEFINE5(mq_timedreceive, mqd_t, mqdes, char __user *, u_msg_ptr,
> if (filp->f_flags & O_NONBLOCK) {
> spin_unlock(&info->lock);
> ret = -EAGAIN;
> + } else if (unlikely(timeout_param_error)) {
> + spin_unlock(&info->lock);
> + ret = timeout_param_error;
> + msg_ptr = NULL; /* just for shutting up warning */
> } else {
> wait.task = current;
> wait.state = STATE_NONE;
> --
> 1.7.4.1
>
>
> --
> 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/
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
2012-03-14 21:46 ` Andrew Morton
@ 2012-03-14 22:08 ` Thomas Gleixner
2012-03-15 0:28 ` Thomas Gleixner
2012-03-14 23:02 ` Thomas Gleixner
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2012-03-14 22:08 UTC (permalink / raw)
To: Andrew Morton
Cc: Akira Takeuchi, linux-kernel, torvalds, Carsten Emde,
Manfred Spraul
On Wed, 14 Mar 2012, Andrew Morton wrote:
> On Fri, 02 Mar 2012 16:42:35 +0900
> Akira Takeuchi <takeuchi.akr@jp.panasonic.com> wrote:
>
> > This patch fixes up the regression problem of mq_timed{send,receive} syscall.
> >
> > When a message of mqueue can be performed immediately,
> > the validity of abs_timeout parameter should not be checked.
> >
> > According to the manpage of mq_timedreceive:
> > Under no circumstance shall the operation fail with a timeout
> > if a message can be removed from the message queue immediately.
> > The validity of the abstime parameter need not be checked
> > if a message can be removed from the message queue immediately.
Those POSIX spec folks definitely have a seriously distorted
relationship to timers and timekeeping.
So the caller knows upfront when he needs to provide a valid timespec
and when not. So the users of mq_.... are into crystal ball
programming or what?
> > On 2.6.35+ kernel, mq_timed{send,receive} returns EINVAL incorrectly,
> > in this situation.
> >
> > I found this problem during the OPTS testcase
> > "conformance/interfaces/mq_timedreceive/10-2":
> >
> > # ./10-2.test
> > FAIL: the validity of abs_timeout is checked
> > Test FAILED
>
> Are you able to identify the commit which caused this regression? I'm
> guessing
>
> commit 9ca7d8e6834c40a99622bbe4a88aaf64313ae43c
> Author: Carsten Emde <C.Emde@osadl.org>
> AuthorDate: Fri Apr 2 22:40:20 2010 +0200
> Commit: Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Tue Apr 6 21:50:03 2010 +0200
>
> mqueue: Convert message queue timeout to use hrtimers
Right, because nobody imagined that the specification would be as
asinine.
I'll pick it up and add a stable tag.
Sigh,
tglx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
2012-03-14 22:08 ` Thomas Gleixner
@ 2012-03-15 0:28 ` Thomas Gleixner
2012-03-15 3:48 ` Linus Torvalds
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2012-03-15 0:28 UTC (permalink / raw)
To: Andrew Morton
Cc: Akira Takeuchi, linux-kernel, torvalds, Carsten Emde,
Manfred Spraul
On Wed, 14 Mar 2012, Thomas Gleixner wrote:
> On Wed, 14 Mar 2012, Andrew Morton wrote:
>
> > On Fri, 02 Mar 2012 16:42:35 +0900
> > Akira Takeuchi <takeuchi.akr@jp.panasonic.com> wrote:
> >
> > > This patch fixes up the regression problem of mq_timed{send,receive} syscall.
> > >
> > > When a message of mqueue can be performed immediately,
> > > the validity of abs_timeout parameter should not be checked.
> > >
> > > According to the manpage of mq_timedreceive:
> > > Under no circumstance shall the operation fail with a timeout
> > > if a message can be removed from the message queue immediately.
> > > The validity of the abstime parameter need not be checked
> > > if a message can be removed from the message queue immediately.
>
> Those POSIX spec folks definitely have a seriously distorted
> relationship to timers and timekeeping.
>
> So the caller knows upfront when he needs to provide a valid timespec
> and when not. So the users of mq_.... are into crystal ball
> programming or what?
Thinking more about it. The manpage according to
git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git
does not mention that insanity at all.
So what the patch is referring to is the POSIX spec:
http://pubs.opengroup.org/onlinepubs/009604499/functions/mq_timedsend.html
http://pubs.opengroup.org/onlinepubs/009604499/functions/mq_timedreceive.html
Aside of my previous ranting about the insanity of that, the following
is actually open for interpretation:
"The validity of the abstime parameter need not be checked if a
message can be removed from the message queue immediately."
It's not necessary to check the validity of the abstime parameter, but
the spec does not forbid it either.
So for the sake of sanity we rather fix that test case and keep the
code as is. There haven't been regression reports from real world
applications within 2 years, so there is no point to add more
convolution to the mqueue code just to satisfy a random interpretation
of the spec by a test suite.
Again, there is no point to interpret it as "avoid the check" simply
because user space cannot know whether there is room to queue or
whether there is a message waiting. So requiring a valid timespec is
always the Right Thing to do.
Thanks,
tglx
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
2012-03-15 0:28 ` Thomas Gleixner
@ 2012-03-15 3:48 ` Linus Torvalds
2012-03-15 11:02 ` Thomas Gleixner
0 siblings, 1 reply; 10+ messages in thread
From: Linus Torvalds @ 2012-03-15 3:48 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, Akira Takeuchi, linux-kernel, Carsten Emde,
Manfred Spraul
On Wed, Mar 14, 2012 at 5:28 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Aside of my previous ranting about the insanity of that, the following
> is actually open for interpretation:
>
> "The validity of the abstime parameter need not be checked if a
> message can be removed from the message queue immediately."
>
> It's not necessary to check the validity of the abstime parameter, but
> the spec does not forbid it either.
Yeah, I have to agree with your reading.
I think that checking the validity is the sane thing to do, especially
if it just "falls out" of how the code is written.
At the same time, I can also imagine code that doesn't even look at
the timeout parameter unless it is about to go to sleep, so I can
understand the POSIX wording too: you don't *have* to check the
validity if it's irrelevant, and not checking it is the natural thing
for the code.
But it certainly doesn't seem to disallow the current code either.
So I do not think that the POSIX wording should be taken to mean that
"you mustn't check the validity". It's a "whatever", not a "must do
X".
With the current code, checking the validity of the timeout falls out
from what we do. I don't think we need to work around that, based on
the POSIX wording. And if there are no applications that actually
broke, I don't think we should care.
Is there some other standard that says that you *have* to let crazy
invalid values go?
Linus
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
2012-03-15 3:48 ` Linus Torvalds
@ 2012-03-15 11:02 ` Thomas Gleixner
2012-03-16 5:11 ` Akira Takeuchi
0 siblings, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2012-03-15 11:02 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Akira Takeuchi, linux-kernel, Carsten Emde,
Manfred Spraul
On Wed, 14 Mar 2012, Linus Torvalds wrote:
> With the current code, checking the validity of the timeout falls out
> from what we do. I don't think we need to work around that, based on
> the POSIX wording. And if there are no applications that actually
> broke, I don't think we should care.
>
> Is there some other standard that says that you *have* to let crazy
> invalid values go?
Not that I'm aware of. Everything I found so far is copied from the
POSIX spec.
Thanks,
tglx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
2012-03-15 11:02 ` Thomas Gleixner
@ 2012-03-16 5:11 ` Akira Takeuchi
2012-03-23 0:03 ` Akira Takeuchi
0 siblings, 1 reply; 10+ messages in thread
From: Akira Takeuchi @ 2012-03-16 5:11 UTC (permalink / raw)
To: Thomas Gleixner, Andrew Morton
Cc: Linus Torvalds, linux-kernel, Carsten Emde, Manfred Spraul
On Thu, 15 Mar 2012 12:02:49 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 14 Mar 2012, Linus Torvalds wrote:
> > With the current code, checking the validity of the timeout falls out
> > from what we do. I don't think we need to work around that, based on
> > the POSIX wording. And if there are no applications that actually
> > broke, I don't think we should care.
> >
> > Is there some other standard that says that you *have* to let crazy
> > invalid values go?
>
> Not that I'm aware of. Everything I found so far is copied from the
> POSIX spec.
Okay. I agree with you on the interpretation of POSIX spec.
Andrew, could you remove my patch from -mm tree ?
I'll post a patch to LTP porject to modify the test program.
Thank you,
Akira Takeuchi
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately
2012-03-14 21:46 ` Andrew Morton
2012-03-14 22:08 ` Thomas Gleixner
@ 2012-03-14 23:02 ` Thomas Gleixner
1 sibling, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2012-03-14 23:02 UTC (permalink / raw)
To: Andrew Morton
Cc: Akira Takeuchi, linux-kernel, torvalds, Carsten Emde,
Manfred Spraul
B1;2601;0cOn Wed, 14 Mar 2012, Andrew Morton wrote:
> On Fri, 02 Mar 2012 16:42:35 +0900
> Akira Takeuchi <takeuchi.akr@jp.panasonic.com> wrote:
> > @@ -996,6 +1015,10 @@ SYSCALL_DEFINE5(mq_timedreceive, mqd_t, mqdes, char __user *, u_msg_ptr,
> > if (filp->f_flags & O_NONBLOCK) {
> > spin_unlock(&info->lock);
> > ret = -EAGAIN;
> > + } else if (unlikely(timeout_param_error)) {
> > + spin_unlock(&info->lock);
> > + ret = timeout_param_error;
> > + msg_ptr = NULL; /* just for shutting up warning */
Huch? We are not "just" shutting up some warning because the compiler
is yelling at us. No, either we initialize the variable upfront to
NULL or we skip the whole return path by going to fput directly.
Duh, that code is convoluted enough already.
Thanks,
tglx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-03-23 9:02 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-02 7:42 [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately Akira Takeuchi
2012-03-14 21:46 ` Andrew Morton
2012-03-14 22:08 ` Thomas Gleixner
2012-03-15 0:28 ` Thomas Gleixner
2012-03-15 3:48 ` Linus Torvalds
2012-03-15 11:02 ` Thomas Gleixner
2012-03-16 5:11 ` Akira Takeuchi
2012-03-23 0:03 ` Akira Takeuchi
2012-03-23 9:01 ` Thomas Gleixner
2012-03-14 23:02 ` Thomas Gleixner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox