RFR: 8253435: Cgroup: 'stomping of _mount_path' crash if manually mounted cpusets exist [v2]

[Bob Vandette]

On Fri, 25 Sep 2020 08:42:04 GMT, Volker Simonis <simonis at openjdk.org> wrote:

>> I was expecting to see some logic in this "else if" section that recorded the first occurance but did the validation on
>> the second pass (cg_infos[CPUSET_IDX]._mount_path != NULL).  When this situation is detected, we accept the mount with
>> the /sys/fs/cgroup.
>>         } else if (strcmp(token, "cpuset") == 0) {
>>           assert(cg_infos[CPUSET_IDX]._mount_path == NULL, "stomping of _mount_path");
>>           cg_infos[CPUSET_IDX]._mount_path = os::strdup(tmpmount);
>>           cg_infos[CPUSET_IDX]._root_mount_path = os::strdup(tmproot);
>>           cg_infos[CPUSET_IDX]._data_complete = true;
>
>> I was expecting to see some logic in this "else if" section that recorded the first occurance but did the validation on
>> the second pass (cg_infos[CPUSET_IDX]._mount_path != NULL). When this situation is detected, we accept the mount with
>> the /sys/fs/cgroup.  ```
>>     } else if (strcmp(token, "cpuset") == 0) {
>>       assert(cg_infos[CPUSET_IDX]._mount_path == NULL, "stomping of _mount_path");
>>       cg_infos[CPUSET_IDX]._mount_path = os::strdup(tmpmount);
>>       cg_infos[CPUSET_IDX]._root_mount_path = os::strdup(tmproot);
>>       cg_infos[CPUSET_IDX]._data_complete = true;
>> ```
> 
> You're right, the logic to ignore a `cpusets` entry should be moved into the `else if (strcmp(token, "cpuset") == 0)`
> and I've done that in the new version of my patch.
> However, the problem is that we don't know if the first or the second occurrence of `cpusets´ is *the right one*. I'm
> also not sure if the Cgroup controllers are ALWAYS mounted under `/sys/fs/cgroup` (see my answer to Severin's
> comments). If you can confirm that that's really the case, we could go with the simple solution proposed by Severin.
> Otherwise, my current solution tries to be conservative and does not assume a predefined mount point for Cgroup
> controllers. Instead it records the mount point of the first controller out of `cpu`, `cpuacct` and `memory` and checks
> that all the other controllers from this set are mounted under the same location. In addition, it only accepts
> `cpusets` if it is under the same mount point like the other controllers or under `/sys/fs/cgroup` if no other
> controller has been seen before.

In my suggestion, it doesn't matter which entry is first.  If we see the manual on first, record it.  When the second
one comes around, replace the first one if it's /sys/fs/cgroup.  In the very unlikely event that there isn't a second
non-manual one, I think we still want to record the manaul mount point since there could be a cpuset limit setup which
we should respect.

As for the second point about /sys/fs/cgroup, I think that using this string is just as good if not better than
assuming they are all mounted in the same subdirectory.

If you follow my suggestion, then we will only be subjected to a failure to mount if there are two cpuset mount entries
AND neither are mounted on /sys/fs/cgroup/cpuset.

-------------

PR: https://git.openjdk.java.net/jdk/pull/295