On Tue, 26 Sep 2023 08:13:18 -0500, Jarkko Sakkinen <jarkko@xxxxxxxxxx>
wrote:
...
> >> /**
> >> @@ -410,7 +429,14 @@ misc_cg_alloc(struct cgroup_subsys_state
> >> *parent_css)
> >> */
> >> static void misc_cg_free(struct cgroup_subsys_state *css)
> >> {
> >> - kfree(css_misc(css));
> >> + struct misc_cg *cg = css_misc(css);
> >> + enum misc_res_type i;
> >> +
> >> + for (i = 0; i < MISC_CG_RES_TYPES; i++)
> >> + if (cg->res[i].free)
> >> + cg->res[i].free(cg);
> >> +
> >> + kfree(cg);
> >> }
> >>
> >> /* Cgroup controller callbacks */
> >> --
> >> 2.25.1
> >
> > Since the only existing client feature requires all callbacks,
should
> > this not have that as an invariant?
> >
> > I.e. it might be better to fail unless *all* ops are non-nil (e.g.
to
> > catch issues in the kernel code).
> >
>
> These callbacks are chained from cgroup_subsys, and they are defined
> separately so it'd be better follow the same pattern. Or change
together
> with cgroup_subsys if we want to do that. Reasonable?
I noticed this one later:
It would better to create a separate ops struct and declare the instance
as const at minimum.
Then there is no need for dynamic assigment of ops and all that is in
rodata. This is improves both security and also allows static analysis
bit better.
Now you have to dynamically trace the struct instance, e.g. in case of
a bug. If this one done, it would be already in the vmlinux.
I.e. then in the driver you can have static const struct declaration
with *all* pointers pre-assigned.
Not sure if cgroups follows this or not but it is *objectively*
better. Previous work is not always best possible work...
IIUC, like vm_ops field in vma structs. Although function pointers in
vm_ops are assigned statically, but you still need dynamically assign
vm_ops for each instance of vma.
So the code will look like this:
if (parent_cg->res[i].misc_ops && parent_cg->res[i].misc_ops->alloc)
{
...
}
I don't see this is the pattern used in cgroups and no strong opinion
either way.
TJ, do you have preference on this?
Thanks
Haitao
