| Message ID | 1525726752-29281-2-git-send-email-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 07, 2018 at 04:59:09PM -0400, Waiman Long wrote: > A user can write arbitrary integer values to msgmni and shmmni sysctl > parameters without getting error, but the actual limit is really > IPCMNI (32k). This can mislead users as they think they can get a > value that is not real. > > The right limits are now set for msgmni and shmmni so that the users > will become aware if they set a value outside of the acceptable range. > > Signed-off-by: Waiman Long <longman@redhat.com> > --- > ipc/ipc_sysctl.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c > index 8ad93c2..f87cb29 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 ipc_mni = IPCMNI; > > static struct ctl_table ipc_kern_table[] = { > { > @@ -120,7 +121,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, > .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 = &ipc_mni, > }, > { > .procname = "shm_rmid_forced", > @@ -147,7 +150,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, > .mode = 0644, > .proc_handler = proc_ipc_dointvec_minmax, > .extra1 = &zero, > - .extra2 = &int_max, > + .extra2 = &ipc_mni, > }, > { > .procname = "auto_msgmni", > -- > 1.8.3.1 It seems negative values are not allowed, if true then having a caller to use proc_douintvec_minmax() would help with ensuring no invalid negative input values are used as well. Luis
On 05/07/2018 06:39 PM, Luis R. Rodriguez wrote: > On Mon, May 07, 2018 at 04:59:09PM -0400, Waiman Long wrote: >> A user can write arbitrary integer values to msgmni and shmmni sysctl >> parameters without getting error, but the actual limit is really >> IPCMNI (32k). This can mislead users as they think they can get a >> value that is not real. >> >> The right limits are now set for msgmni and shmmni so that the users >> will become aware if they set a value outside of the acceptable range. >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> ipc/ipc_sysctl.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c >> index 8ad93c2..f87cb29 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 ipc_mni = IPCMNI; >> >> static struct ctl_table ipc_kern_table[] = { >> { >> @@ -120,7 +121,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, >> .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 = &ipc_mni, >> }, >> { >> .procname = "shm_rmid_forced", >> @@ -147,7 +150,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, >> .mode = 0644, >> .proc_handler = proc_ipc_dointvec_minmax, >> .extra1 = &zero, >> - .extra2 = &int_max, >> + .extra2 = &ipc_mni, >> }, >> { >> .procname = "auto_msgmni", >> -- >> 1.8.3.1 > It seems negative values are not allowed, if true then having > a caller to use proc_douintvec_minmax() would help with ensuring > no invalid negative input values are used as well. > > Luis Negative value doesn't mean sense here. So it is true that we can use proc_douintvec_minmax() instead. However, the data types themselves are defined as "int". So I think it is better to keep using proc_dointvec_minmax() to be consistent with the data type. Cheers, Longman
On Mon, May 07, 2018 at 07:57:12PM -0400, Waiman Long wrote: > On 05/07/2018 06:39 PM, Luis R. Rodriguez wrote: > > On Mon, May 07, 2018 at 04:59:09PM -0400, Waiman Long wrote: > >> A user can write arbitrary integer values to msgmni and shmmni sysctl > >> parameters without getting error, but the actual limit is really > >> IPCMNI (32k). This can mislead users as they think they can get a > >> value that is not real. > >> > >> The right limits are now set for msgmni and shmmni so that the users > >> will become aware if they set a value outside of the acceptable range. > >> > >> Signed-off-by: Waiman Long <longman@redhat.com> > >> --- > >> ipc/ipc_sysctl.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c > >> index 8ad93c2..f87cb29 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 ipc_mni = IPCMNI; > >> > >> static struct ctl_table ipc_kern_table[] = { > >> { > >> @@ -120,7 +121,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, > >> .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 = &ipc_mni, > >> }, > >> { > >> .procname = "shm_rmid_forced", > >> @@ -147,7 +150,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, > >> .mode = 0644, > >> .proc_handler = proc_ipc_dointvec_minmax, > >> .extra1 = &zero, > >> - .extra2 = &int_max, > >> + .extra2 = &ipc_mni, > >> }, > >> { > >> .procname = "auto_msgmni", > >> -- > >> 1.8.3.1 > > It seems negative values are not allowed, if true then having > > a caller to use proc_douintvec_minmax() would help with ensuring > > no invalid negative input values are used as well. > > > > Luis > > Negative value doesn't mean sense here. So it is true that we can use > proc_douintvec_minmax() instead. However, the data types themselves are > defined as "int". So I think it is better to keep using > proc_dointvec_minmax() to be consistent with the data type. Huh, no... If you *know* the valid values *are* only positive, the right thing to do is to then *change* the data type. Tons of odd bugs can creep up because of these stupid things. Luis
On 05/10/2018 03:32 AM, Luis R. Rodriguez wrote: > On Mon, May 07, 2018 at 07:57:12PM -0400, Waiman Long wrote: >> On 05/07/2018 06:39 PM, Luis R. Rodriguez wrote: >>> On Mon, May 07, 2018 at 04:59:09PM -0400, Waiman Long wrote: >>>> A user can write arbitrary integer values to msgmni and shmmni sysctl >>>> parameters without getting error, but the actual limit is really >>>> IPCMNI (32k). This can mislead users as they think they can get a >>>> value that is not real. >>>> >>>> The right limits are now set for msgmni and shmmni so that the users >>>> will become aware if they set a value outside of the acceptable range. >>>> >>>> Signed-off-by: Waiman Long <longman@redhat.com> >>>> --- >>>> ipc/ipc_sysctl.c | 7 +++++-- >>>> 1 file changed, 5 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c >>>> index 8ad93c2..f87cb29 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 ipc_mni = IPCMNI; >>>> >>>> static struct ctl_table ipc_kern_table[] = { >>>> { >>>> @@ -120,7 +121,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, >>>> .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 = &ipc_mni, >>>> }, >>>> { >>>> .procname = "shm_rmid_forced", >>>> @@ -147,7 +150,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, >>>> .mode = 0644, >>>> .proc_handler = proc_ipc_dointvec_minmax, >>>> .extra1 = &zero, >>>> - .extra2 = &int_max, >>>> + .extra2 = &ipc_mni, >>>> }, >>>> { >>>> .procname = "auto_msgmni", >>>> -- >>>> 1.8.3.1 >>> It seems negative values are not allowed, if true then having >>> a caller to use proc_douintvec_Fminmax() would help with ensuring >>> no invalid negative input values are used as well. >>> >>> Luis >> Negative value doesn't mean sense here. So it is true that we can use >> proc_douintvec_minmax() instead. However, the data types themselves are >> defined as "int". So I think it is better to keep using >> proc_dointvec_minmax() to be consistent with the data type. > Huh, no... If you *know* the valid values *are* only positive, the right > thing to do is to then *change* the data type. Tons of odd bugs can creep > up because of these stupid things. > > Luis Sorry for the late reply. First of all, negative value will not be accepted because of the zero lower limit check. The type of msgmni, shmmni and semmni are defined as int in the uapi/linux/msg.h and uapi/linux/shm.h and uapi/linux/sem.h. They are exposed to the userspace and changing them to "unsigned int" may cause some undesirable consequence. Again this is a case of introducing risk without any noticeable benefit. I understand your desire of cleaning thing up. However, I am hesitant to take this risk without seeing any real benefit in this case. Cheers, Longman
diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c index 8ad93c2..f87cb29 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 ipc_mni = IPCMNI; static struct ctl_table ipc_kern_table[] = { { @@ -120,7 +121,9 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, .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 = &ipc_mni, }, { .procname = "shm_rmid_forced", @@ -147,7 +150,7 @@ static int proc_ipc_auto_msgmni(struct ctl_table *table, int write, .mode = 0644, .proc_handler = proc_ipc_dointvec_minmax, .extra1 = &zero, - .extra2 = &int_max, + .extra2 = &ipc_mni, }, { .procname = "auto_msgmni",
A user can write arbitrary integer values to msgmni and shmmni sysctl parameters without getting error, but the actual limit is really IPCMNI (32k). This can mislead users as they think they can get a value that is not real. The right limits are now set for msgmni and shmmni so that the users will become aware if they set a value outside of the acceptable range. Signed-off-by: Waiman Long <longman@redhat.com> --- ipc/ipc_sysctl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)