[PATCH 1/2] ipc/mq: Do not vaild queue attributes default/ceiling value If the user pass attr as NULL

[PATCH 1/2] ipc/mq: Do not vaild queue attributes default/ceiling value If the user pass attr as NULL

[Sasikantha Babu]

Kernel should not validate queue attributes default/ceiling value while
creating a mqueue, if user pass attr as NULL. Otherwise In worst case
If the validation fails then sys_mq_open returns -EINVAL/-EOVERFLOW
which will make user clueless about reason for the failure.

Signed-off-by: Sasikantha Babu <sasikanth.v19@gmail.com>
---
 ipc/mqueue.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index ae1996d..04ece80 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -748,9 +748,6 @@ static struct file *do_create(struct ipc_namespace *ipc_ns, struct inode *dir,
 					 ipc_ns->mq_msg_default);
 		def_attr.mq_msgsize = min(ipc_ns->mq_msgsize_max,
 					  ipc_ns->mq_msgsize_default);
-		ret = mq_attr_ok(ipc_ns, &def_attr);
-		if (ret)
-			return ERR_PTR(ret);
 	}
 
 	mode &= ~current_umask();
-- 
1.7.3.4

[PATCH 2/2] ipc/mq: Removed unused def_attr local variable and its initialization in do_create

[Sasikantha Babu]

In the first patch since we removed calling mq_attr_ok to validate
mqueue attributes default/ceil values, So removing the unused def_attr

Signed-off-by: Sasikantha Babu <sasikanth.v19@gmail.com>
---
 ipc/mqueue.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 04ece80..0bcf69c 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -741,13 +741,6 @@ static struct file *do_create(struct ipc_namespace *ipc_ns, struct inode *dir,
 			return ERR_PTR(ret);
 		/* store for use during create */
 		path->dentry->d_fsdata = attr;
-	} else {
-		struct mq_attr def_attr;
-
-		def_attr.mq_maxmsg = min(ipc_ns->mq_msg_max,
-					 ipc_ns->mq_msg_default);
-		def_attr.mq_msgsize = min(ipc_ns->mq_msgsize_max,
-					  ipc_ns->mq_msgsize_default);
 	}
 
 	mode &= ~current_umask();
-- 
1.7.3.4

Re: [PATCH 1/2] ipc/mq: Do not vaild queue attributes default/ceiling value If the user pass attr as NULL

[Doug Ledford]

On 08/12/2013 11:09 AM, Sasikantha Babu wrote:
> Kernel should not validate queue attributes default/ceiling value while
> creating a mqueue, if user pass attr as NULL. Otherwise In worst case
> If the validation fails then sys_mq_open returns -EINVAL/-EOVERFLOW
> which will make user clueless about reason for the failure.
> 
> Signed-off-by: Sasikantha Babu <sasikanth.v19@gmail.com>
> ---
>  ipc/mqueue.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index ae1996d..04ece80 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -748,9 +748,6 @@ static struct file *do_create(struct ipc_namespace *ipc_ns, struct inode *dir,
>  					 ipc_ns->mq_msg_default);
>  		def_attr.mq_msgsize = min(ipc_ns->mq_msgsize_max,
>  					  ipc_ns->mq_msgsize_default);
> -		ret = mq_attr_ok(ipc_ns, &def_attr);
> -		if (ret)
> -			return ERR_PTR(ret);
>  	}
>  
>  	mode &= ~current_umask();
> 

Nak.  The only two instances where mq_attr_ok() will reject the default
values are if either of the size or num are <= 0 or if the possible
total size of the struct is large enough to overflow the memory
accounting.  Since we can't allow the memory accounting to overflow no
matter what the defaults are or what the user requests, we can't skip
that check.  And we don't allow 0 element or 0 size queues, so we can't
skip that check.  If the user tweaks the default knobs in the kernel
such that the default values result in an impossible allocation, that's
the user's fault.  You don't allow the kernel to create a known broken
queue just because someone twiddled the default values to something invalid.

There are two possible different fixes:

1) Change this code to recognize that the default values resulted in an
error and print out a kernel message for the user to find

or

2) Change the memory accounting such that we don't check accounting
overflow on allocation but instead check it on msg queue and allow
people to create really large queues that they can fill up until they
hit the memory allocation overflow and then start rejecting queueing any
new messages to the queue until some of the old messages have been
removed and space freed up.  If that were done, then the overflow checks
in mq_attr_ok could go away.  But this would add some (although not a
lot) of overhead on the msg queue path.

Re: [PATCH 2/2] ipc/mq: Removed unused def_attr local variable and its initialization in do_create

[Doug Ledford]

On 08/12/2013 11:09 AM, Sasikantha Babu wrote:
> In the first patch since we removed calling mq_attr_ok to validate
> mqueue attributes default/ceil values, So removing the unused def_attr
> 
> Signed-off-by: Sasikantha Babu <sasikanth.v19@gmail.com>
> ---
>  ipc/mqueue.c |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 04ece80..0bcf69c 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -741,13 +741,6 @@ static struct file *do_create(struct ipc_namespace *ipc_ns, struct inode *dir,
>  			return ERR_PTR(ret);
>  		/* store for use during create */
>  		path->dentry->d_fsdata = attr;
> -	} else {
> -		struct mq_attr def_attr;
> -
> -		def_attr.mq_maxmsg = min(ipc_ns->mq_msg_max,
> -					 ipc_ns->mq_msg_default);
> -		def_attr.mq_msgsize = min(ipc_ns->mq_msgsize_max,
> -					  ipc_ns->mq_msgsize_default);
>  	}
>  
>  	mode &= ~current_umask();
> 

Nak to this patch as well since it depends on the previous patch.