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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxx
> 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