[REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Akira Takeuchi]

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

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Andrew Morton]

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/

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Thomas Gleixner]

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

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Thomas Gleixner]

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

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Thomas Gleixner]

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

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Linus Torvalds]

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

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Thomas Gleixner]

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

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Akira Takeuchi]

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

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Akira Takeuchi]

> I'll post a patch to LTP porject to modify the test program.

FYI, I posted a patch to LTP project and it has been accepted.
http://ltp.git.sourceforge.net/git/gitweb.cgi?p=ltp/ltp.git;a=commit;h=d6748f57df113699c7c948d734e2427234ef1d58


Regards,
Akira Takeuchi

Re: [REGRESSION][PATCH] mqueue: Ignore the validity of abs_timeout parameter when message can be performed immediately

[Thomas Gleixner]

On Fri, 23 Mar 2012, Akira Takeuchi wrote:

> > I'll post a patch to LTP porject to modify the test program.
> 
> FYI, I posted a patch to LTP project and it has been accepted.
> http://ltp.git.sourceforge.net/git/gitweb.cgi?p=ltp/ltp.git;a=commit;h=d6748f57df113699c7c948d734e2427234ef1d58
> 

Thanks for doing that.

       tglx