[PATCH] ipc: always handle a new value of auto_msgmni

[PATCH] ipc: always handle a new value of auto_msgmni

[Andrey Vagin]

proc_dointvec_minmax() returns zero, if a new value has been set.
So we don't need to check all charectes have been handled.

Below you can find two examples. In the new value has not been
handled properly.

$ strace ./a.out
open("/proc/sys/kernel/auto_msgmni", O_WRONLY) = 3
write(3, "0\n\0", 3)                    = 2
close(3)                                = 0
exit_group(0)
$ cat /sys/kernel/debug/tracing/trace

$strace ./a.out
open("/proc/sys/kernel/auto_msgmni", O_WRONLY) = 3
write(3, "0\n", 2)                      = 2
close(3)                                = 0

$ cat /sys/kernel/debug/tracing/trace
a.out-697   [000] ....  3280.998235: unregister_ipcns_notifier <-proc_ipcauto_dointvec_minmax

Fixes: 9eefe520c814 ("ipc: do not use a negative value to re-enable msgmni automatic recomputin")
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mathias Krause <minipli@googlemail.com>
Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: Joe Perches <joe@perches.com>
Cc: Davidlohr Bueso <davidlohr@hp.com>
Signed-off-by: Andrey Vagin <avagin@openvz.org>
---
 ipc/ipc_sysctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index c3f0326..e8075b2 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -123,7 +123,6 @@ static int proc_ipcauto_dointvec_minmax(struct ctl_table *table, int write,
 	void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	struct ctl_table ipc_table;
-	size_t lenp_bef = *lenp;
 	int oldval;
 	int rc;
 
@@ -133,7 +132,7 @@ static int proc_ipcauto_dointvec_minmax(struct ctl_table *table, int write,
 
 	rc = proc_dointvec_minmax(&ipc_table, write, buffer, lenp, ppos);
 
-	if (write && !rc && lenp_bef == *lenp) {
+	if (write && !rc) {
 		int newval = *((int *)(ipc_table.data));
 		/*
 		 * The file "auto_msgmni" has correctly been set.
-- 
1.9.3

Re: [PATCH] ipc: always handle a new value of auto_msgmni

[Manfred Spraul]

Hi Andrey,

On 08/13/2014 03:21 PM, Andrey Vagin wrote:
> proc_dointvec_minmax() returns zero, if a new value has been set.
> So we don't need to check all charectes have been handled.
What do you use auto_msgmni for?

I would propose to remove the whole logic - just always allow 32000 
message queues.

--
     Manfred

Re: [PATCH] ipc: always handle a new value of auto_msgmni

[Andrew Vagin]

On Thu, Aug 14, 2014 at 11:37:45AM +0200, Manfred Spraul wrote:
> Hi Andrey,
> 
> On 08/13/2014 03:21 PM, Andrey Vagin wrote:
> >proc_dointvec_minmax() returns zero, if a new value has been set.
> >So we don't need to check all charectes have been handled.
> What do you use auto_msgmni for?

We disable it to check that criu restores a value of the msgmni sysctl correctly.

https://github.com/xemul/criu/blob/master/test/zdtm/live/static/ipc_namespace.c

> 
> I would propose to remove the whole logic - just always allow 32000 message
> queues.

I don't have objections.

> 
> --
>     Manfred

Re: [PATCH] ipc: always handle a new value of auto_msgmni

[Manfred Spraul]

Hi Andrew,

On 08/14/2014 03:34 PM, Andrew Vagin wrote:
> On Thu, Aug 14, 2014 at 11:37:45AM +0200, Manfred Spraul wrote:
>> Hi Andrey,
>>
>> [...]
>> What do you use auto_msgmni for? 
> We disable it to check that criu restores a value of the msgmni sysctl correctly.
>
> https://github.com/xemul/criu/blob/master/test/zdtm/live/static/ipc_namespace.c
Thanks for the link, if I see it right, my patch would break your code:

http://marc.info/?l=linux-kernel&m=140782863804950

It removes auto_msgmni - and
What do you think, should I leave auto_msgmni as a stale variable, 
perhaps with a pr_info() that it doesn't have any effect?


--
     Manfred

Re: [PATCH] ipc: always handle a new value of auto_msgmni

[Andrew Vagin]

On Sat, Aug 16, 2014 at 10:29:16PM +0200, Manfred Spraul wrote:
> Hi Andrew,
> 
> On 08/14/2014 03:34 PM, Andrew Vagin wrote:
> >On Thu, Aug 14, 2014 at 11:37:45AM +0200, Manfred Spraul wrote:
> >>Hi Andrey,
> >>
> >>[...]
> >>What do you use auto_msgmni for?
> >We disable it to check that criu restores a value of the msgmni sysctl correctly.
> >
> >https://github.com/xemul/criu/blob/master/test/zdtm/live/static/ipc_namespace.c
> Thanks for the link, if I see it right, my patch would break your code:
> 
> http://marc.info/?l=linux-kernel&m=140782863804950
> 
> It removes auto_msgmni - and
> What do you think, should I leave auto_msgmni as a stale variable, perhaps
> with a pr_info() that it doesn't have any effect?

Yes, you should. For example "criu restore" will start failing, if you remove
auto_msgmni.
> 
> 
> --
>     Manfred