Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Minchan Kim <minchan.kim@gmail.com>
Date:   Tue, 23 Mar 2021 22:44:26 -0700
From:   Minchan Kim <minchan@kernel.org>
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>, gregkh@linuxfoundation.org,
        surenb@google.com, joaodias@google.com, willy@infradead.org,
        digetx@gmail.com
Subject: Re: [PATCH v6] mm: cma: support sysfs
Message-ID: <YFrRugjVLlazTNPy@google.com>
References: <20210324010547.4134370-1-minchan@kernel.org>
 <cbe10402-6574-6e46-9fd9-98b503bd26a4@nvidia.com>
 <YFqxm7UQBtWqH6VU@google.com>
 <d224c6bd-f5b1-74f6-2afc-c3d5b0519ba4@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d224c6bd-f5b1-74f6-2afc-c3d5b0519ba4@nvidia.com>
Precedence: bulk

On Tue, Mar 23, 2021 at 09:47:27PM -0700, John Hubbard wrote:
> On 3/23/21 8:27 PM, Minchan Kim wrote:
> ...
> > > > +static int __init cma_sysfs_init(void)
> > > > +{
> > > > +	unsigned int i;
> > > > +
> > > > +	cma_kobj_root = kobject_create_and_add("cma", mm_kobj);
> > > > +	if (!cma_kobj_root)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	for (i = 0; i < cma_area_count; i++) {
> > > > +		int err;
> > > > +		struct cma *cma;
> > > > +		struct cma_kobject *cma_kobj;
> > > > +
> > > > +		cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
> > > > +		if (!cma_kobj) {
> > > > +			kobject_put(cma_kobj_root);
> > > > +			return -ENOMEM;
> > > 
> > > This leaks little cma_kobj's all over the floor. :)
> > 
> > I thought kobject_put(cma_kobj_root) should deal with it. No?
> > 
> If this fails when i > 0, there will be cma_kobj instances that
> were stashed in the cma_areas[] array. But this code only deletes
> the most recently allocated cma_kobj, not anything allocated on
> previous iterations of the loop.

Oh, I misunderstood that destroying of root kobject will release
children recursively. Seems not true. Go back to old version.


index 16c81c9cb9b7..418951a3f138 100644
--- a/mm/cma_sysfs.c
+++ b/mm/cma_sysfs.c
@@ -80,20 +80,19 @@ static struct kobj_type cma_ktype = {
 static int __init cma_sysfs_init(void)
 {
        unsigned int i;
+       int err;
+       struct cma *cma;
+       struct cma_kobject *cma_kobj;

        cma_kobj_root = kobject_create_and_add("cma", mm_kobj);
        if (!cma_kobj_root)
                return -ENOMEM;

        for (i = 0; i < cma_area_count; i++) {
-               int err;
-               struct cma *cma;
-               struct cma_kobject *cma_kobj;
-
                cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
                if (!cma_kobj) {
-                       kobject_put(cma_kobj_root);
-                       return -ENOMEM;
+                       err = -ENOMEM;
+                       goto out;
                }

                cma = &cma_areas[i];
@@ -103,11 +102,21 @@ static int __init cma_sysfs_init(void)
                                           cma_kobj_root, "%s", cma->name);
                if (err) {
                        kobject_put(&cma_kobj->kobj);
-                       kobject_put(cma_kobj_root);
-                       return err;
+                       goto out;
                }
        }

        return 0;
+out:
+       while (--i >= 0) {
+               cma = &cma_areas[i];
+
+               kobject_put(&cma->kobj->kobj);
+               kfree(cma->kobj);
+               cma->kobj = NULL;
+       }
+       kobject_put(cma_kobj_root);
+
+       return err;
 }
 subsys_initcall(cma_sysfs_init);