[resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[kosaki.motohiro]

From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Mqueue limitation is slightly naieve parameter likes other ipcs
because unprivileged user can consume kernel memory by using ipcs.

Thus, too aggressive raise bring us security issue. Example,
current setting allow evil unprivileged user use 256GB (= 256
* 1024 * 1024*1024) and it's enough large to system will belome
unresponsive. Don't do that.

Instead, every admin should adjust the knobs for their own systems.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Acked-by: Doug Ledford <dledford@redhat.com>
Acked-by: Joe Korty <joe.korty@ccur.com>
Cc: Amerigo Wang <amwang@redhat.com>
Cc: Serge E. Hallyn <serue@us.ibm.com>
Cc: Jiri Slaby <jslaby@suse.cz>
---
 include/linux/ipc_namespace.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index e2bac00..2488535 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -118,13 +118,13 @@ extern int mq_init_ns(struct ipc_namespace *ns);
 #define DFLT_QUEUESMAX		      256
 #define HARD_QUEUESMAX		     1024
 #define MIN_MSGMAX			1
-#define DFLT_MSG		       64U
-#define DFLT_MSGMAX		     1024
+#define DFLT_MSG		       10U
+#define DFLT_MSGMAX		       10
 #define HARD_MSGMAX		    65536
 #define MIN_MSGSIZEMAX		      128
 #define DFLT_MSGSIZE		     8192U
-#define DFLT_MSGSIZEMAX		(1024*1024)
-#define HARD_MSGSIZEMAX	     (16*1024*1024)
+#define DFLT_MSGSIZEMAX		     8192
+#define HARD_MSGSIZEMAX	    (16*1024*1024)
 #else
 static inline int mq_init_ns(struct ipc_namespace *ns) { return 0; }
 #endif
-- 
1.7.1

[resend][PATCH 2/3] mqueue: don't use kmalloc with KMALLOC_MAX_SIZE

[kosaki.motohiro]

From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

KMALLOC_MAX_SIZE is no good threshold. It is extream high and
problematic. Unfortunately, some silly drivers depend on and
we can't change it. but any new code don't use such extream
ugly high order allocations. It bring us awful fragmentation
issue and system slowdown.

Signed-off-by: KOSAKI Motohiro <mkosaki@jp.fujitsu.com>
Acked-by: Doug Ledford <dledford@redhat.com>
Acked-by: Joe Korty <joe.korty@ccur.com>
Cc: Amerigo Wang <amwang@redhat.com>
Cc: Serge E. Hallyn <serue@us.ibm.com>
Cc: Jiri Slaby <jslaby@suse.cz>
---
 ipc/mqueue.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index c246c83..315f84f 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -151,7 +151,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
 			info->attr.mq_msgsize = attr->mq_msgsize;
 		}
 		mq_msg_tblsz = info->attr.mq_maxmsg * sizeof(struct msg_msg *);
-		if (mq_msg_tblsz > KMALLOC_MAX_SIZE)
+		if (mq_msg_tblsz > PAGE_SIZE)
 			info->messages = vmalloc(mq_msg_tblsz);
 		else
 			info->messages = kmalloc(mq_msg_tblsz, GFP_KERNEL);
@@ -275,7 +275,7 @@ static void mqueue_evict_inode(struct inode *inode)
 	spin_lock(&info->lock);
 	for (i = 0; i < info->attr.mq_curmsgs; i++)
 		free_msg(info->messages[i]);
-	if (info->attr.mq_maxmsg * sizeof(struct msg_msg *) > KMALLOC_MAX_SIZE)
+	if (is_vmalloc_addr(info->messages))
 		vfree(info->messages);
 	else
 		kfree(info->messages);
-- 
1.7.1

[resend][PATCH 3/3] mqueue: separate mqueue default value from maximum value

[kosaki.motohiro]

From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

commit b231cca438 (message queues: increase range limits)
changed mqueue default value when attr parameter is specified NULL
from hard coded value to fs.mqueue.{msg,msgsize}_max sysctl value.

This made large side effect. When user need to use two mqueue
applications 1) using !NULL attr parameter and it require big
message size and 2) using NULL attr parameter and only need small
size message, app (1) require to raise fs.mqueue.msgsize_max and
app (2) consume large memory size even though it doesn't need.

Doug Ledford propsed to switch back it to static hard coded value.
However it also has a compatibility problem. Some applications might
started depend on the default value is tunable.

The solution is to separate default value from maximum value.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Acked-by: Doug Ledford <dledford@redhat.com>
Acked-by: Joe Korty <joe.korty@ccur.com>
Cc: Amerigo Wang <amwang@redhat.com>
Cc: Serge E. Hallyn <serue@us.ibm.com>
Cc: Jiri Slaby <jslaby@suse.cz>
---
 Documentation/sysctl/fs.txt   |    7 +++++++
 include/linux/ipc_namespace.h |    2 ++
 ipc/mq_sysctl.c               |   18 ++++++++++++++++++
 ipc/mqueue.c                  |    9 ++++++---
 ipc/msgutil.c                 |    2 ++
 5 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 88fd7f5..13d6166 100644
--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt
@@ -225,6 +225,13 @@ a queue must be less or equal then msg_max.
 maximum  message size value (it is every  message queue's attribute set during
 its creation).
 
+/proc/sys/fs/mqueue/msg_default is  a read/write  file for setting/getting the
+default number of messages in a queue value if attr parameter of mq_open(2) is
+NULL. If it exceed msg_max, the default value is initialized msg_max.
+
+/proc/sys/fs/mqueue/msgsize_default is a read/write file for setting/getting
+the default message size value if attr parameter of mq_open(2) is NULL. If it
+exceed msgsize_max, the default value is initialized msgsize_max.
 
 4. /proc/sys/fs/epoll - Configuration options for the epoll interface
 --------------------------------------------------------
diff --git a/include/linux/ipc_namespace.h b/include/linux/ipc_namespace.h
index 2488535..5499c92 100644
--- a/include/linux/ipc_namespace.h
+++ b/include/linux/ipc_namespace.h
@@ -62,6 +62,8 @@ struct ipc_namespace {
 	unsigned int    mq_queues_max;   /* initialized to DFLT_QUEUESMAX */
 	unsigned int    mq_msg_max;      /* initialized to DFLT_MSGMAX */
 	unsigned int    mq_msgsize_max;  /* initialized to DFLT_MSGSIZEMAX */
+	unsigned int    mq_msg_default;
+	unsigned int    mq_msgsize_default;
 
 	/* user_ns which owns the ipc ns */
 	struct user_namespace *user_ns;
diff --git a/ipc/mq_sysctl.c b/ipc/mq_sysctl.c
index e22336a..383d638 100644
--- a/ipc/mq_sysctl.c
+++ b/ipc/mq_sysctl.c
@@ -73,6 +73,24 @@ static ctl_table mq_sysctls[] = {
 		.extra1		= &msg_maxsize_limit_min,
 		.extra2		= &msg_maxsize_limit_max,
 	},
+	{
+		.procname	= "msg_default",
+		.data		= &init_ipc_ns.mq_msg_default,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_mq_dointvec_minmax,
+		.extra1		= &msg_max_limit_min,
+		.extra2		= &msg_max_limit_max,
+	},
+	{
+		.procname	= "msgsize_default",
+		.data		= &init_ipc_ns.mq_msgsize_default,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_mq_dointvec_minmax,
+		.extra1		= &msg_maxsize_limit_min,
+		.extra2		= &msg_maxsize_limit_max,
+	},
 	{}
 };
 
diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 315f84f..894a61b 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -143,9 +143,10 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
 		info->qsize = 0;
 		info->user = NULL;	/* set when all is ok */
 		memset(&info->attr, 0, sizeof(info->attr));
-		info->attr.mq_maxmsg = min(ipc_ns->mq_msg_max, DFLT_MSG);
-		info->attr.mq_msgsize =
-			min(ipc_ns->mq_msgsize_max, DFLT_MSGSIZE);
+		info->attr.mq_maxmsg = min(ipc_ns->mq_msg_max,
+					   ipc_ns->mq_msg_default);
+		info->attr.mq_msgsize = min(ipc_ns->mq_msgsize_max,
+					    ipc_ns->mq_msgsize_default);
 		if (attr) {
 			info->attr.mq_maxmsg = attr->mq_maxmsg;
 			info->attr.mq_msgsize = attr->mq_msgsize;
@@ -1266,6 +1267,8 @@ int mq_init_ns(struct ipc_namespace *ns)
 	ns->mq_queues_max    = DFLT_QUEUESMAX;
 	ns->mq_msg_max       = DFLT_MSGMAX;
 	ns->mq_msgsize_max   = DFLT_MSGSIZEMAX;
+	ns->mq_msg_default   = DFLT_MSG;
+	ns->mq_msgsize_default  = DFLT_MSGSIZE;
 
 	ns->mq_mnt = kern_mount_data(&mqueue_fs_type, ns);
 	if (IS_ERR(ns->mq_mnt)) {
diff --git a/ipc/msgutil.c b/ipc/msgutil.c
index 8b5ce5d..a344216 100644
--- a/ipc/msgutil.c
+++ b/ipc/msgutil.c
@@ -31,6 +31,8 @@ struct ipc_namespace init_ipc_ns = {
 	.mq_queues_max   = DFLT_QUEUESMAX,
 	.mq_msg_max      = DFLT_MSGMAX,
 	.mq_msgsize_max  = DFLT_MSGSIZEMAX,
+	.mq_msg_default	    = DFLT_MSG,
+	.mq_msgsize_default = DFLT_MSGSIZE,
 #endif
 	.user_ns = &init_user_ns,
 };
-- 
1.7.1

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[KOSAKI Motohiro]

(12/9/11 5:35 PM), kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> 
> Mqueue limitation is slightly naieve parameter likes other ipcs
> because unprivileged user can consume kernel memory by using ipcs.
> 
> Thus, too aggressive raise bring us security issue. Example,
> current setting allow evil unprivileged user use 256GB (= 256
> * 1024 * 1024*1024) and it's enough large to system will belome
> unresponsive. Don't do that.
> 
> Instead, every admin should adjust the knobs for their own systems.
> 
> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> Acked-by: Doug Ledford<dledford@redhat.com>
> Acked-by: Joe Korty<joe.korty@ccur.com>
> Cc: Amerigo Wang<amwang@redhat.com>
> Cc: Serge E. Hallyn<serue@us.ibm.com>
> Cc: Jiri Slaby<jslaby@suse.cz>

Andrew, can you please pick up this series into your tree? because your
tree have related Doug mqueue.

Thanks.

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[KOSAKI Motohiro]

(12/9/11 5:35 PM), kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> 
> Mqueue limitation is slightly naieve parameter likes other ipcs
> because unprivileged user can consume kernel memory by using ipcs.
> 
> Thus, too aggressive raise bring us security issue. Example,
> current setting allow evil unprivileged user use 256GB (= 256
> * 1024 * 1024*1024) and it's enough large to system will belome
> unresponsive. Don't do that.
> 
> Instead, every admin should adjust the knobs for their own systems.
> 
> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> Acked-by: Doug Ledford<dledford@redhat.com>
> Acked-by: Joe Korty<joe.korty@ccur.com>
> Cc: Amerigo Wang<amwang@redhat.com>
> Cc: Serge E. Hallyn<serue@us.ibm.com>
> Cc: Jiri Slaby<jslaby@suse.cz>

Andrew, can you please pick up this series into your tree? because your
tree have related Doug mqueue patch series.

Thanks.

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[Andrew Morton]

On Sun, 18 Dec 2011 13:29:56 -0500 KOSAKI Motohiro <kosaki.motohiro@gmail.com> wrote:

> (12/9/11 5:35 PM), kosaki.motohiro@gmail.com wrote:
> > From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> > 
> > Mqueue limitation is slightly naieve parameter likes other ipcs
> > because unprivileged user can consume kernel memory by using ipcs.
> > 
> > Thus, too aggressive raise bring us security issue. Example,
> > current setting allow evil unprivileged user use 256GB (= 256
> > * 1024 * 1024*1024) and it's enough large to system will belome
> > unresponsive. Don't do that.
> > 
> > Instead, every admin should adjust the knobs for their own systems.
> > 
> > Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> > Acked-by: Doug Ledford<dledford@redhat.com>
> > Acked-by: Joe Korty<joe.korty@ccur.com>
> > Cc: Amerigo Wang<amwang@redhat.com>
> > Cc: Serge E. Hallyn<serue@us.ibm.com>
> > Cc: Jiri Slaby<jslaby@suse.cz>
> 
> Andrew, can you please pick up this series into your tree? because your
> tree have related Doug mqueue patch series.

Soon.  I need to go back and read the email trail regarding Doug's patches.

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3737 bytes --]

On 12/18/2011 01:38 PM, Andrew Morton wrote:
> On Sun, 18 Dec 2011 13:29:56 -0500 KOSAKI Motohiro <kosaki.motohiro@gmail.com> wrote:
> 
>> (12/9/11 5:35 PM), kosaki.motohiro@gmail.com wrote:
>>> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>>
>>> Mqueue limitation is slightly naieve parameter likes other ipcs
>>> because unprivileged user can consume kernel memory by using ipcs.
>>>
>>> Thus, too aggressive raise bring us security issue. Example,
>>> current setting allow evil unprivileged user use 256GB (= 256
>>> * 1024 * 1024*1024) and it's enough large to system will belome
>>> unresponsive. Don't do that.
>>>
>>> Instead, every admin should adjust the knobs for their own systems.
>>>
>>> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>> Acked-by: Doug Ledford<dledford@redhat.com>
>>> Acked-by: Joe Korty<joe.korty@ccur.com>
>>> Cc: Amerigo Wang<amwang@redhat.com>
>>> Cc: Serge E. Hallyn<serue@us.ibm.com>
>>> Cc: Jiri Slaby<jslaby@suse.cz>
>>
>> Andrew, can you please pick up this series into your tree? because your
>> tree have related Doug mqueue patch series.
> 
> Soon.  I need to go back and read the email trail regarding Doug's patches.

This has obviously fallen through the cracks.  Can we please see if we
can get this put to bed.

Andrew, long and short of the story on this entire patch set is:

A) Current Linus kernel is broken in regards to customer needs, maximums
are too low.
B) My patch set upped maximums and also upped the default maximums on
the system, but inadvertently also upped the default mqueue size the
original patch that my entire patch set is more or less reverting tied
maximums and defaults together.  This broke some apps that didn't pass
an attr struct to open as it caused a default sized mqueue to exceed a
non-root RLIMIT for message queue bytes.
C) My next patches brought defaults back down by decoupling defaults
from maximums.  This then broke some apps again that wanted to be able
to fiddle with the maximum settings in /proc and have that modify the
default value for an app that didn't pass an attr struct (fortunately,
all of these apps appear to be nothing more than regression test suites
at the moment where the people writing the suite used a combination of
setting the maximum size in proc, then running a test program that
created a default queue and checking it, then re-running with different
sizes in /proc, etc).
D) Motohiro and I argued for some time over this behavior until we both
agreed that apps can not be written to rely upon the maximum setting of
the system wide mqueue limits as their default queue size as this is
fundamentally unfriendly behavior to any multi-application OS (the
situation of two apps both needing specific, different queue sizes, and
both expecting to get them by default by twiddling values in /proc is
simply not supportable).  In the end, we agreed that the best approach
to solve the problem was to leave the defaults decoupled from the
maximums, but add a default knob in /proc so that if there are any apps
out there that have been coded to expect this behavior, then there is a
workaround path for them until they get coded to use an attr struct on
open instead of relying on values twiddled in /proc.  This is what
Motohiro's patch set does as well as a few other cleanups.

Hopefully that clears things up, and given the unsupportability of the
current design in Linus' kernel (changing maximums changes defaults for
all apps and encourages poor programming choices) we can move this
forward please.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[Andrew Morton]

On Wed, 14 Mar 2012 17:28:33 -0400
Doug Ledford <dledford@redhat.com> wrote:

> This has obviously fallen through the cracks.

It sure has.  Please dig out whatever is the currently favored
patchset, refresh, retest and resend, with changelogging which fully
covers the reasoning and decision process?

> so that if there are any apps out there that have been coded to expect
> this behavior, then there is a workaround path for them until they get
> coded

We'll be especially interested in the implications of this part.

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 939 bytes --]

On 03/14/2012 05:38 PM, Andrew Morton wrote:
> On Wed, 14 Mar 2012 17:28:33 -0400
> Doug Ledford <dledford@redhat.com> wrote:
> 
>> This has obviously fallen through the cracks.
> 
> It sure has.  Please dig out whatever is the currently favored
> patchset, refresh, retest and resend, with changelogging which fully
> covers the reasoning and decision process?

OK, completely redoing patch set then against current Linus tree.

Motohiro, would you be so kind as to resend my your patches that went on
top of mine and I'll create a complete patch set?

>> so that if there are any apps out there that have been coded to expect
>> this behavior, then there is a workaround path for them until they get
>> coded
> 
> We'll be especially interested in the implications of this part.

Certainly.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[KOSAKI Motohiro]

On 3/14/2012 5:45 PM, Doug Ledford wrote:
> On 03/14/2012 05:38 PM, Andrew Morton wrote:
>> On Wed, 14 Mar 2012 17:28:33 -0400
>> Doug Ledford <dledford@redhat.com> wrote:
>>
>>> This has obviously fallen through the cracks.
>>
>> It sure has.  Please dig out whatever is the currently favored
>> patchset, refresh, retest and resend, with changelogging which fully
>> covers the reasoning and decision process?
> 
> OK, completely redoing patch set then against current Linus tree.
> 
> Motohiro, would you be so kind as to resend my your patches that went on
> top of mine and I'll create a complete patch set?

I'm sorry. I already have a rebased patch. but I haven't posted. I have to
hurry.


> 
>>> so that if there are any apps out there that have been coded to expect
>>> this behavior, then there is a workaround path for them until they get
>>> coded
>>
>> We'll be especially interested in the implications of this part.
> 
> Certainly.
> 

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[Doug Ledford]

----- Original Message -----
> On 3/14/2012 5:45 PM, Doug Ledford wrote:
> > On 03/14/2012 05:38 PM, Andrew Morton wrote:
> >> On Wed, 14 Mar 2012 17:28:33 -0400
> >> Doug Ledford <dledford@redhat.com> wrote:
> >>
> >>> This has obviously fallen through the cracks.
> >>
> >> It sure has.  Please dig out whatever is the currently favored
> >> patchset, refresh, retest and resend, with changelogging which
> >> fully
> >> covers the reasoning and decision process?
> > 
> > OK, completely redoing patch set then against current Linus tree.
> > 
> > Motohiro, would you be so kind as to resend my your patches that
> > went on
> > top of mine and I'll create a complete patch set?
> 
> I'm sorry. I already have a rebased patch. but I haven't posted. I
> have to
> hurry.

Actually, I already pulled them from the lkml archives and fixed up
the one patch that didn't apply cleanly.  The only reason I haven't
sent the entire patchset out yet is that I'm writing a test/verification
app to go along with it.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD
	      http://people.redhat.com/dledford

Re: [resend][PATCH 1/3] mqueue: revert bump up DFLT_*MAX

[KOSAKI Motohiro]

On 3/19/2012 2:16 PM, Doug Ledford wrote:
> ----- Original Message -----
>> On 3/14/2012 5:45 PM, Doug Ledford wrote:
>>> On 03/14/2012 05:38 PM, Andrew Morton wrote:
>>>> On Wed, 14 Mar 2012 17:28:33 -0400
>>>> Doug Ledford <dledford@redhat.com> wrote:
>>>>
>>>>> This has obviously fallen through the cracks.
>>>>
>>>> It sure has.  Please dig out whatever is the currently favored
>>>> patchset, refresh, retest and resend, with changelogging which
>>>> fully
>>>> covers the reasoning and decision process?
>>>
>>> OK, completely redoing patch set then against current Linus tree.
>>>
>>> Motohiro, would you be so kind as to resend my your patches that
>>> went on
>>> top of mine and I'll create a complete patch set?
>>
>> I'm sorry. I already have a rebased patch. but I haven't posted. I
>> have toa
>> hurry.
> 
> Actually, I already pulled them from the lkml archives and fixed up
> the one patch that didn't apply cleanly.  The only reason I haven't
> sent the entire patchset out yet is that I'm writing a test/verification
> app to go along with it.

OK, I'll stop this work then. Good luck. :)

But, please rebase the patch on top linux-next. It's more akpm happy and
you are going to see more conflicts. That's the reason why I thought I
need more tests.