| Message ID | 20231025110533.6516-1-rpalethorpe@suse.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cgroup: Handle trailing new line in cgroup.controllers | expand |
Am Mittwoch, 25. Oktober 2023, 13:05:33 CEST schrieb Richard Palethorpe via ltp: > + switch (ctrl_name[l]) { > + case '\n': break; > + case '\0': break; > + default: > + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, ctrl_name[l]); I'm wondering whether that's a bit too restrictive. Or is there any official documentation says that you really can only have the letters a-z in cgroup names (and not even A-Z). Otherwise it might be better to make this just a warning or allow any printable characters.
Hi Richie, Martchus,
@Richie, please add Fixes: tag when commit
I suppose it should be
Fixes: 310da3784 ("Add new CGroups APIs")
but please check yourself.
Why this is useful? It helps to identify which test failures were false
positives. Also, you actually not just fix a line character, but also do other
validation, it would be worth to mention that.
> Am Mittwoch, 25. Oktober 2023, 13:05:33 CEST schrieb Richard Palethorpe via
> ltp:
> > + switch (ctrl_name[l]) {
> > + case '\n': break;
> > + case '\0': break;
> > + default:
> > + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name,
> ctrl_name[l]);
> I'm wondering whether that's a bit too restrictive. Or is there any official
> documentation says that you really can only have the letters a-z in cgroup
> names (and not even A-Z). Otherwise it might be better to make this just a
> warning or allow any printable characters.
I guess for cgroup v1 [1]
The name should match [\w.-]+
\w Matches a "word" character (alphanumeric plus "_", plus other connector
punctuation chars plus Unicode marks). Also '.' and '-' can be used.
=> [A-Z.-] and others are valid names in v1. Although I'm not sure if
cgroup_find_ctrl() is used on systems with cgroups v1.
Also, shouldn't we check with MAX_CGROUP_TYPE_NAMELEN:
- name: should be initialized to a unique subsystem name. Should be
no longer than MAX_CGROUP_TYPE_NAMELEN.
For cgroup v2 [2] it looks to be:
All cgroup core interface files are prefixed with "cgroup." and each
controller's interface files are prefixed with the controller name and a
dot. A controller's name is composed of lower case alphabets and '_'s but
never begins with an '_' so it can be used as the prefix character for
collision avoidance. Also, interface file names won't start or end with
terms which are often used in categorizing workloads such as job, service,
slice, unit or workload.
=> It matches ^[a-z][a-z_]. At least "_" is missing. Also this validation should
specify somewhere if it's for v2 only or for both.
Kind regards,
Petr
[1] https://www.kernel.org/doc/Documentation/cgroup-v1/cgroups.txt
[2] https://docs.kernel.org/admin-guide/cgroup-v2.html#avoid-name-collisions
Hello, Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, Martchus, > > @Richie, please add Fixes: tag when commit > > I suppose it should be > Fixes: 310da3784 ("Add new CGroups APIs") > but please check yourself. Yes that is right. > > Why this is useful? It helps to identify which test failures were false > positives. Also, you actually not just fix a line character, but also do other > validation, it would be worth to mention that. The validation is primarily checking my assumptions. We don't want to cut the name off at a '_' then get more confusing errors in the future. > > >> Am Mittwoch, 25. Oktober 2023, 13:05:33 CEST schrieb Richard Palethorpe via >> ltp: >> > + switch (ctrl_name[l]) { >> > + case '\n': break; >> > + case '\0': break; >> > + default: >> > + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, >> ctrl_name[l]); > >> I'm wondering whether that's a bit too restrictive. Or is there any official >> documentation says that you really can only have the letters a-z in cgroup >> names (and not even A-Z). Otherwise it might be better to make this just a >> warning or allow any printable characters. Well I assumed there wasn't, but it seems this was actually thought about and specified to some extent. I should have scanned the docs. > > I guess for cgroup v1 [1] > > The name should match [\w.-]+ Thats the name of the "hierarchy" AFAICT. I don't think that is the controller/subsystem name. Those characters would be a pain for naming things in C. > > \w Matches a "word" character (alphanumeric plus "_", plus other connector > punctuation chars plus Unicode marks). Also '.' and '-' can be used. > => [A-Z.-] and others are valid names in v1. Although I'm not sure if > cgroup_find_ctrl() is used on systems with cgroups v1. None of the existing upstream controllers contain a _ unless we are missing one or more. However we should allow it, so I'll add it. > > Also, shouldn't we check with MAX_CGROUP_TYPE_NAMELEN: > > - name: should be initialized to a unique subsystem name. Should be > no longer than MAX_CGROUP_TYPE_NAMELEN. > It's not exposed to userland or specified in the docs. I suppose I could issue a WARN if it is over 32 though. Mostly likely if that happens then there is a parsing error. The kernel will also issue a WARN if the subsystem name is over 32. > For cgroup v2 [2] it looks to be: > > All cgroup core interface files are prefixed with "cgroup." and each > controller's interface files are prefixed with the controller name and a > dot. A controller's name is composed of lower case alphabets and '_'s but > never begins with an '_' so it can be used as the prefix character for > collision avoidance. Also, interface file names won't start or end with > terms which are often used in categorizing workloads such as job, service, > slice, unit or workload. > > => It matches ^[a-z][a-z_]. At least "_" is missing. Also this validation should > specify somewhere if it's for v2 only or for both. > > Kind regards, > Petr > > [1] https://www.kernel.org/doc/Documentation/cgroup-v1/cgroups.txt > [2] https://docs.kernel.org/admin-guide/cgroup-v2.html#avoid-name-collisions
> Hello, > Petr Vorel <pvorel@suse.cz> writes: > > Hi Richie, Martchus, > > @Richie, please add Fixes: tag when commit > > I suppose it should be > > Fixes: 310da3784 ("Add new CGroups APIs") > > but please check yourself. > Yes that is right. +1 > > Why this is useful? It helps to identify which test failures were false > > positives. Also, you actually not just fix a line character, but also do other > > validation, it would be worth to mention that. > The validation is primarily checking my assumptions. We don't want to > cut the name off at a '_' then get more confusing errors in the future. +1, looking into the code and description once more, it makes sense. > >> Am Mittwoch, 25. Oktober 2023, 13:05:33 CEST schrieb Richard Palethorpe via > >> ltp: > >> > + switch (ctrl_name[l]) { > >> > + case '\n': break; > >> > + case '\0': break; > >> > + default: > >> > + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, > >> ctrl_name[l]); > >> I'm wondering whether that's a bit too restrictive. Or is there any official > >> documentation says that you really can only have the letters a-z in cgroup > >> names (and not even A-Z). Otherwise it might be better to make this just a > >> warning or allow any printable characters. > Well I assumed there wasn't, but it seems this was actually thought > about and specified to some extent. I should have scanned the docs. > > I guess for cgroup v1 [1] > > The name should match [\w.-]+ > Thats the name of the "hierarchy" AFAICT. I don't think that is the > controller/subsystem name. Those characters would be a pain for naming > things in C. Ah, right, I'm sorry. > > \w Matches a "word" character (alphanumeric plus "_", plus other connector > > punctuation chars plus Unicode marks). Also '.' and '-' can be used. > > => [A-Z.-] and others are valid names in v1. Although I'm not sure if > > cgroup_find_ctrl() is used on systems with cgroups v1. > None of the existing upstream controllers contain a _ unless we are > missing one or more. However we should allow it, so I'll add it. +1 > > Also, shouldn't we check with MAX_CGROUP_TYPE_NAMELEN: > > - name: should be initialized to a unique subsystem name. Should be > > no longer than MAX_CGROUP_TYPE_NAMELEN. > It's not exposed to userland or specified in the docs. I suppose I could > issue a WARN if it is over 32 though. Mostly likely if that happens then > there is a parsing error. +1 > The kernel will also issue a WARN if the subsystem name is over 32. Good. But warning in LTP will be more visible (not everybody parses dmesg). Kind regards, Petr > > For cgroup v2 [2] it looks to be: > > All cgroup core interface files are prefixed with "cgroup." and each > > controller's interface files are prefixed with the controller name and a > > dot. A controller's name is composed of lower case alphabets and '_'s but > > never begins with an '_' so it can be used as the prefix character for > > collision avoidance. Also, interface file names won't start or end with > > terms which are often used in categorizing workloads such as job, service, > > slice, unit or workload. > > => It matches ^[a-z][a-z_]. At least "_" is missing. Also this validation should > > specify somewhere if it's for v2 only or for both. > > Kind regards, > > Petr > > [1] https://www.kernel.org/doc/Documentation/cgroup-v1/cgroups.txt > > [2] https://docs.kernel.org/admin-guide/cgroup-v2.html#avoid-name-collisions
diff --git a/lib/tst_cgroup.c b/lib/tst_cgroup.c index 5240aadaa..d70b5431d 100644 --- a/lib/tst_cgroup.c +++ b/lib/tst_cgroup.c @@ -433,9 +433,20 @@ __attribute__ ((nonnull, warn_unused_result)) static struct cgroup_ctrl *cgroup_find_ctrl(const char *const ctrl_name) { struct cgroup_ctrl *ctrl; + int l = 0; + + while (ctrl_name[l] >= 'a' && ctrl_name[l] <= 'z') + l++; + + switch (ctrl_name[l]) { + case '\n': break; + case '\0': break; + default: + tst_brk(TBROK, "Unexpected char in %s: %c", ctrl_name, ctrl_name[l]); + } for_each_ctrl(ctrl) { - if (!strcmp(ctrl_name, ctrl->ctrl_name)) + if (!strncmp(ctrl_name, ctrl->ctrl_name, l)) return ctrl; }
The last item in cgroup.controllers (misc in my case) contained a new line character which caused the controller search to fail. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- This stops cgroup_regression_test.sh from failing on Tumbleweed. However there are other issues with that test which are not addressed by this patch. lib/tst_cgroup.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)