[PATCH] ipc: Limit sysctl value to IPCMNI

[PATCH] ipc: Limit sysctl value to IPCMNI

[Takashi Iwai]

Currently shmmni proc entry accepts all entered integer values, but
the practical limit is IPCMNI (32768).  This confuses user as if a
bigger value were accepted but not applied correctly.

This patch changes the proc entry to use *_minmax variant to limit the
accepted values accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 ipc/ipc_sysctl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index 8ad93c29f511..0c8f89f54e61 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
 static int zero;
 static int one = 1;
 static int int_max = INT_MAX;
+static int ipcmni = IPCMNI;
 
 static struct ctl_table ipc_kern_table[] = {
 	{
@@ -120,7 +121,9 @@ static struct ctl_table ipc_kern_table[] = {
 		.data		= &init_ipc_ns.shm_ctlmni,
 		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
 		.mode		= 0644,
-		.proc_handler	= proc_ipc_dointvec,
+		.proc_handler	= proc_ipc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &ipcmni,
 	},
 	{
 		.procname	= "shm_rmid_forced",
-- 
2.17.0

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Andrew Morton]

On Fri,  8 Jun 2018 15:49:49 +0200 Takashi Iwai <tiwai@suse.de> wrote:

> Currently shmmni proc entry accepts all entered integer values, but
> the practical limit is IPCMNI (32768).  This confuses user as if a
> bigger value were accepted but not applied correctly.
> 
> This patch changes the proc entry to use *_minmax variant to limit the
> accepted values accordingly.

Waiman Long was working on a (vastly more complicated) patchset to
address this.

> --- a/ipc/ipc_sysctl.c
> +++ b/ipc/ipc_sysctl.c
> @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>  static int zero;
>  static int one = 1;
>  static int int_max = INT_MAX;
> +static int ipcmni = IPCMNI;
>  
>  static struct ctl_table ipc_kern_table[] = {
>  	{
> @@ -120,7 +121,9 @@ static struct ctl_table ipc_kern_table[] = {
>  		.data		= &init_ipc_ns.shm_ctlmni,
>  		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
>  		.mode		= 0644,
> -		.proc_handler	= proc_ipc_dointvec,
> +		.proc_handler	= proc_ipc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &ipcmni,
>  	},
>  	{
>  		.procname	= "shm_rmid_forced",

What is the back-compatibility situation here?

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Takashi Iwai]

On Fri, 08 Jun 2018 23:16:59 +0200,
Andrew Morton wrote:
> 
> On Fri,  8 Jun 2018 15:49:49 +0200 Takashi Iwai <tiwai@suse.de> wrote:
> 
> > Currently shmmni proc entry accepts all entered integer values, but
> > the practical limit is IPCMNI (32768).  This confuses user as if a
> > bigger value were accepted but not applied correctly.
> > 
> > This patch changes the proc entry to use *_minmax variant to limit the
> > accepted values accordingly.
> 
> Waiman Long was working on a (vastly more complicated) patchset to
> address this.

That's great.  Any patch available for testing?


> > --- a/ipc/ipc_sysctl.c
> > +++ b/ipc/ipc_sysctl.c
> > @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
> >  static int zero;
> >  static int one = 1;
> >  static int int_max = INT_MAX;
> > +static int ipcmni = IPCMNI;
> >  
> >  static struct ctl_table ipc_kern_table[] = {
> >  	{
> > @@ -120,7 +121,9 @@ static struct ctl_table ipc_kern_table[] = {
> >  		.data		= &init_ipc_ns.shm_ctlmni,
> >  		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
> >  		.mode		= 0644,
> > -		.proc_handler	= proc_ipc_dointvec,
> > +		.proc_handler	= proc_ipc_dointvec_minmax,
> > +		.extra1		= &zero,
> > +		.extra2		= &ipcmni,
> >  	},
> >  	{
> >  		.procname	= "shm_rmid_forced",
> 
> What is the back-compatibility situation here?

It's obviously an error to set such a high value and suppose that it
were accepted.  So relying on that behavior must be broken in
anyway...


thanks,

Takashi

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Andrew Morton]

On Sat, 09 Jun 2018 08:48:48 +0200 Takashi Iwai <tiwai@suse.de> wrote:

> On Fri, 08 Jun 2018 23:16:59 +0200,
> Andrew Morton wrote:
> > 
> > On Fri,  8 Jun 2018 15:49:49 +0200 Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > > Currently shmmni proc entry accepts all entered integer values, but
> > > the practical limit is IPCMNI (32768).  This confuses user as if a
> > > bigger value were accepted but not applied correctly.
> > > 
> > > This patch changes the proc entry to use *_minmax variant to limit the
> > > accepted values accordingly.
> > 
> > Waiman Long was working on a (vastly more complicated) patchset to
> > address this.
> 
> That's great.  Any patch available for testing?

I think
http://lkml.kernel.org/r/1520885744-1546-1-git-send-email-longman@redhat.com
is the most recent version.

> 
> > > --- a/ipc/ipc_sysctl.c
> > > +++ b/ipc/ipc_sysctl.c
> > > @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
> > >  static int zero;
> > >  static int one = 1;
> > >  static int int_max = INT_MAX;
> > > +static int ipcmni = IPCMNI;
> > >  
> > >  static struct ctl_table ipc_kern_table[] = {
> > >  	{
> > > @@ -120,7 +121,9 @@ static struct ctl_table ipc_kern_table[] = {
> > >  		.data		= &init_ipc_ns.shm_ctlmni,
> > >  		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
> > >  		.mode		= 0644,
> > > -		.proc_handler	= proc_ipc_dointvec,
> > > +		.proc_handler	= proc_ipc_dointvec_minmax,
> > > +		.extra1		= &zero,
> > > +		.extra2		= &ipcmni,
> > >  	},
> > >  	{
> > >  		.procname	= "shm_rmid_forced",
> > 
> > What is the back-compatibility situation here?
> 
> It's obviously an error to set such a high value and suppose that it
> were accepted.  So relying on that behavior must be broken in
> anyway...

Well the present behaviour is to convert higher values downwards, yes?

int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
{
	kuid_t euid;
	kgid_t egid;
	int id, err;

	if (limit > IPCMNI)
		limit = IPCMNI;

So if someone out there is presently setting this to 999999 then their
kernel will work just fine.  After your proposed change, it will no
longer do so - the tuning attempt will fail with -EINVAL.

It really does us no good to say "you shouldn't have been doing that". 
The fact that they *are* doing it and that it works OK is the kernel
developers' fault for not applying suitable checking on day one.  I
think we're stuck with continuing to accept such input.

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Takashi Iwai]

On Tue, 12 Jun 2018 01:18:45 +0200,
Andrew Morton wrote:
> 
> On Sat, 09 Jun 2018 08:48:48 +0200 Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Fri, 08 Jun 2018 23:16:59 +0200,
> > Andrew Morton wrote:
> > > 
> > > On Fri,  8 Jun 2018 15:49:49 +0200 Takashi Iwai <tiwai@suse.de> wrote:
> > > 
> > > > Currently shmmni proc entry accepts all entered integer values, but
> > > > the practical limit is IPCMNI (32768).  This confuses user as if a
> > > > bigger value were accepted but not applied correctly.
> > > > 
> > > > This patch changes the proc entry to use *_minmax variant to limit the
> > > > accepted values accordingly.
> > > 
> > > Waiman Long was working on a (vastly more complicated) patchset to
> > > address this.
> > 
> > That's great.  Any patch available for testing?
> 
> I think
> http://lkml.kernel.org/r/1520885744-1546-1-git-send-email-longman@redhat.com
> is the most recent version.
> 
> > 
> > > > --- a/ipc/ipc_sysctl.c
> > > > +++ b/ipc/ipc_sysctl.c
> > > > @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
> > > >  static int zero;
> > > >  static int one = 1;
> > > >  static int int_max = INT_MAX;
> > > > +static int ipcmni = IPCMNI;
> > > >  
> > > >  static struct ctl_table ipc_kern_table[] = {
> > > >  	{
> > > > @@ -120,7 +121,9 @@ static struct ctl_table ipc_kern_table[] = {
> > > >  		.data		= &init_ipc_ns.shm_ctlmni,
> > > >  		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
> > > >  		.mode		= 0644,
> > > > -		.proc_handler	= proc_ipc_dointvec,
> > > > +		.proc_handler	= proc_ipc_dointvec_minmax,
> > > > +		.extra1		= &zero,
> > > > +		.extra2		= &ipcmni,
> > > >  	},
> > > >  	{
> > > >  		.procname	= "shm_rmid_forced",
> > > 
> > > What is the back-compatibility situation here?
> > 
> > It's obviously an error to set such a high value and suppose that it
> > were accepted.  So relying on that behavior must be broken in
> > anyway...
> 
> Well the present behaviour is to convert higher values downwards, yes?
> 
> int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
> {
> 	kuid_t euid;
> 	kgid_t egid;
> 	int id, err;
> 
> 	if (limit > IPCMNI)
> 		limit = IPCMNI;
> 
> So if someone out there is presently setting this to 999999 then their
> kernel will work just fine.  After your proposed change, it will no
> longer do so - the tuning attempt will fail with -EINVAL.
> 
> It really does us no good to say "you shouldn't have been doing that". 
> The fact that they *are* doing it and that it works OK is the kernel
> developers' fault for not applying suitable checking on day one.  I
> think we're stuck with continuing to accept such input.

Hm, that's one concern, yes.

OTOH, we do secretly ignore the input value, and this isn't what's
expected by user, either.  Moreover, user-space has no slightest idea
which value can be accepted and which not.

Actually I posted it just because of requests from customers who
needed to raise the bar, but didn't notice the effect.

Maybe another possible solution would be to add another proc entry to
handle this correctly, and make the old one only for compatibility.


thanks,

Takashi

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Andrew Morton]

On Tue, 12 Jun 2018 08:36:32 +0200 Takashi Iwai <tiwai@suse.de> wrote:

> > Well the present behaviour is to convert higher values downwards, yes?
> > 
> > int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
> > {
> > 	kuid_t euid;
> > 	kgid_t egid;
> > 	int id, err;
> > 
> > 	if (limit > IPCMNI)
> > 		limit = IPCMNI;
> > 
> > So if someone out there is presently setting this to 999999 then their
> > kernel will work just fine.  After your proposed change, it will no
> > longer do so - the tuning attempt will fail with -EINVAL.
> > 
> > It really does us no good to say "you shouldn't have been doing that". 
> > The fact that they *are* doing it and that it works OK is the kernel
> > developers' fault for not applying suitable checking on day one.  I
> > think we're stuck with continuing to accept such input.
> 
> Hm, that's one concern, yes.
> 
> OTOH, we do secretly ignore the input value, and this isn't what's
> expected by user, either.  Moreover, user-space has no slightest idea
> which value can be accepted and which not.
> 
> Actually I posted it just because of requests from customers who
> needed to raise the bar, but didn't notice the effect.
> 
> Maybe another possible solution would be to add another proc entry to
> handle this correctly, and make the old one only for compatibility.

Yes, I guess so.  Just leave the old tunable alone.  Possibly we
could add a printk_once("please switch to the new interface").

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Waiman Long]

On 06/13/2018 07:36 AM, Andrew Morton wrote:
> On Tue, 12 Jun 2018 08:36:32 +0200 Takashi Iwai <tiwai@suse.de> wrote:
>
>>> Well the present behaviour is to convert higher values downwards, yes?
>>>
>>> int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
>>> {
>>> 	kuid_t euid;
>>> 	kgid_t egid;
>>> 	int id, err;
>>>
>>> 	if (limit > IPCMNI)
>>> 		limit = IPCMNI;
>>>
>>> So if someone out there is presently setting this to 999999 then their
>>> kernel will work just fine.  After your proposed change, it will no
>>> longer do so - the tuning attempt will fail with -EINVAL.
>>>
>>> It really does us no good to say "you shouldn't have been doing that". 
>>> The fact that they *are* doing it and that it works OK is the kernel
>>> developers' fault for not applying suitable checking on day one.  I
>>> think we're stuck with continuing to accept such input.
>> Hm, that's one concern, yes.
>>
>> OTOH, we do secretly ignore the input value, and this isn't what's
>> expected by user, either.  Moreover, user-space has no slightest idea
>> which value can be accepted and which not.
>>
>> Actually I posted it just because of requests from customers who
>> needed to raise the bar, but didn't notice the effect.

My patchset actually contains patch to increase the IPC limit to over 32k.

>>
>> Maybe another possible solution would be to add another proc entry to
>> handle this correctly, and make the old one only for compatibility.
> Yes, I guess so.  Just leave the old tunable alone.  Possibly we
> could add a printk_once("please switch to the new interface").

A problem with that is the fact that those sysctl parameters have been
there for so long that it may be hard to persuade users to use the new
sysctl parameters. Earlier version of the patchset had added a clamping
minmax function to address that issue. However, it was deemed overkill
for adding quite a lot of code with not that much benefit.

Another alternative is to add a new sysctl parameter to turn on compat
mode where no error will be reported if it exceeds the limit just like
before. So a sysadmin can turn on compat mode if reporting range error
is an issue for existing applications.

Cheers,
Longman

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Andrew Morton]

On Mon, 18 Jun 2018 13:27:24 +0800 Waiman Long <longman@redhat.com> wrote:

> >>
> >> Maybe another possible solution would be to add another proc entry to
> >> handle this correctly, and make the old one only for compatibility.
> > Yes, I guess so.  Just leave the old tunable alone.  Possibly we
> > could add a printk_once("please switch to the new interface").
> 
> A problem with that is the fact that those sysctl parameters have been
> there for so long that it may be hard to persuade users to use the new
> sysctl parameters.

That's what a once-off warning could help with.  Tell them they're
using a deprecated interface.  It would take a few years, but when
people see the old interface is scheduled for removal, they'll switch,
over time.

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Waiman Long]

On 06/08/2018 05:16 PM, Andrew Morton wrote:
> On Fri,  8 Jun 2018 15:49:49 +0200 Takashi Iwai <tiwai@suse.de> wrote:
>
>> Currently shmmni proc entry accepts all entered integer values, but
>> the practical limit is IPCMNI (32768).  This confuses user as if a
>> bigger value were accepted but not applied correctly.
>>
>> This patch changes the proc entry to use *_minmax variant to limit the
>> accepted values accordingly.
> Waiman Long was working on a (vastly more complicated) patchset to
> address this.
>
>> --- a/ipc/ipc_sysctl.c
>> +++ b/ipc/ipc_sysctl.c
>> @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
>>  static int zero;
>>  static int one = 1;
>>  static int int_max = INT_MAX;
>> +static int ipcmni = IPCMNI;
>>  
>>  static struct ctl_table ipc_kern_table[] = {
>>  	{
>> @@ -120,7 +121,9 @@ static struct ctl_table ipc_kern_table[] = {
>>  		.data		= &init_ipc_ns.shm_ctlmni,
>>  		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
>>  		.mode		= 0644,
>> -		.proc_handler	= proc_ipc_dointvec,
>> +		.proc_handler	= proc_ipc_dointvec_minmax,
>> +		.extra1		= &zero,
>> +		.extra2		= &ipcmni,
>>  	},
>>  	{
>>  		.procname	= "shm_rmid_forced",
> What is the back-compatibility situation here?
>
>
Sorry for the late reply. I am planning to send out an updated patch
once the merge window is closed. The latest patch can be found in

https://lkml.org/lkml/2018/5/7/666

Luis has some concern about the use of __read_mostly tag which I am
going to remove in the next version.

-Longman

Re: [PATCH] ipc: Limit sysctl value to IPCMNI

[Takashi Iwai]

On Mon, 11 Jun 2018 23:27:08 +0200,
Waiman Long wrote:
> 
> On 06/08/2018 05:16 PM, Andrew Morton wrote:
> > On Fri,  8 Jun 2018 15:49:49 +0200 Takashi Iwai <tiwai@suse.de> wrote:
> >
> >> Currently shmmni proc entry accepts all entered integer values, but
> >> the practical limit is IPCMNI (32768).  This confuses user as if a
> >> bigger value were accepted but not applied correctly.
> >>
> >> This patch changes the proc entry to use *_minmax variant to limit the
> >> accepted values accordingly.
> > Waiman Long was working on a (vastly more complicated) patchset to
> > address this.
> >
> >> --- a/ipc/ipc_sysctl.c
> >> +++ b/ipc/ipc_sysctl.c
> >> @@ -99,6 +99,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write,
> >>  static int zero;
> >>  static int one = 1;
> >>  static int int_max = INT_MAX;
> >> +static int ipcmni = IPCMNI;
> >>  
> >>  static struct ctl_table ipc_kern_table[] = {
> >>  	{
> >> @@ -120,7 +121,9 @@ static struct ctl_table ipc_kern_table[] = {
> >>  		.data		= &init_ipc_ns.shm_ctlmni,
> >>  		.maxlen		= sizeof(init_ipc_ns.shm_ctlmni),
> >>  		.mode		= 0644,
> >> -		.proc_handler	= proc_ipc_dointvec,
> >> +		.proc_handler	= proc_ipc_dointvec_minmax,
> >> +		.extra1		= &zero,
> >> +		.extra2		= &ipcmni,
> >>  	},
> >>  	{
> >>  		.procname	= "shm_rmid_forced",
> > What is the back-compatibility situation here?
> >
> >
> Sorry for the late reply. I am planning to send out an updated patch
> once the merge window is closed. The latest patch can be found in
> 
> https://lkml.org/lkml/2018/5/7/666
> 
> Luis has some concern about the use of __read_mostly tag which I am
> going to remove in the next version.

Thanks, that's as trivial as my patch, unsurprisingly :)


Takashi