Re: [PATCH v4 4/6] ipc: Clamp msgmni and shmmni to the real IPCMNI limit

[Eric W. Biederman]

Waiman Long <longman@xxxxxxxxxx> writes:

> On 03/13/2018 02:17 PM, Eric W. Biederman wrote:
>> Waiman Long <longman@xxxxxxxxxx> writes:
>>
>>> 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.
>>>
>>> Enforcing the limit by failing the sysctl parameter write, however,
>>> can break existing user applications.
>> Which applications examples please.
>>
>> I am seeing this patchset late but it looks like a whole lot of changes
>> to avoid a theoretical possibility.
>>
>> Changes that have an impact on more than just the ipc code you are
>> patching.
>>
>> That makes me feel very uncomfortable with these changes.
>>
>> Eric
>
> This patchset is constructed to address a customer request that there is
> no easy way to find out the actual usable range of a sysctl parameter.
> In this particular case, the customer wants to use more than 32k shared
> memory segments. They can put in a large value into shmmni, but the
> application didn't work properly because shmmni was internally clamped
> to 32k without any visible sign that a smaller limit has been imposed.
>
> Out of a concern that there might be customers out there setting those
> sysctl parameters outside of the allowable range without knowing it,
> just enforcing the right limits may have the undesirable consequence of
> breaking their existing setup scripts. I don't have concrete example of
> what customers are doing that, but it  won't look good if we wait until
> the complaints come in.
>
> The new code won't affect existing code unless the necessary flag is
> set. So would you mind elaborating what other impact do you see that
> will affect other non-IPC code in an undesirable way?

The increase in size of struct ctl_table.  Every caller is affected.
Plus it increases everyone's cognitive load to figure out what is
this flags field as they fill out ctl_table.

Just introducing a proc_dointvec_minmax_clamped follows the existing
pattern and it makes it easier for everyone who both read the code.



It strikes me as quite peculiar that the response to bug report where
the complaint is an error is not given, is to continue the current
behavior without giving an error.


Arguably the simplest fix here would be to kill IPCMNI entirely.  Assign
the shmids from a sequence counter.  And place those structures in a
rbtree indexed by shmni.  There are 32bit fields but I don't think we
must keep the low 16bits for an index into an array and the high 16bits
as the actual sequence number.

Except for the checkpoint/restart case which is aguably much too
specific about how these ids are assigned that would give much more
freedom and allow people the number of shm segments that they actually
want to use.


For a further complication I don't expect you can get away with changing
the size or the fields in struct ctl_table in the kernel your customers
are running.


So please use a new function not flags it will simplify everyone's life.
If you can please actually fix this so you can have more shmids that
would be the really classy thing todo.

Eric