linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
@ 2021-03-24 19:20 Minchan Kim
  2021-03-24 19:43 ` Dmitry Osipenko
  2021-03-24 19:45 ` John Hubbard
  0 siblings, 2 replies; 10+ messages in thread
From: Minchan Kim @ 2021-03-24 19:20 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy,
	digetx, Minchan Kim

struct cma_stat's lifespan for cma_sysfs is different with
struct cma because kobject for sysfs requires dynamic object
while CMA is static object[1]. When CMA is initialized,
it couldn't use slab to allocate cma_stat since slab was not
initialized yet. Thus, it allocates the dynamic object
in subsys_initcall.

However, the cma allocation can happens before subsys_initcall
then, it goes crash.

Dmitry reported[2]:

..
[    1.226190] [<c027762f>] (cma_sysfs_alloc_pages_count) from [<c027706f>] (cma_alloc+0x153/0x274)
[    1.226720] [<c027706f>] (cma_alloc) from [<c01112ab>] (__alloc_from_contiguous+0x37/0x8c)
[    1.227272] [<c01112ab>] (__alloc_from_contiguous) from [<c1104af9>] (atomic_pool_init+0x7b/0x126)
[    1.233596] [<c1104af9>] (atomic_pool_init) from [<c0101d69>] (do_one_initcall+0x45/0x1e4)
[    1.234188] [<c0101d69>] (do_one_initcall) from [<c1101141>] (kernel_init_freeable+0x157/0x1a6)
[    1.234741] [<c1101141>] (kernel_init_freeable) from [<c0a27fd1>] (kernel_init+0xd/0xe0)
[    1.235289] [<c0a27fd1>] (kernel_init) from [<c0100155>] (ret_from_fork+0x11/0x1c)

This patch moves those statistic fields of cma_stat into struct cma
and introduces cma_kobject wrapper to follow kobject's rule.

At the same time, it fixes other routines based on suggestions[3][4].

[1] https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/
[2] https://lore.kernel.org/linux-mm/fead70a2-4330-79ff-e79a-d8511eab1256@gmail.com/
[3] https://lore.kernel.org/linux-mm/20210323195050.2577017-1-minchan@kernel.org/
[4] https://lore.kernel.org/linux-mm/20210324010547.4134370-1-minchan@kernel.org/

Reported-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Suggested-by: Dmitry Osipenko <digetx@gmail.com>
Suggested-by: John Hubbard <jhubbard@nvidia.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
I belive it's worth to have separate patch rather than replacing
original patch. It will also help to merge without conflict
since we already filed other patch based on it.
Strictly speaking, separating fix part and readbility part
in this patch would be better but it's gray to separate them
since most code in this patch was done while we were fixing
the bug. Since we don't release it yet, I hope it will work.
Otherwise, I can send a replacement patch inclucing all of
changes happend until now with gathering SoB.

 mm/cma.c       |  4 +--
 mm/cma.h       | 25 +++++++-------
 mm/cma_sysfs.c | 88 ++++++++++++++++++++++++++++----------------------
 3 files changed, 65 insertions(+), 52 deletions(-)

diff --git a/mm/cma.c b/mm/cma.c
index 90e27458ddb7..08c45157911a 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -509,11 +509,11 @@ struct page *cma_alloc(struct cma *cma, size_t count, unsigned int align,
 out:
 	if (page) {
 		count_vm_event(CMA_ALLOC_SUCCESS);
-		cma_sysfs_alloc_pages_count(cma, count);
+		cma_sysfs_account_success_pages(cma, count);
 	} else {
 		count_vm_event(CMA_ALLOC_FAIL);
 		if (cma)
-			cma_sysfs_fail_pages_count(cma, count);
+			cma_sysfs_account_fail_pages(cma, count);
 	}
 
 	return page;
diff --git a/mm/cma.h b/mm/cma.h
index 95d1aa2d808a..37b9b7858c8e 100644
--- a/mm/cma.h
+++ b/mm/cma.h
@@ -5,12 +5,8 @@
 #include <linux/debugfs.h>
 #include <linux/kobject.h>
 
-struct cma_stat {
-	spinlock_t lock;
-	/* the number of CMA page successful allocations */
-	unsigned long nr_pages_succeeded;
-	/* the number of CMA page allocation failures */
-	unsigned long nr_pages_failed;
+struct cma_kobject {
+	struct cma *cma;
 	struct kobject kobj;
 };
 
@@ -27,7 +23,12 @@ struct cma {
 #endif
 	char name[CMA_MAX_NAME];
 #ifdef CONFIG_CMA_SYSFS
-	struct cma_stat	*stat;
+	/* the number of CMA page successful allocations */
+	atomic64_t nr_pages_succeeded;
+	/* the number of CMA page allocation failures */
+	atomic64_t nr_pages_failed;
+	/* kobject requires dynamic object */
+	struct cma_kobject *cma_kobj;
 #endif
 };
 
@@ -40,10 +41,12 @@ static inline unsigned long cma_bitmap_maxno(struct cma *cma)
 }
 
 #ifdef CONFIG_CMA_SYSFS
-void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count);
-void cma_sysfs_fail_pages_count(struct cma *cma, size_t count);
+void cma_sysfs_account_success_pages(struct cma *cma, unsigned long nr_pages);
+void cma_sysfs_account_fail_pages(struct cma *cma, unsigned long nr_pages);
 #else
-static inline void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count) {};
-static inline void cma_sysfs_fail_pages_count(struct cma *cma, size_t count) {};
+static inline void cma_sysfs_account_success_pages(struct cma *cma,
+						   unsigned long nr_pages) {};
+static inline void cma_sysfs_account_fail_pages(struct cma *cma,
+						unsigned long nr_pages) {};
 #endif
 #endif
diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c
index 3134b2b3a96d..a670a80aad6f 100644
--- a/mm/cma_sysfs.c
+++ b/mm/cma_sysfs.c
@@ -11,50 +11,54 @@
 
 #include "cma.h"
 
-static struct cma_stat *cma_stats;
-
-void cma_sysfs_alloc_pages_count(struct cma *cma, size_t count)
+void cma_sysfs_account_success_pages(struct cma *cma, unsigned long nr_pages)
 {
-	spin_lock(&cma->stat->lock);
-	cma->stat->nr_pages_succeeded += count;
-	spin_unlock(&cma->stat->lock);
+	atomic64_add(nr_pages, &cma->nr_pages_succeeded);
 }
 
-void cma_sysfs_fail_pages_count(struct cma *cma, size_t count)
+void cma_sysfs_account_fail_pages(struct cma *cma, unsigned long nr_pages)
 {
-	spin_lock(&cma->stat->lock);
-	cma->stat->nr_pages_failed += count;
-	spin_unlock(&cma->stat->lock);
+	atomic64_add(nr_pages, &cma->nr_pages_failed);
 }
 
 #define CMA_ATTR_RO(_name) \
 	static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
 
-static struct kobject *cma_kobj;
+static inline struct cma *cma_from_kobj(struct kobject *kobj)
+{
+	struct cma_kobject *cma_kobj = container_of(kobj, struct cma_kobject,
+						    kobj);
+	struct cma *cma = cma_kobj->cma;
+
+	return cma;
+}
 
 static ssize_t alloc_pages_success_show(struct kobject *kobj,
-			struct kobj_attribute *attr, char *buf)
+					struct kobj_attribute *attr, char *buf)
 {
-	struct cma_stat *stat = container_of(kobj, struct cma_stat, kobj);
+	struct cma *cma = cma_from_kobj(kobj);
 
-	return sysfs_emit(buf, "%lu\n", stat->nr_pages_succeeded);
+	return sysfs_emit(buf, "%llu\n",
+			atomic64_read(&cma->nr_pages_succeeded));
 }
 CMA_ATTR_RO(alloc_pages_success);
 
 static ssize_t alloc_pages_fail_show(struct kobject *kobj,
-			struct kobj_attribute *attr, char *buf)
+				     struct kobj_attribute *attr, char *buf)
 {
-	struct cma_stat *stat = container_of(kobj, struct cma_stat, kobj);
+	struct cma *cma = cma_from_kobj(kobj);
 
-	return sysfs_emit(buf, "%lu\n", stat->nr_pages_failed);
+	return sysfs_emit(buf, "%llu\n", atomic64_read(&cma->nr_pages_failed));
 }
 CMA_ATTR_RO(alloc_pages_fail);
 
 static void cma_kobj_release(struct kobject *kobj)
 {
-	struct cma_stat *stat = container_of(kobj, struct cma_stat, kobj);
+	struct cma *cma = cma_from_kobj(kobj);
+	struct cma_kobject *cma_kobj = cma->cma_kobj;
 
-	kfree(stat);
+	kfree(cma_kobj);
+	cma->cma_kobj = NULL;
 }
 
 static struct attribute *cma_attrs[] = {
@@ -67,44 +71,50 @@ ATTRIBUTE_GROUPS(cma);
 static struct kobj_type cma_ktype = {
 	.release = cma_kobj_release,
 	.sysfs_ops = &kobj_sysfs_ops,
-	.default_groups = cma_groups
+	.default_groups = cma_groups,
 };
 
 static int __init cma_sysfs_init(void)
 {
-	int i = 0;
+	struct kobject *cma_kobj_root;
+	struct cma_kobject *cma_kobj;
 	struct cma *cma;
+	unsigned int i;
+	int err;
 
-	cma_kobj = kobject_create_and_add("cma", mm_kobj);
-	if (!cma_kobj)
+	cma_kobj_root = kobject_create_and_add("cma", mm_kobj);
+	if (!cma_kobj_root)
 		return -ENOMEM;
 
-	cma_stats = kmalloc_array(cma_area_count, sizeof(struct cma_stat),
-				GFP_KERNEL|__GFP_ZERO);
-	if (ZERO_OR_NULL_PTR(cma_stats))
-		goto out;
+	for (i = 0; i < cma_area_count; i++) {
+		cma_kobj = kzalloc(sizeof(*cma_kobj), GFP_KERNEL);
+		if (!cma_kobj) {
+			err = -ENOMEM;
+			goto out;
+		}
 
-	do {
 		cma = &cma_areas[i];
-		cma->stat = &cma_stats[i];
-		spin_lock_init(&cma->stat->lock);
-		if (kobject_init_and_add(&cma->stat->kobj, &cma_ktype,
-					cma_kobj, "%s", cma->name)) {
-			kobject_put(&cma->stat->kobj);
+		cma->cma_kobj = cma_kobj;
+		cma_kobj->cma = cma;
+		err = kobject_init_and_add(&cma_kobj->kobj, &cma_ktype,
+				cma_kobj_root, "%s", cma->name);
+		if (err) {
+			kobject_put(&cma_kobj->kobj);
 			goto out;
 		}
-	} while (++i < cma_area_count);
+	}
 
 	return 0;
 out:
 	while (--i >= 0) {
 		cma = &cma_areas[i];
-		kobject_put(&cma->stat->kobj);
-	}
 
-	kfree(cma_stats);
-	kobject_put(cma_kobj);
+		kobject_put(&cma->cma_kobj->kobj);
+		kfree(cma->cma_kobj);
+		cma->cma_kobj = NULL;
+	}
+	kobject_put(cma_kobj_root);
 
-	return -ENOMEM;
+	return err;
 }
 subsys_initcall(cma_sysfs_init);
-- 
2.31.0.291.g576ba9dcdaf-goog



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:20 [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count Minchan Kim
@ 2021-03-24 19:43 ` Dmitry Osipenko
  2021-03-24 19:49   ` Minchan Kim
  2021-03-24 19:49   ` Dmitry Osipenko
  2021-03-24 19:45 ` John Hubbard
  1 sibling, 2 replies; 10+ messages in thread
From: Dmitry Osipenko @ 2021-03-24 19:43 UTC (permalink / raw)
  To: Minchan Kim, Andrew Morton
  Cc: linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy

24.03.2021 22:20, Minchan Kim пишет:
>  static int __init cma_sysfs_init(void)
>  {
> -	int i = 0;
> +	struct kobject *cma_kobj_root;
> +	struct cma_kobject *cma_kobj;
>  	struct cma *cma;
> +	unsigned int i;

>  	while (--i >= 0) {

Do you realize that this doesn't work anymore?

>  		cma = &cma_areas[i];
> -		kobject_put(&cma->stat->kobj);
> -	}
>  
> -	kfree(cma_stats);
> -	kobject_put(cma_kobj);
> +		kobject_put(&cma->cma_kobj->kobj);
> +		kfree(cma->cma_kobj);

Freeing a null pointer?

> +		cma->cma_kobj = NULL;
> +	}
> +	kobject_put(cma_kobj_root);



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:20 [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count Minchan Kim
  2021-03-24 19:43 ` Dmitry Osipenko
@ 2021-03-24 19:45 ` John Hubbard
  2021-03-24 19:53   ` David Hildenbrand
  1 sibling, 1 reply; 10+ messages in thread
From: John Hubbard @ 2021-03-24 19:45 UTC (permalink / raw)
  To: Minchan Kim, Andrew Morton
  Cc: linux-mm, LKML, gregkh, surenb, joaodias, willy, digetx

On 3/24/21 12:20 PM, Minchan Kim wrote:
> struct cma_stat's lifespan for cma_sysfs is different with
> struct cma because kobject for sysfs requires dynamic object
> while CMA is static object[1]. When CMA is initialized,
> it couldn't use slab to allocate cma_stat since slab was not
> initialized yet. Thus, it allocates the dynamic object
> in subsys_initcall.
> 
> However, the cma allocation can happens before subsys_initcall
> then, it goes crash.
> 
> Dmitry reported[2]:
> 
> ..
> [    1.226190] [<c027762f>] (cma_sysfs_alloc_pages_count) from [<c027706f>] (cma_alloc+0x153/0x274)
> [    1.226720] [<c027706f>] (cma_alloc) from [<c01112ab>] (__alloc_from_contiguous+0x37/0x8c)
> [    1.227272] [<c01112ab>] (__alloc_from_contiguous) from [<c1104af9>] (atomic_pool_init+0x7b/0x126)
> [    1.233596] [<c1104af9>] (atomic_pool_init) from [<c0101d69>] (do_one_initcall+0x45/0x1e4)
> [    1.234188] [<c0101d69>] (do_one_initcall) from [<c1101141>] (kernel_init_freeable+0x157/0x1a6)
> [    1.234741] [<c1101141>] (kernel_init_freeable) from [<c0a27fd1>] (kernel_init+0xd/0xe0)
> [    1.235289] [<c0a27fd1>] (kernel_init) from [<c0100155>] (ret_from_fork+0x11/0x1c)
> 
> This patch moves those statistic fields of cma_stat into struct cma
> and introduces cma_kobject wrapper to follow kobject's rule.
> 
> At the same time, it fixes other routines based on suggestions[3][4].
> 
> [1] https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/
> [2] https://lore.kernel.org/linux-mm/fead70a2-4330-79ff-e79a-d8511eab1256@gmail.com/
> [3] https://lore.kernel.org/linux-mm/20210323195050.2577017-1-minchan@kernel.org/
> [4] https://lore.kernel.org/linux-mm/20210324010547.4134370-1-minchan@kernel.org/
> 
> Reported-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> Suggested-by: Dmitry Osipenko <digetx@gmail.com>
> Suggested-by: John Hubbard <jhubbard@nvidia.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> I belive it's worth to have separate patch rather than replacing
> original patch. It will also help to merge without conflict
> since we already filed other patch based on it.
> Strictly speaking, separating fix part and readbility part
> in this patch would be better but it's gray to separate them
> since most code in this patch was done while we were fixing
> the bug. Since we don't release it yet, I hope it will work.
> Otherwise, I can send a replacement patch inclucing all of
> changes happend until now with gathering SoB.

If we still have a choice, we should not merge a patch that has a known
serious problem, such as a crash. That's only done if the broken problematic
patch has already been committed to a tree that doesn't allow rebasing,
such as of course the main linux.git.

Here, I *think* it's just in linux-next and mmotm, so we still are allowed
to fix the original patch.

thanks,
-- 
John Hubbard
NVIDIA


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:43 ` Dmitry Osipenko
@ 2021-03-24 19:49   ` Minchan Kim
  2021-03-24 19:49   ` Dmitry Osipenko
  1 sibling, 0 replies; 10+ messages in thread
From: Minchan Kim @ 2021-03-24 19:49 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy

On Wed, Mar 24, 2021 at 10:43:49PM +0300, Dmitry Osipenko wrote:
> 24.03.2021 22:20, Minchan Kim пишет:
> >  static int __init cma_sysfs_init(void)
> >  {
> > -	int i = 0;
> > +	struct kobject *cma_kobj_root;
> > +	struct cma_kobject *cma_kobj;
> >  	struct cma *cma;
> > +	unsigned int i;
> 
> >  	while (--i >= 0) {
> 
> Do you realize that this doesn't work anymore?
> 
> >  		cma = &cma_areas[i];
> > -		kobject_put(&cma->stat->kobj);
> > -	}
> >  
> > -	kfree(cma_stats);
> > -	kobject_put(cma_kobj);
> > +		kobject_put(&cma->cma_kobj->kobj);
> > +		kfree(cma->cma_kobj);
> 
> Freeing a null pointer?

Need coffee.
 
diff --git a/mm/cma_sysfs.c b/mm/cma_sysfs.c
index a670a80aad6f..73463be08df7 100644
--- a/mm/cma_sysfs.c
+++ b/mm/cma_sysfs.c
@@ -79,8 +79,7 @@ static int __init cma_sysfs_init(void)
        struct kobject *cma_kobj_root;
        struct cma_kobject *cma_kobj;
        struct cma *cma;
-       unsigned int i;
-       int err;
+       int i, err;

        cma_kobj_root = kobject_create_and_add("cma", mm_kobj);
        if (!cma_kobj_root)
@@ -108,10 +107,7 @@ static int __init cma_sysfs_init(void)
 out:
        while (--i >= 0) {
                cma = &cma_areas[i];
-
                kobject_put(&cma->cma_kobj->kobj);
-               kfree(cma->cma_kobj);
-               cma->cma_kobj = NULL;
        }
        kobject_put(cma_kobj_root);




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:43 ` Dmitry Osipenko
  2021-03-24 19:49   ` Minchan Kim
@ 2021-03-24 19:49   ` Dmitry Osipenko
  2021-03-24 19:57     ` Minchan Kim
  1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Osipenko @ 2021-03-24 19:49 UTC (permalink / raw)
  To: Minchan Kim, Andrew Morton
  Cc: linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy

24.03.2021 22:43, Dmitry Osipenko пишет:
> 24.03.2021 22:20, Minchan Kim пишет:
>>  static int __init cma_sysfs_init(void)
>>  {
>> -	int i = 0;
>> +	struct kobject *cma_kobj_root;
>> +	struct cma_kobject *cma_kobj;
>>  	struct cma *cma;
>> +	unsigned int i;
> 
>>  	while (--i >= 0) {
> 
> Do you realize that this doesn't work anymore?
> 
>>  		cma = &cma_areas[i];
>> -		kobject_put(&cma->stat->kobj);
>> -	}
>>  
>> -	kfree(cma_stats);
>> -	kobject_put(cma_kobj);
>> +		kobject_put(&cma->cma_kobj->kobj);
>> +		kfree(cma->cma_kobj);
> 
> Freeing a null pointer?
> 
>> +		cma->cma_kobj = NULL;
>> +	}
>> +	kobject_put(cma_kobj_root);
> 

Please try to simulate the errors and check that error path is working
properly in the next version.

Alternatively, we could remove the cma_kobj_release entirely, like Greg
suggested previously, and then don't care about cleaning up at all.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:45 ` John Hubbard
@ 2021-03-24 19:53   ` David Hildenbrand
  2021-03-24 19:55     ` Minchan Kim
  0 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2021-03-24 19:53 UTC (permalink / raw)
  To: John Hubbard, Minchan Kim, Andrew Morton
  Cc: linux-mm, LKML, gregkh, surenb, joaodias, willy, digetx

On 24.03.21 20:45, John Hubbard wrote:
> On 3/24/21 12:20 PM, Minchan Kim wrote:
>> struct cma_stat's lifespan for cma_sysfs is different with
>> struct cma because kobject for sysfs requires dynamic object
>> while CMA is static object[1]. When CMA is initialized,
>> it couldn't use slab to allocate cma_stat since slab was not
>> initialized yet. Thus, it allocates the dynamic object
>> in subsys_initcall.
>>
>> However, the cma allocation can happens before subsys_initcall
>> then, it goes crash.
>>
>> Dmitry reported[2]:
>>
>> ..
>> [    1.226190] [<c027762f>] (cma_sysfs_alloc_pages_count) from [<c027706f>] (cma_alloc+0x153/0x274)
>> [    1.226720] [<c027706f>] (cma_alloc) from [<c01112ab>] (__alloc_from_contiguous+0x37/0x8c)
>> [    1.227272] [<c01112ab>] (__alloc_from_contiguous) from [<c1104af9>] (atomic_pool_init+0x7b/0x126)
>> [    1.233596] [<c1104af9>] (atomic_pool_init) from [<c0101d69>] (do_one_initcall+0x45/0x1e4)
>> [    1.234188] [<c0101d69>] (do_one_initcall) from [<c1101141>] (kernel_init_freeable+0x157/0x1a6)
>> [    1.234741] [<c1101141>] (kernel_init_freeable) from [<c0a27fd1>] (kernel_init+0xd/0xe0)
>> [    1.235289] [<c0a27fd1>] (kernel_init) from [<c0100155>] (ret_from_fork+0x11/0x1c)
>>
>> This patch moves those statistic fields of cma_stat into struct cma
>> and introduces cma_kobject wrapper to follow kobject's rule.
>>
>> At the same time, it fixes other routines based on suggestions[3][4].
>>
>> [1] https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/
>> [2] https://lore.kernel.org/linux-mm/fead70a2-4330-79ff-e79a-d8511eab1256@gmail.com/
>> [3] https://lore.kernel.org/linux-mm/20210323195050.2577017-1-minchan@kernel.org/
>> [4] https://lore.kernel.org/linux-mm/20210324010547.4134370-1-minchan@kernel.org/
>>
>> Reported-by: Dmitry Osipenko <digetx@gmail.com>
>> Tested-by: Dmitry Osipenko <digetx@gmail.com>
>> Suggested-by: Dmitry Osipenko <digetx@gmail.com>
>> Suggested-by: John Hubbard <jhubbard@nvidia.com>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>> ---
>> I belive it's worth to have separate patch rather than replacing
>> original patch. It will also help to merge without conflict
>> since we already filed other patch based on it.
>> Strictly speaking, separating fix part and readbility part
>> in this patch would be better but it's gray to separate them
>> since most code in this patch was done while we were fixing
>> the bug. Since we don't release it yet, I hope it will work.
>> Otherwise, I can send a replacement patch inclucing all of
>> changes happend until now with gathering SoB.
> 
> If we still have a choice, we should not merge a patch that has a known
> serious problem, such as a crash. That's only done if the broken problematic
> patch has already been committed to a tree that doesn't allow rebasing,
> such as of course the main linux.git.
> 
> Here, I *think* it's just in linux-next and mmotm, so we still are allowed
> to fix the original patch.

Yes, that's what we should do in case it's not upstream yet. Clean 
resend + re-apply.


-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:53   ` David Hildenbrand
@ 2021-03-24 19:55     ` Minchan Kim
  0 siblings, 0 replies; 10+ messages in thread
From: Minchan Kim @ 2021-03-24 19:55 UTC (permalink / raw)
  To: David Hildenbrand
  Cc: John Hubbard, Andrew Morton, linux-mm, LKML, gregkh, surenb,
	joaodias, willy, digetx

On Wed, Mar 24, 2021 at 08:53:26PM +0100, David Hildenbrand wrote:
> On 24.03.21 20:45, John Hubbard wrote:
> > On 3/24/21 12:20 PM, Minchan Kim wrote:
> > > struct cma_stat's lifespan for cma_sysfs is different with
> > > struct cma because kobject for sysfs requires dynamic object
> > > while CMA is static object[1]. When CMA is initialized,
> > > it couldn't use slab to allocate cma_stat since slab was not
> > > initialized yet. Thus, it allocates the dynamic object
> > > in subsys_initcall.
> > > 
> > > However, the cma allocation can happens before subsys_initcall
> > > then, it goes crash.
> > > 
> > > Dmitry reported[2]:
> > > 
> > > ..
> > > [    1.226190] [<c027762f>] (cma_sysfs_alloc_pages_count) from [<c027706f>] (cma_alloc+0x153/0x274)
> > > [    1.226720] [<c027706f>] (cma_alloc) from [<c01112ab>] (__alloc_from_contiguous+0x37/0x8c)
> > > [    1.227272] [<c01112ab>] (__alloc_from_contiguous) from [<c1104af9>] (atomic_pool_init+0x7b/0x126)
> > > [    1.233596] [<c1104af9>] (atomic_pool_init) from [<c0101d69>] (do_one_initcall+0x45/0x1e4)
> > > [    1.234188] [<c0101d69>] (do_one_initcall) from [<c1101141>] (kernel_init_freeable+0x157/0x1a6)
> > > [    1.234741] [<c1101141>] (kernel_init_freeable) from [<c0a27fd1>] (kernel_init+0xd/0xe0)
> > > [    1.235289] [<c0a27fd1>] (kernel_init) from [<c0100155>] (ret_from_fork+0x11/0x1c)
> > > 
> > > This patch moves those statistic fields of cma_stat into struct cma
> > > and introduces cma_kobject wrapper to follow kobject's rule.
> > > 
> > > At the same time, it fixes other routines based on suggestions[3][4].
> > > 
> > > [1] https://lore.kernel.org/linux-mm/YCOAmXqt6dZkCQYs@kroah.com/
> > > [2] https://lore.kernel.org/linux-mm/fead70a2-4330-79ff-e79a-d8511eab1256@gmail.com/
> > > [3] https://lore.kernel.org/linux-mm/20210323195050.2577017-1-minchan@kernel.org/
> > > [4] https://lore.kernel.org/linux-mm/20210324010547.4134370-1-minchan@kernel.org/
> > > 
> > > Reported-by: Dmitry Osipenko <digetx@gmail.com>
> > > Tested-by: Dmitry Osipenko <digetx@gmail.com>
> > > Suggested-by: Dmitry Osipenko <digetx@gmail.com>
> > > Suggested-by: John Hubbard <jhubbard@nvidia.com>
> > > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > > ---
> > > I belive it's worth to have separate patch rather than replacing
> > > original patch. It will also help to merge without conflict
> > > since we already filed other patch based on it.
> > > Strictly speaking, separating fix part and readbility part
> > > in this patch would be better but it's gray to separate them
> > > since most code in this patch was done while we were fixing
> > > the bug. Since we don't release it yet, I hope it will work.
> > > Otherwise, I can send a replacement patch inclucing all of
> > > changes happend until now with gathering SoB.
> > 
> > If we still have a choice, we should not merge a patch that has a known
> > serious problem, such as a crash. That's only done if the broken problematic
> > patch has already been committed to a tree that doesn't allow rebasing,
> > such as of course the main linux.git.
> > 
> > Here, I *think* it's just in linux-next and mmotm, so we still are allowed
> > to fix the original patch.
> 
> Yes, that's what we should do in case it's not upstream yet. Clean resend +
> re-apply.

Okay, let me replace the original one including all other patches.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:49   ` Dmitry Osipenko
@ 2021-03-24 19:57     ` Minchan Kim
  2021-03-24 20:02       ` Dmitry Osipenko
  0 siblings, 1 reply; 10+ messages in thread
From: Minchan Kim @ 2021-03-24 19:57 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy

On Wed, Mar 24, 2021 at 10:49:58PM +0300, Dmitry Osipenko wrote:
> 24.03.2021 22:43, Dmitry Osipenko пишет:
> > 24.03.2021 22:20, Minchan Kim пишет:
> >>  static int __init cma_sysfs_init(void)
> >>  {
> >> -	int i = 0;
> >> +	struct kobject *cma_kobj_root;
> >> +	struct cma_kobject *cma_kobj;
> >>  	struct cma *cma;
> >> +	unsigned int i;
> > 
> >>  	while (--i >= 0) {
> > 
> > Do you realize that this doesn't work anymore?
> > 
> >>  		cma = &cma_areas[i];
> >> -		kobject_put(&cma->stat->kobj);
> >> -	}
> >>  
> >> -	kfree(cma_stats);
> >> -	kobject_put(cma_kobj);
> >> +		kobject_put(&cma->cma_kobj->kobj);
> >> +		kfree(cma->cma_kobj);
> > 
> > Freeing a null pointer?
> > 
> >> +		cma->cma_kobj = NULL;
> >> +	}
> >> +	kobject_put(cma_kobj_root);
> > 
> 
> Please try to simulate the errors and check that error path is working
> properly in the next version.
> 
> Alternatively, we could remove the cma_kobj_release entirely, like Greg
> suggested previously, and then don't care about cleaning up at all.

Does he suggested it to remove cma_kobj_release?(Initially, I did but
was rejected from Greg)



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 19:57     ` Minchan Kim
@ 2021-03-24 20:02       ` Dmitry Osipenko
  2021-03-24 20:55         ` Minchan Kim
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Osipenko @ 2021-03-24 20:02 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy

24.03.2021 22:57, Minchan Kim пишет:
> On Wed, Mar 24, 2021 at 10:49:58PM +0300, Dmitry Osipenko wrote:
>> 24.03.2021 22:43, Dmitry Osipenko пишет:
>>> 24.03.2021 22:20, Minchan Kim пишет:
>>>>  static int __init cma_sysfs_init(void)
>>>>  {
>>>> -	int i = 0;
>>>> +	struct kobject *cma_kobj_root;
>>>> +	struct cma_kobject *cma_kobj;
>>>>  	struct cma *cma;
>>>> +	unsigned int i;
>>>
>>>>  	while (--i >= 0) {
>>>
>>> Do you realize that this doesn't work anymore?
>>>
>>>>  		cma = &cma_areas[i];
>>>> -		kobject_put(&cma->stat->kobj);
>>>> -	}
>>>>  
>>>> -	kfree(cma_stats);
>>>> -	kobject_put(cma_kobj);
>>>> +		kobject_put(&cma->cma_kobj->kobj);
>>>> +		kfree(cma->cma_kobj);
>>>
>>> Freeing a null pointer?
>>>
>>>> +		cma->cma_kobj = NULL;
>>>> +	}
>>>> +	kobject_put(cma_kobj_root);
>>>
>>
>> Please try to simulate the errors and check that error path is working
>> properly in the next version.
>>
>> Alternatively, we could remove the cma_kobj_release entirely, like Greg
>> suggested previously, and then don't care about cleaning up at all.
> 
> Does he suggested it to remove cma_kobj_release?(Initially, I did but
> was rejected from Greg)
> 

Alright, I haven't followed the previous threads fully and only saw the
reply where he suggested to removed it.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count
  2021-03-24 20:02       ` Dmitry Osipenko
@ 2021-03-24 20:55         ` Minchan Kim
  0 siblings, 0 replies; 10+ messages in thread
From: Minchan Kim @ 2021-03-24 20:55 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Andrew Morton, linux-mm, LKML, gregkh, surenb, joaodias, jhubbard, willy

On Wed, Mar 24, 2021 at 11:02:47PM +0300, Dmitry Osipenko wrote:
> 24.03.2021 22:57, Minchan Kim пишет:
> > On Wed, Mar 24, 2021 at 10:49:58PM +0300, Dmitry Osipenko wrote:
> >> 24.03.2021 22:43, Dmitry Osipenko пишет:
> >>> 24.03.2021 22:20, Minchan Kim пишет:
> >>>>  static int __init cma_sysfs_init(void)
> >>>>  {
> >>>> -	int i = 0;
> >>>> +	struct kobject *cma_kobj_root;
> >>>> +	struct cma_kobject *cma_kobj;
> >>>>  	struct cma *cma;
> >>>> +	unsigned int i;
> >>>
> >>>>  	while (--i >= 0) {
> >>>
> >>> Do you realize that this doesn't work anymore?
> >>>
> >>>>  		cma = &cma_areas[i];
> >>>> -		kobject_put(&cma->stat->kobj);
> >>>> -	}
> >>>>  
> >>>> -	kfree(cma_stats);
> >>>> -	kobject_put(cma_kobj);
> >>>> +		kobject_put(&cma->cma_kobj->kobj);
> >>>> +		kfree(cma->cma_kobj);
> >>>
> >>> Freeing a null pointer?
> >>>
> >>>> +		cma->cma_kobj = NULL;
> >>>> +	}
> >>>> +	kobject_put(cma_kobj_root);
> >>>
> >>
> >> Please try to simulate the errors and check that error path is working
> >> properly in the next version.
> >>
> >> Alternatively, we could remove the cma_kobj_release entirely, like Greg
> >> suggested previously, and then don't care about cleaning up at all.
> > 
> > Does he suggested it to remove cma_kobj_release?(Initially, I did but
> > was rejected from Greg)
> > 
> 
> Alright, I haven't followed the previous threads fully and only saw the
> reply where he suggested to removed it.

No problem. I just posted it new version. Hopefully, it tastes good
for you. ;-)

Thanks for the review!


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-03-24 20:55 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-24 19:20 [PATCH] mm: cma: fix corruption cma_sysfs_alloc_pages_count Minchan Kim
2021-03-24 19:43 ` Dmitry Osipenko
2021-03-24 19:49   ` Minchan Kim
2021-03-24 19:49   ` Dmitry Osipenko
2021-03-24 19:57     ` Minchan Kim
2021-03-24 20:02       ` Dmitry Osipenko
2021-03-24 20:55         ` Minchan Kim
2021-03-24 19:45 ` John Hubbard
2021-03-24 19:53   ` David Hildenbrand
2021-03-24 19:55     ` Minchan Kim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).