Re: [PATCH 1/2] zram: fix crashes due to use of cpu hotplug multistate

There are two issues.

quoted

Below specific example is one of them and every sysfs code in zram
could access freed object(e.g., zram->something).
And you are claiming there isn't good way to fix it in kernfs(please
add it in the description, too) even though it's general problem.

Correct, the easier route would have been through the block layer as
its the one adding our syfs attributes for us but even it canno deal
with this race on behalf of drivers given the currently exposed
semantics on kernfs.

quoted

(If we had, we may detect the race and make zram_remove_cb fails
so unloading modulies fails, finally)

So, shouldn't we introcude a global rw_semaphore?

destroy_devices
  class_unregister
  down_write(&zram_unload);
  idr_for_each(zram_remove_cb);
  up_write(&zram_unload);

And every sysfs code hold the lock with down_read in the first place
and release the lock right before return. So, introduce a zram sysfs
wrapper to centralize all of locking logic.

Actually that does work but only if we use write lock attempts so to
be able to knock two birds with one stone, so to address the deadlock
with sysfs attribute removal. We're not asking politely to read some
value off of a zram devices with these locks, we're ensuring to not
run if our driver is going to be removed, so replacing the
try_module_get() with something similar.

quoted

Does it make sense?

quoted

And so we can have:

static void destroy_devices(void)
{
	class_unregister(&zram_control_class);
	idr_for_each(&zram_index_idr, &zram_remove_cb, NULL);
	zram_debugfs_destroy();
	idr_destroy(&zram_index_idr);
	unregister_blkdev(zram_major, "zram");
	cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE);
}

While destroy_devices() runs we can have this race:


CPU 1                            CPU 2

class_unregister(...);
idr_for_each(...);

				  I think sysfs was already remove here.

quoted

zram_debugfs_destroy();
                                disksize_store(...)
idr_destroy(...);
unregister_blkdev(...);
cpuhp_remove_multi_state(...);

So,

	CPU 1                            CPU 2
 
destroy_devices
..
				 disksize_store()
				   zcomp_create
				   zcomp_init
idr_for_each(zram_remove_cb
  zram_remove
  zram_reset
  zcomp_destroy
  cpuhp_state_remove_instance
				   cpuhp_state_add_instance
..

cpuhp_remove_multi_state(...)
  Detect!
    kernel: Error: Removing state 63 which has instances left.

Yup true.

quoted

quoted

The warning comes up on cpuhp_remove_multi_state() when it sees
that the state for CPUHP_ZCOMP_PREPARE does not have an empty
instance linked list.

After the idr_destory() its also easy to run into a crash. The
above predicament also leads to memory leaks.

And so we need to have a state machine for when we're up and when
we're going down generically.

Let me know if it makes sense now, if so I can amend the commit log
accordingly.

Yub, It's much better. Let's have it in the commit log.

Sure OK, well the following is what I end up with. With this approach
only one patch is needed.

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a711a2e2a794..3b86ee94309e 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c

@@ -41,6 +41,15 @@ static DEFINE_IDR(zram_index_idr);
 /* idr index must be protected */
 static DEFINE_MUTEX(zram_index_mutex);
 
+/*
+ * Protects against:
+ * a) Hotplug cpu multistate races against compression algorithm removals
+ *    and additions prior to its removal of the zram CPU hotplug callback
+ * b) Deadlock which is possible when sysfs attributes are used and we
+ *    share a lock on removal.
+ */
+DECLARE_RWSEM(zram_unload);
+
 static int zram_major;
 static const char *default_compressor = CONFIG_ZRAM_DEF_COMP;
 

@@ -1723,6 +1732,9 @@ static ssize_t disksize_store(struct device *dev,
 	if (!disksize)
 		return -EINVAL;
 
+	if (!down_write_trylock(&zram_unload))
+		return -ENODEV;
+
 	down_write(&zram->init_lock);
 	if (init_done(zram)) {
 		pr_info("Cannot change disksize for initialized device\n");

@@ -1748,6 +1760,7 @@ static ssize_t disksize_store(struct device *dev,
 	zram->disksize = disksize;
 	set_capacity_and_notify(zram->disk, zram->disksize >> SECTOR_SHIFT);
 	up_write(&zram->init_lock);
+	up_write(&zram_unload);
 
 	return len;
 

@@ -1755,6 +1768,7 @@ static ssize_t disksize_store(struct device *dev,
 	zram_meta_free(zram, disksize);
 out_unlock:
 	up_write(&zram->init_lock);
+	up_write(&zram_unload);
 	return err;
 }
 

@@ -1773,14 +1787,17 @@ static ssize_t reset_store(struct device *dev,
 	if (!do_reset)
 		return -EINVAL;
 
+	if (!down_write_trylock(&zram_unload))
+		return -ENODEV;
+
 	zram = dev_to_zram(dev);
 	bdev = zram->disk->part0;
 
 	mutex_lock(&bdev->bd_mutex);
 	/* Do not reset an active device or claimed device */
 	if (bdev->bd_openers || zram->claim) {
-		mutex_unlock(&bdev->bd_mutex);
-		return -EBUSY;
+		len = -EBUSY;
+		goto out;
 	}
 
 	/* From now on, anyone can't open /dev/zram[0-9] */

@@ -1793,7 +1810,10 @@ static ssize_t reset_store(struct device *dev,
 
 	mutex_lock(&bdev->bd_mutex);
 	zram->claim = false;
+
+out:
 	mutex_unlock(&bdev->bd_mutex);
+	up_write(&zram_unload);
 
 	return len;
 }

@@ -2015,10 +2035,15 @@ static ssize_t hot_add_show(struct class *class,
 {
 	int ret;
 
+	if (!down_write_trylock(&zram_unload))
+		return -ENODEV;
+
 	mutex_lock(&zram_index_mutex);
 	ret = zram_add();
 	mutex_unlock(&zram_index_mutex);
 
+	up_write(&zram_unload);
+
 	if (ret < 0)
 		return ret;
 	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);

@@ -2041,6 +2066,9 @@ static ssize_t hot_remove_store(struct class *class,
 	if (dev_id < 0)
 		return -EINVAL;
 
+	if (!down_write_trylock(&zram_unload))
+		return -ENODEV;
+
 	mutex_lock(&zram_index_mutex);
 
 	zram = idr_find(&zram_index_idr, dev_id);

@@ -2053,6 +2081,7 @@ static ssize_t hot_remove_store(struct class *class,
 	}
 
 	mutex_unlock(&zram_index_mutex);
+	up_write(&zram_unload);
 	return ret ? ret : count;
 }
 static CLASS_ATTR_WO(hot_remove);

@@ -2078,12 +2107,14 @@ static int zram_remove_cb(int id, void *ptr, void *data)
 
 static void destroy_devices(void)
 {
+	down_write(&zram_unload);
 	class_unregister(&zram_control_class);
 	idr_for_each(&zram_index_idr, &zram_remove_cb, NULL);
 	zram_debugfs_destroy();
 	idr_destroy(&zram_index_idr);
 	unregister_blkdev(zram_major, "zram");
 	cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE);
+	up_write(&zram_unload);
 }
 
 static int __init zram_init(void)

@@ -2095,10 +2126,13 @@ static int __init zram_init(void)
 	if (ret < 0)
 		return ret;
 
+	down_write(&zram_unload);
+
 	ret = class_register(&zram_control_class);
 	if (ret) {
 		pr_err("Unable to register zram-control class\n");
 		cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE);
+		up_write(&zram_unload);
 		return ret;
 	}
 

@@ -2108,6 +2142,7 @@ static int __init zram_init(void)
 		pr_err("Unable to get major number\n");
 		class_unregister(&zram_control_class);
 		cpuhp_remove_multi_state(CPUHP_ZCOMP_PREPARE);
+		up_write(&zram_unload);
 		return -EBUSY;
 	}
 

@@ -2119,10 +2154,12 @@ static int __init zram_init(void)
 			goto out_error;
 		num_devices--;
 	}
+	up_write(&zram_unload);
 
 	return 0;
 
 out_error:
+	up_write(&zram_unload);
 	destroy_devices();
 	return ret;
 }