On Fri, 14 Mar 2025 10:55:00 +0000 Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> On Thu, 13 Mar 2025 11:52:18 -0400
> Gregory Price <gourry@xxxxxxxxxx> wrote:
>
> > On Thu, Mar 13, 2025 at 03:31:38PM +0900, Rakie Kim wrote:
> > > > Is this correct? If kobject_init_and_add fails, from other examples we
> > > > need only free the mempolicy_kobj - because it failed to initialize and
> > > > therefore should not have any references. I think this causes an
> > > > underflow.
> > >
> > > Regarding the reordering of mempolicy_kobj allocation:
> > > 1) In kobject_init_and_add(), kobject_init() is always called, which
> >
> > Quite right, mea culpa.
> >
> > >
> > > 2) The release function for mempolicy_kobj is responsible for freeing
> > > associated memory:
> > >
> > > static void mempolicy_kobj_release(struct kobject *kobj)
> > > {
> > > ...
> > > kfree(ngrp->nattrs);
> > > kfree(ngrp);
> > > kfree(kobj);
> > > }
> > >
> >
> > I see what you're trying to do now after looking at the free-ordering
> > at little closer.
> >
> > Lets do the following:
> >
> > 1) allocate node_attrs and mempolicy_kobj up front and keep your
> > reordering, this lets us clean up allocations on failure before
> > kobject_init is called
> >
> > 2) after this remove all the other code and just let
> > mempolicy_kobj_release clean up node_attrs
> >
> > 3) Add a (%d) to the error message to differentiate failures
>
> Given how unlikely (and noisy) a memory allocation failure is,
> maybe just drop the printing at all in those paths - allowing
> early returns.
>
> The lifetime rules around node_attrs in here are making readability
> poor. It is implicitly owned by the mempolicy_kobj, but no direct association.
> Maybe just encapsulating the kobject in a structure that contains
> this as a [] array at the end. Then we end up with single allocation of
> stuff that is effectively one thing.
>
Hi Jonathan
Thank you for your response regarding this patch.
Your suggestions seem very appropriate. As you recommended, I will proceed to
encapsulate node_attrs and mempolicy_kobj into a single structure.
Rakie
>
> >
> > This is a little bit cleaner and is a bit less code. (Not built or
> > tested, just a recommendation).
> >
> > I'd recommend submitting this patch by itself to mm-stable, since the
> > remainder of the patch line changes functionality and this fixes a bug
> > in LTS kernels.
> >
> > ~Gregory
> >
> > ---
> >
> >
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index 530e71fe9147..05a410db08b4 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -3541,38 +3541,34 @@ static int __init mempolicy_sysfs_init(void)
> > int err;
> > static struct kobject *mempolicy_kobj;
> >
> > - mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
> > - if (!mempolicy_kobj) {
> > + node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
> > + GFP_KERNEL);
> > + if (!node_attrs) {
> > err = -ENOMEM;
> > goto err_out;
> > }
> >
> > - node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
> > - GFP_KERNEL);
> > - if (!node_attrs) {
> > + mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
> > + if (!mempolicy_kobj) {
> > err = -ENOMEM;
> > - goto mempol_out;
> > + kfree(node_attrs);
> > + goto err_out;
> > }
> >
> > err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
> > "mempolicy");
> > if (err)
> > - goto node_out;
> > + goto mempol_out;
> >
> > err = add_weighted_interleave_group(mempolicy_kobj);
> > - if (err) {
> > - pr_err("mempolicy sysfs structure failed to initialize\n");
> > - kobject_put(mempolicy_kobj);
> > - return err;
> > - }
> > + if (err)
> > + goto mempol_out;
> >
> > - return err;
> > -node_out:
> > - kfree(node_attrs);
> > + return 0;
> > mempol_out:
> > - kfree(mempolicy_kobj);
> > + kobject_put(mempolicy_kobj);
> > err_out:
> > - pr_err("failed to add mempolicy kobject to the system\n");
> > + pr_err("mempolicy sysfs structure failed to initialize (%d)\n", err);
> > return err;
> > }
> >
> >
>
