All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] memcg: fix a crash in wb_workfn when a device disappears
@ 2019-12-27 19:48 Theodore Ts'o
  2019-12-27 20:31 ` Theodore Ts'o
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Theodore Ts'o @ 2019-12-27 19:48 UTC (permalink / raw)
  To: Linux Filesystem Development List; +Cc: linux-mm, Theodore Ts'o

Without memcg, there is a one-to-one mapping between the bdi and
bdi_writeback structures.  In this world, things are fairly
straightforward; the first thing bdi_unregister() does is to shutdown
the bdi_writeback structure (or wb), and part of that writeback
ensures that no other work queued against the wb, and that the wb is
fully drained.

With memcg, however, there is a one-to-many relationship between the
bdi and bdi_writeback structures; that is, there are multiple wb
objects which can all point to a single bdi.  There is a refcount
which prevents the bdi object from being released (and hence,
unregistered).  So in theory, the bdi_unregister() *should* only get
called once its refcount goes to zero (bdi_put will drop the refcount,
and when it is zero, release_bdi gets called, which calls
bdi_unregister.

Unfortunately, del_gendisk() in block/gen_hd.c never got the memo
about the Brave New memcg World, and calls bdi_unregister directly.
It does this without informing the file system, or the memcg code, or
anything else.  This causes the root wb associated with the bdi to be
unregistered, but none of the memcg-specific wb's are shutdown.  So when
one of these wb's are woken up to do delayed work, they try to
dereference their wb->bdi->dev to fetch the device name, but
unfortunately bdi->dev is now NULL, thanks to the bdi_unregister()
called by del_gendisk().   As a result, *boom*.

Fortunately, it looks like the rest of the writeback path is perfectly
happy with bdi->dev and bdi->owner being NULL, so the simplest fix is
to create a bdi_dev_name() function which can handle bdi->dev being
NULL.  This also allows us to bulletproof the writeback tracepoints to
prevent them from dereferencing a NULL pointer and crashing the kernel
if one is tracing with memcg's enabled, and an iSCSI device dies or a
USB storage stick is pulled.

Google-Bug-Id: 145475544
Tested: fs smoke test
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/fs-writeback.c                |  2 +-
 include/linux/backing-dev.h      |  9 ++++++++
 include/trace/events/writeback.h | 37 +++++++++++++++-----------------
 mm/backing-dev.c                 |  1 +
 4 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 335607b8c5c0..76ac9c7d32ec 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2063,7 +2063,7 @@ void wb_workfn(struct work_struct *work)
 						struct bdi_writeback, dwork);
 	long pages_written;
 
-	set_worker_desc("flush-%s", dev_name(wb->bdi->dev));
+	set_worker_desc("flush-%s", bdi_dev_name(wb->bdi));
 	current->flags |= PF_SWAPWRITE;
 
 	if (likely(!current_is_workqueue_rescuer() ||
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 97967ce06de3..d968a50a0be5 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -504,4 +504,13 @@ static inline int bdi_rw_congested(struct backing_dev_info *bdi)
 				  (1 << WB_async_congested));
 }
 
+extern const char *bdi_unknown_name;
+
+static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
+{
+	if (!bdi || !bdi->dev)
+		return bdi_unknown_name;
+	return dev_name(bdi->dev);
+}
+
 #endif	/* _LINUX_BACKING_DEV_H */
diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index ef50be4e5e6c..d94def25e4dc 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -67,8 +67,8 @@ DECLARE_EVENT_CLASS(writeback_page_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)",
-			    32);
+			    bdi_dev_name(mapping ? inode_to_bdi(mapping->host) :
+					 NULL), 32);
 		__entry->ino = mapping ? mapping->host->i_ino : 0;
 		__entry->index = page->index;
 	),
@@ -111,8 +111,7 @@ DECLARE_EVENT_CLASS(writeback_dirty_inode_template,
 		struct backing_dev_info *bdi = inode_to_bdi(inode);
 
 		/* may be called for files on pseudo FSes w/ unregistered bdi */
-		strscpy_pad(__entry->name,
-			    bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->flags		= flags;
@@ -193,7 +192,7 @@ TRACE_EVENT(inode_foreign_history,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name, dev_name(inode_to_bdi(inode)->dev), 32);
+		strncpy(__entry->name, bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->cgroup_ino	= __trace_wbc_assign_cgroup(wbc);
 		__entry->history	= history;
@@ -222,7 +221,7 @@ TRACE_EVENT(inode_switch_wbs,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name,	dev_name(old_wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(old_wb->bdi), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->old_cgroup_ino	= __trace_wb_assign_cgroup(old_wb);
 		__entry->new_cgroup_ino	= __trace_wb_assign_cgroup(new_wb);
@@ -255,7 +254,7 @@ TRACE_EVENT(track_foreign_dirty,
 		struct address_space *mapping = page_mapping(page);
 		struct inode *inode = mapping ? mapping->host : NULL;
 
-		strncpy(__entry->name,	dev_name(wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(wb->bdi), 32);
 		__entry->bdi_id		= wb->bdi->id;
 		__entry->ino		= inode ? inode->i_ino : 0;
 		__entry->memcg_id	= wb->memcg_css->id;
@@ -288,7 +287,7 @@ TRACE_EVENT(flush_foreign,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name,	dev_name(wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(wb->bdi), 32);
 		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
 		__entry->frn_bdi_id	= frn_bdi_id;
 		__entry->frn_memcg_id	= frn_memcg_id;
@@ -318,7 +317,7 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->sync_mode	= wbc->sync_mode;
 		__entry->cgroup_ino	= __trace_wbc_assign_cgroup(wbc);
@@ -361,9 +360,7 @@ DECLARE_EVENT_CLASS(writeback_work_class,
 		__field(ino_t, cgroup_ino)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name,
-			    wb->bdi->dev ? dev_name(wb->bdi->dev) :
-			    "(unknown)", 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->nr_pages = work->nr_pages;
 		__entry->sb_dev = work->sb ? work->sb->s_dev : 0;
 		__entry->sync_mode = work->sync_mode;
@@ -416,7 +413,7 @@ DECLARE_EVENT_CLASS(writeback_class,
 		__field(ino_t, cgroup_ino)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
 	),
 	TP_printk("bdi %s: cgroup_ino=%lu",
@@ -438,7 +435,7 @@ TRACE_EVENT(writeback_bdi_register,
 		__array(char, name, 32)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 	),
 	TP_printk("bdi %s",
 		__entry->name
@@ -463,7 +460,7 @@ DECLARE_EVENT_CLASS(wbc_class,
 	),
 
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 		__entry->nr_to_write	= wbc->nr_to_write;
 		__entry->pages_skipped	= wbc->pages_skipped;
 		__entry->sync_mode	= wbc->sync_mode;
@@ -514,7 +511,7 @@ TRACE_EVENT(writeback_queue_io,
 	),
 	TP_fast_assign(
 		unsigned long *older_than_this = work->older_than_this;
-		strscpy_pad(__entry->name, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->older	= older_than_this ?  *older_than_this : 0;
 		__entry->age	= older_than_this ?
 				  (jiffies - *older_than_this) * 1000 / HZ : -1;
@@ -600,7 +597,7 @@ TRACE_EVENT(bdi_dirty_ratelimit,
 	),
 
 	TP_fast_assign(
-		strscpy_pad(__entry->bdi, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
 		__entry->write_bw	= KBps(wb->write_bandwidth);
 		__entry->avg_write_bw	= KBps(wb->avg_write_bandwidth);
 		__entry->dirty_rate	= KBps(dirty_rate);
@@ -665,7 +662,7 @@ TRACE_EVENT(balance_dirty_pages,
 
 	TP_fast_assign(
 		unsigned long freerun = (thresh + bg_thresh) / 2;
-		strscpy_pad(__entry->bdi, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
 
 		__entry->limit		= global_wb_domain.dirty_limit;
 		__entry->setpoint	= (global_wb_domain.dirty_limit +
@@ -726,7 +723,7 @@ TRACE_EVENT(writeback_sb_inodes_requeue,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->dirtied_when	= inode->dirtied_when;
@@ -800,7 +797,7 @@ DECLARE_EVENT_CLASS(writeback_single_inode_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->dirtied_when	= inode->dirtied_when;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c360f6a6c844..62f05f605fb5 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -21,6 +21,7 @@ struct backing_dev_info noop_backing_dev_info = {
 EXPORT_SYMBOL_GPL(noop_backing_dev_info);
 
 static struct class *bdi_class;
+const char *bdi_unknown_name = "(unknown)";
 
 /*
  * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH] memcg: fix a crash in wb_workfn when a device disappears
  2019-12-27 19:48 [PATCH] memcg: fix a crash in wb_workfn when a device disappears Theodore Ts'o
@ 2019-12-27 20:31 ` Theodore Ts'o
  2019-12-27 21:16   ` kbuild test robot
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Theodore Ts'o @ 2019-12-27 20:31 UTC (permalink / raw)
  To: Linux Filesystem Development List, linux-mm; +Cc: Theodore Ts'o

Without memcg, there is a one-to-one mapping between the bdi and
bdi_writeback structures.  In this world, things are fairly
straightforward; the first thing bdi_unregister() does is to shutdown
the bdi_writeback structure (or wb), and part of that writeback
ensures that no other work queued against the wb, and that the wb is
fully drained.

With memcg, however, there is a one-to-many relationship between the
bdi and bdi_writeback structures; that is, there are multiple wb
objects which can all point to a single bdi.  There is a refcount
which prevents the bdi object from being released (and hence,
unregistered).  So in theory, the bdi_unregister() *should* only get
called once its refcount goes to zero (bdi_put will drop the refcount,
and when it is zero, release_bdi gets called, which calls
bdi_unregister).

Unfortunately, del_gendisk() in block/gen_hd.c never got the memo
about the Brave New memcg World, and calls bdi_unregister directly.
It does this without informing the file system, or the memcg code, or
anything else.  This causes the root wb associated with the bdi to be
unregistered, but none of the memcg-specific wb's are shutdown.  So when
one of these wb's are woken up to do delayed work, they try to
dereference their wb->bdi->dev to fetch the device name, but
unfortunately bdi->dev is now NULL, thanks to the bdi_unregister()
called by del_gendisk().   As a result, *boom*.

Fortunately, it looks like the rest of the writeback path is perfectly
happy with bdi->dev and bdi->owner being NULL, so the simplest fix is
to create a bdi_dev_name() function which can handle bdi->dev being
NULL.  This also allows us to bulletproof the writeback tracepoints to
prevent them from dereferencing a NULL pointer and crashing the kernel
if one is tracing with memcg's enabled, and an iSCSI device dies or a
USB storage stick is pulled.

Previous-Version-Link: https://lore.kernel.org/k/20191227194829.150110-1-tytso@mit.edu
Google-Bug-Id: 145475544
Tested: fs smoke test
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/fs-writeback.c                |  2 +-
 include/linux/backing-dev.h      | 10 +++++++++
 include/trace/events/writeback.h | 37 +++++++++++++++-----------------
 mm/backing-dev.c                 |  1 +
 4 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 335607b8c5c0..76ac9c7d32ec 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2063,7 +2063,7 @@ void wb_workfn(struct work_struct *work)
 						struct bdi_writeback, dwork);
 	long pages_written;
 
-	set_worker_desc("flush-%s", dev_name(wb->bdi->dev));
+	set_worker_desc("flush-%s", bdi_dev_name(wb->bdi));
 	current->flags |= PF_SWAPWRITE;
 
 	if (likely(!current_is_workqueue_rescuer() ||
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 97967ce06de3..f88197c1ffc2 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -13,6 +13,7 @@
 #include <linux/fs.h>
 #include <linux/sched.h>
 #include <linux/blkdev.h>
+#include <linux/device.h>
 #include <linux/writeback.h>
 #include <linux/blk-cgroup.h>
 #include <linux/backing-dev-defs.h>
@@ -504,4 +505,13 @@ static inline int bdi_rw_congested(struct backing_dev_info *bdi)
 				  (1 << WB_async_congested));
 }
 
+extern const char *bdi_unknown_name;
+
+static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
+{
+	if (!bdi || !bdi->dev)
+		return bdi_unknown_name;
+	return dev_name(bdi->dev);
+}
+
 #endif	/* _LINUX_BACKING_DEV_H */
diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index ef50be4e5e6c..d94def25e4dc 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -67,8 +67,8 @@ DECLARE_EVENT_CLASS(writeback_page_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)",
-			    32);
+			    bdi_dev_name(mapping ? inode_to_bdi(mapping->host) :
+					 NULL), 32);
 		__entry->ino = mapping ? mapping->host->i_ino : 0;
 		__entry->index = page->index;
 	),
@@ -111,8 +111,7 @@ DECLARE_EVENT_CLASS(writeback_dirty_inode_template,
 		struct backing_dev_info *bdi = inode_to_bdi(inode);
 
 		/* may be called for files on pseudo FSes w/ unregistered bdi */
-		strscpy_pad(__entry->name,
-			    bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->flags		= flags;
@@ -193,7 +192,7 @@ TRACE_EVENT(inode_foreign_history,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name, dev_name(inode_to_bdi(inode)->dev), 32);
+		strncpy(__entry->name, bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->cgroup_ino	= __trace_wbc_assign_cgroup(wbc);
 		__entry->history	= history;
@@ -222,7 +221,7 @@ TRACE_EVENT(inode_switch_wbs,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name,	dev_name(old_wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(old_wb->bdi), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->old_cgroup_ino	= __trace_wb_assign_cgroup(old_wb);
 		__entry->new_cgroup_ino	= __trace_wb_assign_cgroup(new_wb);
@@ -255,7 +254,7 @@ TRACE_EVENT(track_foreign_dirty,
 		struct address_space *mapping = page_mapping(page);
 		struct inode *inode = mapping ? mapping->host : NULL;
 
-		strncpy(__entry->name,	dev_name(wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(wb->bdi), 32);
 		__entry->bdi_id		= wb->bdi->id;
 		__entry->ino		= inode ? inode->i_ino : 0;
 		__entry->memcg_id	= wb->memcg_css->id;
@@ -288,7 +287,7 @@ TRACE_EVENT(flush_foreign,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name,	dev_name(wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(wb->bdi), 32);
 		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
 		__entry->frn_bdi_id	= frn_bdi_id;
 		__entry->frn_memcg_id	= frn_memcg_id;
@@ -318,7 +317,7 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->sync_mode	= wbc->sync_mode;
 		__entry->cgroup_ino	= __trace_wbc_assign_cgroup(wbc);
@@ -361,9 +360,7 @@ DECLARE_EVENT_CLASS(writeback_work_class,
 		__field(ino_t, cgroup_ino)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name,
-			    wb->bdi->dev ? dev_name(wb->bdi->dev) :
-			    "(unknown)", 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->nr_pages = work->nr_pages;
 		__entry->sb_dev = work->sb ? work->sb->s_dev : 0;
 		__entry->sync_mode = work->sync_mode;
@@ -416,7 +413,7 @@ DECLARE_EVENT_CLASS(writeback_class,
 		__field(ino_t, cgroup_ino)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
 	),
 	TP_printk("bdi %s: cgroup_ino=%lu",
@@ -438,7 +435,7 @@ TRACE_EVENT(writeback_bdi_register,
 		__array(char, name, 32)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 	),
 	TP_printk("bdi %s",
 		__entry->name
@@ -463,7 +460,7 @@ DECLARE_EVENT_CLASS(wbc_class,
 	),
 
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 		__entry->nr_to_write	= wbc->nr_to_write;
 		__entry->pages_skipped	= wbc->pages_skipped;
 		__entry->sync_mode	= wbc->sync_mode;
@@ -514,7 +511,7 @@ TRACE_EVENT(writeback_queue_io,
 	),
 	TP_fast_assign(
 		unsigned long *older_than_this = work->older_than_this;
-		strscpy_pad(__entry->name, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->older	= older_than_this ?  *older_than_this : 0;
 		__entry->age	= older_than_this ?
 				  (jiffies - *older_than_this) * 1000 / HZ : -1;
@@ -600,7 +597,7 @@ TRACE_EVENT(bdi_dirty_ratelimit,
 	),
 
 	TP_fast_assign(
-		strscpy_pad(__entry->bdi, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
 		__entry->write_bw	= KBps(wb->write_bandwidth);
 		__entry->avg_write_bw	= KBps(wb->avg_write_bandwidth);
 		__entry->dirty_rate	= KBps(dirty_rate);
@@ -665,7 +662,7 @@ TRACE_EVENT(balance_dirty_pages,
 
 	TP_fast_assign(
 		unsigned long freerun = (thresh + bg_thresh) / 2;
-		strscpy_pad(__entry->bdi, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
 
 		__entry->limit		= global_wb_domain.dirty_limit;
 		__entry->setpoint	= (global_wb_domain.dirty_limit +
@@ -726,7 +723,7 @@ TRACE_EVENT(writeback_sb_inodes_requeue,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->dirtied_when	= inode->dirtied_when;
@@ -800,7 +797,7 @@ DECLARE_EVENT_CLASS(writeback_single_inode_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->dirtied_when	= inode->dirtied_when;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c360f6a6c844..62f05f605fb5 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -21,6 +21,7 @@ struct backing_dev_info noop_backing_dev_info = {
 EXPORT_SYMBOL_GPL(noop_backing_dev_info);
 
 static struct class *bdi_class;
+const char *bdi_unknown_name = "(unknown)";
 
 /*
  * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
  2019-12-27 19:48 [PATCH] memcg: fix a crash in wb_workfn when a device disappears Theodore Ts'o
  2019-12-27 20:31 ` Theodore Ts'o
@ 2019-12-27 21:16   ` kbuild test robot
  2019-12-27 22:32   ` kbuild test robot
  2019-12-28  0:52 ` [PATCH -v2] " Theodore Ts'o
  3 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2019-12-27 21:16 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: kbuild-all, Linux Filesystem Development List, linux-mm,
	Theodore Ts'o

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

Hi Theodore,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/perf/core]
[also build test ERROR on linus/master v5.5-rc3 next-20191220]
[cannot apply to tytso-fscrypt/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/memcg-fix-a-crash-in-wb_workfn-when-a-device-disappears/20191228-035221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1f676247f36a4bdea134de5e8bc5041db9678c4e
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from mm/fadvise.c:16:0:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
     return dev_name(bdi->dev);
            ^~~~~~~~
            getname
>> include/linux/backing-dev.h:513:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return dev_name(bdi->dev);
            ^~~~~~~~~~~~~~~~~~
   In file included from include/linux/node.h:18:0,
                    from include/linux/cpu.h:17,
                    from include/linux/perf_event.h:50,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from mm/fadvise.c:20:
   include/linux/device.h: At top level:
>> include/linux/device.h:1370:27: error: conflicting types for 'dev_name'
    static inline const char *dev_name(const struct device *dev)
                              ^~~~~~~~
   In file included from mm/fadvise.c:16:0:
   include/linux/backing-dev.h:513:9: note: previous implicit declaration of 'dev_name' was here
     return dev_name(bdi->dev);
            ^~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from fs/super.c:32:0:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
     return dev_name(bdi->dev);
            ^~~~~~~~
            getname
>> include/linux/backing-dev.h:513:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return dev_name(bdi->dev);
            ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +513 include/linux/backing-dev.h

   508	
   509	static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
   510	{
   511		if (!bdi || !bdi->dev)
   512			return bdi_unknown_name;
 > 513		return dev_name(bdi->dev);
   514	}
   515	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7283 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
@ 2019-12-27 21:16   ` kbuild test robot
  0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2019-12-27 21:16 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: kbuild-all, Linux Filesystem Development List, linux-mm,
	Theodore Ts'o

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

Hi Theodore,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/perf/core]
[also build test ERROR on linus/master v5.5-rc3 next-20191220]
[cannot apply to tytso-fscrypt/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/memcg-fix-a-crash-in-wb_workfn-when-a-device-disappears/20191228-035221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1f676247f36a4bdea134de5e8bc5041db9678c4e
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from mm/fadvise.c:16:0:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
     return dev_name(bdi->dev);
            ^~~~~~~~
            getname
>> include/linux/backing-dev.h:513:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return dev_name(bdi->dev);
            ^~~~~~~~~~~~~~~~~~
   In file included from include/linux/node.h:18:0,
                    from include/linux/cpu.h:17,
                    from include/linux/perf_event.h:50,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from mm/fadvise.c:20:
   include/linux/device.h: At top level:
>> include/linux/device.h:1370:27: error: conflicting types for 'dev_name'
    static inline const char *dev_name(const struct device *dev)
                              ^~~~~~~~
   In file included from mm/fadvise.c:16:0:
   include/linux/backing-dev.h:513:9: note: previous implicit declaration of 'dev_name' was here
     return dev_name(bdi->dev);
            ^~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from fs/super.c:32:0:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
     return dev_name(bdi->dev);
            ^~~~~~~~
            getname
>> include/linux/backing-dev.h:513:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return dev_name(bdi->dev);
            ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +513 include/linux/backing-dev.h

   508	
   509	static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
   510	{
   511		if (!bdi || !bdi->dev)
   512			return bdi_unknown_name;
 > 513		return dev_name(bdi->dev);
   514	}
   515	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7283 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
@ 2019-12-27 21:16   ` kbuild test robot
  0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2019-12-27 21:16 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]

Hi Theodore,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/perf/core]
[also build test ERROR on linus/master v5.5-rc3 next-20191220]
[cannot apply to tytso-fscrypt/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/memcg-fix-a-crash-in-wb_workfn-when-a-device-disappears/20191228-035221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1f676247f36a4bdea134de5e8bc5041db9678c4e
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from mm/fadvise.c:16:0:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
     return dev_name(bdi->dev);
            ^~~~~~~~
            getname
>> include/linux/backing-dev.h:513:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return dev_name(bdi->dev);
            ^~~~~~~~~~~~~~~~~~
   In file included from include/linux/node.h:18:0,
                    from include/linux/cpu.h:17,
                    from include/linux/perf_event.h:50,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from mm/fadvise.c:20:
   include/linux/device.h: At top level:
>> include/linux/device.h:1370:27: error: conflicting types for 'dev_name'
    static inline const char *dev_name(const struct device *dev)
                              ^~~~~~~~
   In file included from mm/fadvise.c:16:0:
   include/linux/backing-dev.h:513:9: note: previous implicit declaration of 'dev_name' was here
     return dev_name(bdi->dev);
            ^~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from fs/super.c:32:0:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
     return dev_name(bdi->dev);
            ^~~~~~~~
            getname
>> include/linux/backing-dev.h:513:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
     return dev_name(bdi->dev);
            ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +513 include/linux/backing-dev.h

   508	
   509	static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
   510	{
   511		if (!bdi || !bdi->dev)
   512			return bdi_unknown_name;
 > 513		return dev_name(bdi->dev);
   514	}
   515	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 7283 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
  2019-12-27 21:16   ` kbuild test robot
  (?)
@ 2019-12-27 21:19     ` Theodore Y. Ts'o
  -1 siblings, 0 replies; 16+ messages in thread
From: Theodore Y. Ts'o @ 2019-12-27 21:19 UTC (permalink / raw)
  To: kbuild test robot; +Cc: kbuild-all, Linux Filesystem Development List, linux-mm

On Sat, Dec 28, 2019 at 05:16:49AM +0800, kbuild test robot wrote:
> Hi Theodore,
> 
> I love your patch! Yet something to improve:
> 
>    In file included from mm/fadvise.c:16:0:
>    include/linux/backing-dev.h: In function 'bdi_dev_name':
> >> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
>      return dev_name(bdi->dev);
>             ^~~~~~~~
>             getname

Already fixed in the V2 version of the patch.  (Which I also forgot to label as V2, sigh...)


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
@ 2019-12-27 21:19     ` Theodore Y. Ts'o
  0 siblings, 0 replies; 16+ messages in thread
From: Theodore Y. Ts'o @ 2019-12-27 21:19 UTC (permalink / raw)
  To: kbuild test robot; +Cc: kbuild-all, Linux Filesystem Development List, linux-mm

On Sat, Dec 28, 2019 at 05:16:49AM +0800, kbuild test robot wrote:
> Hi Theodore,
> 
> I love your patch! Yet something to improve:
> 
>    In file included from mm/fadvise.c:16:0:
>    include/linux/backing-dev.h: In function 'bdi_dev_name':
> >> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
>      return dev_name(bdi->dev);
>             ^~~~~~~~
>             getname

Already fixed in the V2 version of the patch.  (Which I also forgot to label as V2, sigh...)



^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
@ 2019-12-27 21:19     ` Theodore Y. Ts'o
  0 siblings, 0 replies; 16+ messages in thread
From: Theodore Y. Ts'o @ 2019-12-27 21:19 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

On Sat, Dec 28, 2019 at 05:16:49AM +0800, kbuild test robot wrote:
> Hi Theodore,
> 
> I love your patch! Yet something to improve:
> 
>    In file included from mm/fadvise.c:16:0:
>    include/linux/backing-dev.h: In function 'bdi_dev_name':
> >> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name'; did you mean 'getname'? [-Werror=implicit-function-declaration]
>      return dev_name(bdi->dev);
>             ^~~~~~~~
>             getname

Already fixed in the V2 version of the patch.  (Which I also forgot to label as V2, sigh...)

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
  2019-12-27 19:48 [PATCH] memcg: fix a crash in wb_workfn when a device disappears Theodore Ts'o
  2019-12-27 20:31 ` Theodore Ts'o
@ 2019-12-27 22:32   ` kbuild test robot
  2019-12-27 22:32   ` kbuild test robot
  2019-12-28  0:52 ` [PATCH -v2] " Theodore Ts'o
  3 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2019-12-27 22:32 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: kbuild-all, Linux Filesystem Development List, linux-mm,
	Theodore Ts'o

[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]

Hi Theodore,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/perf/core]
[also build test ERROR on linus/master v5.5-rc3 next-20191220]
[cannot apply to tytso-fscrypt/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/memcg-fix-a-crash-in-wb_workfn-when-a-device-disappears/20191228-035221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1f676247f36a4bdea134de5e8bc5041db9678c4e
config: nds32-allnoconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from mm/fadvise.c:16:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name' [-Werror=implicit-function-declaration]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
>> include/linux/backing-dev.h:513:9: warning: returning 'int' from a function with return type 'const char *' makes pointer from integer without a cast [-Wint-conversion]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~~~~~~~~~~~
   In file included from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from include/linux/perf_event.h:50,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from mm/fadvise.c:20:
   include/linux/device.h: At top level:
   include/linux/device.h:1370:27: error: conflicting types for 'dev_name'
    1370 | static inline const char *dev_name(const struct device *dev)
         |                           ^~~~~~~~
   In file included from mm/fadvise.c:16:
   include/linux/backing-dev.h:513:9: note: previous implicit declaration of 'dev_name' was here
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from fs/super.c:32:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name' [-Werror=implicit-function-declaration]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
>> include/linux/backing-dev.h:513:9: warning: returning 'int' from a function with return type 'const char *' makes pointer from integer without a cast [-Wint-conversion]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/dev_name +513 include/linux/backing-dev.h

   508	
   509	static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
   510	{
   511		if (!bdi || !bdi->dev)
   512			return bdi_unknown_name;
 > 513		return dev_name(bdi->dev);
   514	}
   515	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5229 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
@ 2019-12-27 22:32   ` kbuild test robot
  0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2019-12-27 22:32 UTC (permalink / raw)
  To: Theodore Ts'o
  Cc: kbuild-all, Linux Filesystem Development List, linux-mm,
	Theodore Ts'o

[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]

Hi Theodore,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/perf/core]
[also build test ERROR on linus/master v5.5-rc3 next-20191220]
[cannot apply to tytso-fscrypt/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/memcg-fix-a-crash-in-wb_workfn-when-a-device-disappears/20191228-035221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1f676247f36a4bdea134de5e8bc5041db9678c4e
config: nds32-allnoconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from mm/fadvise.c:16:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name' [-Werror=implicit-function-declaration]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
>> include/linux/backing-dev.h:513:9: warning: returning 'int' from a function with return type 'const char *' makes pointer from integer without a cast [-Wint-conversion]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~~~~~~~~~~~
   In file included from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from include/linux/perf_event.h:50,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from mm/fadvise.c:20:
   include/linux/device.h: At top level:
   include/linux/device.h:1370:27: error: conflicting types for 'dev_name'
    1370 | static inline const char *dev_name(const struct device *dev)
         |                           ^~~~~~~~
   In file included from mm/fadvise.c:16:
   include/linux/backing-dev.h:513:9: note: previous implicit declaration of 'dev_name' was here
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from fs/super.c:32:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name' [-Werror=implicit-function-declaration]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
>> include/linux/backing-dev.h:513:9: warning: returning 'int' from a function with return type 'const char *' makes pointer from integer without a cast [-Wint-conversion]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/dev_name +513 include/linux/backing-dev.h

   508	
   509	static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
   510	{
   511		if (!bdi || !bdi->dev)
   512			return bdi_unknown_name;
 > 513		return dev_name(bdi->dev);
   514	}
   515	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5229 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix a crash in wb_workfn when a device disappears
@ 2019-12-27 22:32   ` kbuild test robot
  0 siblings, 0 replies; 16+ messages in thread
From: kbuild test robot @ 2019-12-27 22:32 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3661 bytes --]

Hi Theodore,

I love your patch! Yet something to improve:

[auto build test ERROR on tip/perf/core]
[also build test ERROR on linus/master v5.5-rc3 next-20191220]
[cannot apply to tytso-fscrypt/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/memcg-fix-a-crash-in-wb_workfn-when-a-device-disappears/20191228-035221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 1f676247f36a4bdea134de5e8bc5041db9678c4e
config: nds32-allnoconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   In file included from mm/fadvise.c:16:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name' [-Werror=implicit-function-declaration]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
>> include/linux/backing-dev.h:513:9: warning: returning 'int' from a function with return type 'const char *' makes pointer from integer without a cast [-Wint-conversion]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~~~~~~~~~~~
   In file included from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from include/linux/perf_event.h:50,
                    from include/linux/trace_events.h:10,
                    from include/trace/syscall.h:7,
                    from include/linux/syscalls.h:85,
                    from mm/fadvise.c:20:
   include/linux/device.h: At top level:
   include/linux/device.h:1370:27: error: conflicting types for 'dev_name'
    1370 | static inline const char *dev_name(const struct device *dev)
         |                           ^~~~~~~~
   In file included from mm/fadvise.c:16:
   include/linux/backing-dev.h:513:9: note: previous implicit declaration of 'dev_name' was here
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from fs/super.c:32:
   include/linux/backing-dev.h: In function 'bdi_dev_name':
>> include/linux/backing-dev.h:513:9: error: implicit declaration of function 'dev_name' [-Werror=implicit-function-declaration]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~
>> include/linux/backing-dev.h:513:9: warning: returning 'int' from a function with return type 'const char *' makes pointer from integer without a cast [-Wint-conversion]
     513 |  return dev_name(bdi->dev);
         |         ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/dev_name +513 include/linux/backing-dev.h

   508	
   509	static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
   510	{
   511		if (!bdi || !bdi->dev)
   512			return bdi_unknown_name;
 > 513		return dev_name(bdi->dev);
   514	}
   515	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 5229 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH -v2] memcg: fix a crash in wb_workfn when a device disappears
  2019-12-27 19:48 [PATCH] memcg: fix a crash in wb_workfn when a device disappears Theodore Ts'o
                   ` (2 preceding siblings ...)
  2019-12-27 22:32   ` kbuild test robot
@ 2019-12-28  0:52 ` Theodore Ts'o
  2020-01-03 17:15   ` Theodore Y. Ts'o
  2020-01-07 23:33   ` Andrew Morton
  3 siblings, 2 replies; 16+ messages in thread
From: Theodore Ts'o @ 2019-12-28  0:52 UTC (permalink / raw)
  To: Linux Filesystem Development List, linux-mm; +Cc: Theodore Ts'o

Without memcg, there is a one-to-one mapping between the bdi and
bdi_writeback structures.  In this world, things are fairly
straightforward; the first thing bdi_unregister() does is to shutdown
the bdi_writeback structure (or wb), and part of that writeback
ensures that no other work queued against the wb, and that the wb is
fully drained.

With memcg, however, there is a one-to-many relationship between the
bdi and bdi_writeback structures; that is, there are multiple wb
objects which can all point to a single bdi.  There is a refcount
which prevents the bdi object from being released (and hence,
unregistered).  So in theory, the bdi_unregister() *should* only get
called once its refcount goes to zero (bdi_put will drop the refcount,
and when it is zero, release_bdi gets called, which calls
bdi_unregister).

Unfortunately, del_gendisk() in block/gen_hd.c never got the memo
about the Brave New memcg World, and calls bdi_unregister directly.
It does this without informing the file system, or the memcg code, or
anything else.  This causes the root wb associated with the bdi to be
unregistered, but none of the memcg-specific wb's are shutdown.  So when
one of these wb's are woken up to do delayed work, they try to
dereference their wb->bdi->dev to fetch the device name, but
unfortunately bdi->dev is now NULL, thanks to the bdi_unregister()
called by del_gendisk().   As a result, *boom*.

Fortunately, it looks like the rest of the writeback path is perfectly
happy with bdi->dev and bdi->owner being NULL, so the simplest fix is
to create a bdi_dev_name() function which can handle bdi->dev being
NULL.  This also allows us to bulletproof the writeback tracepoints to
prevent them from dereferencing a NULL pointer and crashing the kernel
if one is tracing with memcg's enabled, and an iSCSI device dies or a
USB storage stick is pulled.

Previous-Version-Link: https://lore.kernel.org/r/20191227194829.150110-1-tytso@mit.edu
Google-Bug-Id: 145475544
Tested: fs smoke test
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---

Notes:
    v2: add #include for linux/device.h

 fs/fs-writeback.c                |  2 +-
 include/linux/backing-dev.h      | 10 +++++++++
 include/trace/events/writeback.h | 37 +++++++++++++++-----------------
 mm/backing-dev.c                 |  1 +
 4 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 335607b8c5c0..76ac9c7d32ec 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2063,7 +2063,7 @@ void wb_workfn(struct work_struct *work)
 						struct bdi_writeback, dwork);
 	long pages_written;
 
-	set_worker_desc("flush-%s", dev_name(wb->bdi->dev));
+	set_worker_desc("flush-%s", bdi_dev_name(wb->bdi));
 	current->flags |= PF_SWAPWRITE;
 
 	if (likely(!current_is_workqueue_rescuer() ||
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 97967ce06de3..f88197c1ffc2 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -13,6 +13,7 @@
 #include <linux/fs.h>
 #include <linux/sched.h>
 #include <linux/blkdev.h>
+#include <linux/device.h>
 #include <linux/writeback.h>
 #include <linux/blk-cgroup.h>
 #include <linux/backing-dev-defs.h>
@@ -504,4 +505,13 @@ static inline int bdi_rw_congested(struct backing_dev_info *bdi)
 				  (1 << WB_async_congested));
 }
 
+extern const char *bdi_unknown_name;
+
+static inline const char *bdi_dev_name(struct backing_dev_info *bdi)
+{
+	if (!bdi || !bdi->dev)
+		return bdi_unknown_name;
+	return dev_name(bdi->dev);
+}
+
 #endif	/* _LINUX_BACKING_DEV_H */
diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index ef50be4e5e6c..d94def25e4dc 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -67,8 +67,8 @@ DECLARE_EVENT_CLASS(writeback_page_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    mapping ? dev_name(inode_to_bdi(mapping->host)->dev) : "(unknown)",
-			    32);
+			    bdi_dev_name(mapping ? inode_to_bdi(mapping->host) :
+					 NULL), 32);
 		__entry->ino = mapping ? mapping->host->i_ino : 0;
 		__entry->index = page->index;
 	),
@@ -111,8 +111,7 @@ DECLARE_EVENT_CLASS(writeback_dirty_inode_template,
 		struct backing_dev_info *bdi = inode_to_bdi(inode);
 
 		/* may be called for files on pseudo FSes w/ unregistered bdi */
-		strscpy_pad(__entry->name,
-			    bdi->dev ? dev_name(bdi->dev) : "(unknown)", 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->flags		= flags;
@@ -193,7 +192,7 @@ TRACE_EVENT(inode_foreign_history,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name, dev_name(inode_to_bdi(inode)->dev), 32);
+		strncpy(__entry->name, bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->cgroup_ino	= __trace_wbc_assign_cgroup(wbc);
 		__entry->history	= history;
@@ -222,7 +221,7 @@ TRACE_EVENT(inode_switch_wbs,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name,	dev_name(old_wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(old_wb->bdi), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->old_cgroup_ino	= __trace_wb_assign_cgroup(old_wb);
 		__entry->new_cgroup_ino	= __trace_wb_assign_cgroup(new_wb);
@@ -255,7 +254,7 @@ TRACE_EVENT(track_foreign_dirty,
 		struct address_space *mapping = page_mapping(page);
 		struct inode *inode = mapping ? mapping->host : NULL;
 
-		strncpy(__entry->name,	dev_name(wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(wb->bdi), 32);
 		__entry->bdi_id		= wb->bdi->id;
 		__entry->ino		= inode ? inode->i_ino : 0;
 		__entry->memcg_id	= wb->memcg_css->id;
@@ -288,7 +287,7 @@ TRACE_EVENT(flush_foreign,
 	),
 
 	TP_fast_assign(
-		strncpy(__entry->name,	dev_name(wb->bdi->dev), 32);
+		strncpy(__entry->name,	bdi_dev_name(wb->bdi), 32);
 		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
 		__entry->frn_bdi_id	= frn_bdi_id;
 		__entry->frn_memcg_id	= frn_memcg_id;
@@ -318,7 +317,7 @@ DECLARE_EVENT_CLASS(writeback_write_inode_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->sync_mode	= wbc->sync_mode;
 		__entry->cgroup_ino	= __trace_wbc_assign_cgroup(wbc);
@@ -361,9 +360,7 @@ DECLARE_EVENT_CLASS(writeback_work_class,
 		__field(ino_t, cgroup_ino)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name,
-			    wb->bdi->dev ? dev_name(wb->bdi->dev) :
-			    "(unknown)", 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->nr_pages = work->nr_pages;
 		__entry->sb_dev = work->sb ? work->sb->s_dev : 0;
 		__entry->sync_mode = work->sync_mode;
@@ -416,7 +413,7 @@ DECLARE_EVENT_CLASS(writeback_class,
 		__field(ino_t, cgroup_ino)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->cgroup_ino = __trace_wb_assign_cgroup(wb);
 	),
 	TP_printk("bdi %s: cgroup_ino=%lu",
@@ -438,7 +435,7 @@ TRACE_EVENT(writeback_bdi_register,
 		__array(char, name, 32)
 	),
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 	),
 	TP_printk("bdi %s",
 		__entry->name
@@ -463,7 +460,7 @@ DECLARE_EVENT_CLASS(wbc_class,
 	),
 
 	TP_fast_assign(
-		strscpy_pad(__entry->name, dev_name(bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(bdi), 32);
 		__entry->nr_to_write	= wbc->nr_to_write;
 		__entry->pages_skipped	= wbc->pages_skipped;
 		__entry->sync_mode	= wbc->sync_mode;
@@ -514,7 +511,7 @@ TRACE_EVENT(writeback_queue_io,
 	),
 	TP_fast_assign(
 		unsigned long *older_than_this = work->older_than_this;
-		strscpy_pad(__entry->name, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->name, bdi_dev_name(wb->bdi), 32);
 		__entry->older	= older_than_this ?  *older_than_this : 0;
 		__entry->age	= older_than_this ?
 				  (jiffies - *older_than_this) * 1000 / HZ : -1;
@@ -600,7 +597,7 @@ TRACE_EVENT(bdi_dirty_ratelimit,
 	),
 
 	TP_fast_assign(
-		strscpy_pad(__entry->bdi, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
 		__entry->write_bw	= KBps(wb->write_bandwidth);
 		__entry->avg_write_bw	= KBps(wb->avg_write_bandwidth);
 		__entry->dirty_rate	= KBps(dirty_rate);
@@ -665,7 +662,7 @@ TRACE_EVENT(balance_dirty_pages,
 
 	TP_fast_assign(
 		unsigned long freerun = (thresh + bg_thresh) / 2;
-		strscpy_pad(__entry->bdi, dev_name(wb->bdi->dev), 32);
+		strscpy_pad(__entry->bdi, bdi_dev_name(wb->bdi), 32);
 
 		__entry->limit		= global_wb_domain.dirty_limit;
 		__entry->setpoint	= (global_wb_domain.dirty_limit +
@@ -726,7 +723,7 @@ TRACE_EVENT(writeback_sb_inodes_requeue,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->dirtied_when	= inode->dirtied_when;
@@ -800,7 +797,7 @@ DECLARE_EVENT_CLASS(writeback_single_inode_template,
 
 	TP_fast_assign(
 		strscpy_pad(__entry->name,
-			    dev_name(inode_to_bdi(inode)->dev), 32);
+			    bdi_dev_name(inode_to_bdi(inode)), 32);
 		__entry->ino		= inode->i_ino;
 		__entry->state		= inode->i_state;
 		__entry->dirtied_when	= inode->dirtied_when;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c360f6a6c844..62f05f605fb5 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -21,6 +21,7 @@ struct backing_dev_info noop_backing_dev_info = {
 EXPORT_SYMBOL_GPL(noop_backing_dev_info);
 
 static struct class *bdi_class;
+const char *bdi_unknown_name = "(unknown)";
 
 /*
  * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH -v2] memcg: fix a crash in wb_workfn when a device disappears
  2019-12-28  0:52 ` [PATCH -v2] " Theodore Ts'o
@ 2020-01-03 17:15   ` Theodore Y. Ts'o
  2020-01-03 17:46     ` Chris Mason
  2020-01-07 23:33   ` Andrew Morton
  1 sibling, 1 reply; 16+ messages in thread
From: Theodore Y. Ts'o @ 2020-01-03 17:15 UTC (permalink / raw)
  To: Linux Filesystem Development List, linux-mm, Andrew Morton

On Fri, Dec 27, 2019 at 07:52:11PM -0500, Theodore Ts'o wrote:
> Without memcg, there is a one-to-one mapping between the bdi and
> bdi_writeback structures.  In this world, things are fairly
> straightforward; the first thing bdi_unregister() does is to shutdown
> the bdi_writeback structure (or wb), and part of that writeback
> ensures that no other work queued against the wb, and that the wb is
> fully drained.
> 
> With memcg, however, there is a one-to-many relationship between the
> bdi and bdi_writeback structures; that is, there are multiple wb
> objects which can all point to a single bdi.  There is a refcount
> which prevents the bdi object from being released (and hence,
> unregistered).  So in theory, the bdi_unregister() *should* only get
> called once its refcount goes to zero (bdi_put will drop the refcount,
> and when it is zero, release_bdi gets called, which calls
> bdi_unregister).
> 
> Unfortunately, del_gendisk() in block/gen_hd.c never got the memo
> about the Brave New memcg World, and calls bdi_unregister directly.
> It does this without informing the file system, or the memcg code, or
> anything else.  This causes the root wb associated with the bdi to be
> unregistered, but none of the memcg-specific wb's are shutdown.  So when
> one of these wb's are woken up to do delayed work, they try to
> dereference their wb->bdi->dev to fetch the device name, but
> unfortunately bdi->dev is now NULL, thanks to the bdi_unregister()
> called by del_gendisk().   As a result, *boom*.
> 
> Fortunately, it looks like the rest of the writeback path is perfectly
> happy with bdi->dev and bdi->owner being NULL, so the simplest fix is
> to create a bdi_dev_name() function which can handle bdi->dev being
> NULL.  This also allows us to bulletproof the writeback tracepoints to
> prevent them from dereferencing a NULL pointer and crashing the kernel
> if one is tracing with memcg's enabled, and an iSCSI device dies or a
> USB storage stick is pulled.
> 
> Previous-Version-Link: https://lore.kernel.org/r/20191227194829.150110-1-tytso@mit.edu
> Google-Bug-Id: 145475544
> Tested: fs smoke test
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> 
> Notes:
>     v2: add #include for linux/device.h
> 
>  fs/fs-writeback.c                |  2 +-
>  include/linux/backing-dev.h      | 10 +++++++++
>  include/trace/events/writeback.h | 37 +++++++++++++++-----------------
>  mm/backing-dev.c                 |  1 +
>  4 files changed, 29 insertions(+), 21 deletions(-)

Ping?

Any comments?  Any objections if I carry this patch[1] in the ext4
tree?  Or would it be better for Andrew to carry it in the linux-mm
tree?

[1] https://lore.kernel.org/k/20191227203117.152399-1-tytso@mit.edu

						- Ted

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH -v2] memcg: fix a crash in wb_workfn when a device disappears
  2020-01-03 17:15   ` Theodore Y. Ts'o
@ 2020-01-03 17:46     ` Chris Mason
  0 siblings, 0 replies; 16+ messages in thread
From: Chris Mason @ 2020-01-03 17:46 UTC (permalink / raw)
  To: Theodore Y. Ts'o
  Cc: Linux Filesystem Development List, linux-mm, Andrew Morton

On 3 Jan 2020, at 12:15, Theodore Y. Ts'o wrote:

> On Fri, Dec 27, 2019 at 07:52:11PM -0500, Theodore Ts'o wrote:
>>
>> Fortunately, it looks like the rest of the writeback path is 
>> perfectly
>> happy with bdi->dev and bdi->owner being NULL, so the simplest fix is
>> to create a bdi_dev_name() function which can handle bdi->dev being
>> NULL.  This also allows us to bulletproof the writeback tracepoints 
>> to
>> prevent them from dereferencing a NULL pointer and crashing the 
>> kernel
>> if one is tracing with memcg's enabled, and an iSCSI device dies or a
>> USB storage stick is pulled.
>>
>> Previous-Version-Link: 
>> https://lore.kernel.org/r/20191227194829.150110-1-tytso@mit.edu
>> Google-Bug-Id: 145475544
>> Tested: fs smoke test
>> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
>> ---
>>
>> Notes:
>>     v2: add #include for linux/device.h
>>
>>  fs/fs-writeback.c                |  2 +-
>>  include/linux/backing-dev.h      | 10 +++++++++
>>  include/trace/events/writeback.h | 37 
>> +++++++++++++++-----------------
>>  mm/backing-dev.c                 |  1 +
>>  4 files changed, 29 insertions(+), 21 deletions(-)
>
> Ping?
>
> Any comments?  Any objections if I carry this patch[1] in the ext4
> tree?  Or would it be better for Andrew to carry it in the linux-mm
> tree?
>
> [1] https://lore.kernel.org/k/20191227203117.152399-1-tytso@mit.edu

Seems sane to me, and we probably want this even if del_gendisk() 
embraces the brave new memcg world because synchronizing all of this is 
going to get messy.

-chris

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH -v2] memcg: fix a crash in wb_workfn when a device disappears
  2019-12-28  0:52 ` [PATCH -v2] " Theodore Ts'o
  2020-01-03 17:15   ` Theodore Y. Ts'o
@ 2020-01-07 23:33   ` Andrew Morton
  2020-01-08  2:12     ` Theodore Y. Ts'o
  1 sibling, 1 reply; 16+ messages in thread
From: Andrew Morton @ 2020-01-07 23:33 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Linux Filesystem Development List, linux-mm

On Fri, 27 Dec 2019 19:52:11 -0500 "Theodore Ts'o" <tytso@mit.edu> wrote:

> Without memcg, there is a one-to-one mapping between the bdi and
> bdi_writeback structures.  In this world, things are fairly
> straightforward; the first thing bdi_unregister() does is to shutdown
> the bdi_writeback structure (or wb), and part of that writeback
> ensures that no other work queued against the wb, and that the wb is
> fully drained.
> 
> With memcg, however, there is a one-to-many relationship between the
> bdi and bdi_writeback structures; that is, there are multiple wb
> objects which can all point to a single bdi.  There is a refcount
> which prevents the bdi object from being released (and hence,
> unregistered).  So in theory, the bdi_unregister() *should* only get
> called once its refcount goes to zero (bdi_put will drop the refcount,
> and when it is zero, release_bdi gets called, which calls
> bdi_unregister).
> 
> Unfortunately, del_gendisk() in block/gen_hd.c never got the memo
> about the Brave New memcg World, and calls bdi_unregister directly.
> It does this without informing the file system, or the memcg code, or
> anything else.  This causes the root wb associated with the bdi to be
> unregistered, but none of the memcg-specific wb's are shutdown.  So when
> one of these wb's are woken up to do delayed work, they try to
> dereference their wb->bdi->dev to fetch the device name, but
> unfortunately bdi->dev is now NULL, thanks to the bdi_unregister()
> called by del_gendisk().   As a result, *boom*.
> 
> Fortunately, it looks like the rest of the writeback path is perfectly
> happy with bdi->dev and bdi->owner being NULL, so the simplest fix is
> to create a bdi_dev_name() function which can handle bdi->dev being
> NULL.  This also allows us to bulletproof the writeback tracepoints to
> prevent them from dereferencing a NULL pointer and crashing the kernel
> if one is tracing with memcg's enabled, and an iSCSI device dies or a
> USB storage stick is pulled.
> 

Is hotremoval of a device while tracing writeback the only known way of
triggering this?

Is it worth a cc:stable?

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH -v2] memcg: fix a crash in wb_workfn when a device disappears
  2020-01-07 23:33   ` Andrew Morton
@ 2020-01-08  2:12     ` Theodore Y. Ts'o
  0 siblings, 0 replies; 16+ messages in thread
From: Theodore Y. Ts'o @ 2020-01-08  2:12 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Linux Filesystem Development List, linux-mm

On Tue, Jan 07, 2020 at 03:33:48PM -0800, Andrew Morton wrote:
> On Fri, 27 Dec 2019 19:52:11 -0500 "Theodore Ts'o" <tytso@mit.edu> wrote:
> 
> > Unfortunately, del_gendisk() in block/gen_hd.c never got the memo
> > about the Brave New memcg World, and calls bdi_unregister directly.
> > It does this without informing the file system, or the memcg code, or
> > anything else.  This causes the root wb associated with the bdi to be
> > unregistered, but none of the memcg-specific wb's are shutdown.  So when
> > one of these wb's are woken up to do delayed work, they try to
> > dereference their wb->bdi->dev to fetch the device name, but
> > unfortunately bdi->dev is now NULL, thanks to the bdi_unregister()
> > called by del_gendisk().   As a result, *boom*.
> > 
> > Fortunately, it looks like the rest of the writeback path is perfectly
> > happy with bdi->dev and bdi->owner being NULL, so the simplest fix is
> > to create a bdi_dev_name() function which can handle bdi->dev being
> > NULL.  This also allows us to bulletproof the writeback tracepoints to
> > prevent them from dereferencing a NULL pointer and crashing the kernel
> > if one is tracing with memcg's enabled, and an iSCSI device dies or a
> > USB storage stick is pulled.
> 
> Is hotremoval of a device while tracing writeback the only known way of
> triggering this?

The most common way of triggering this will be hotremoval of a device
while writeback with memcg enabled is going on.  It was triggering
several times a day in a heavily loaded production environment.

> Is it worth a cc:stable?

Yes, I think so.

						- Ted

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-01-08  2:12 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-27 19:48 [PATCH] memcg: fix a crash in wb_workfn when a device disappears Theodore Ts'o
2019-12-27 20:31 ` Theodore Ts'o
2019-12-27 21:16 ` kbuild test robot
2019-12-27 21:16   ` kbuild test robot
2019-12-27 21:16   ` kbuild test robot
2019-12-27 21:19   ` Theodore Y. Ts'o
2019-12-27 21:19     ` Theodore Y. Ts'o
2019-12-27 21:19     ` Theodore Y. Ts'o
2019-12-27 22:32 ` kbuild test robot
2019-12-27 22:32   ` kbuild test robot
2019-12-27 22:32   ` kbuild test robot
2019-12-28  0:52 ` [PATCH -v2] " Theodore Ts'o
2020-01-03 17:15   ` Theodore Y. Ts'o
2020-01-03 17:46     ` Chris Mason
2020-01-07 23:33   ` Andrew Morton
2020-01-08  2:12     ` Theodore Y. Ts'o

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.