From 1ef8de232bfd6162cac88e3ff238b8ad9d44e6bc Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Fri, 6 Nov 2009 18:39:52 +0000 Subject: [PATCH] kobject tricky lifetime races fixed git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@1323 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 36 ++++++-- scst/src/scst_mem.c | 16 ++-- scst/src/scst_sysfs.c | 196 +++++++++++++++++++++++++++++++++++++++++- 3 files changed, 230 insertions(+), 18 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index ed68cf009..096ce5e05 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -1145,6 +1145,12 @@ struct scst_tgt { /* Set if tgt_kobj was initialized */ unsigned int tgt_kobj_initialized:1; + /* + * Used to protect sysfs attributes to be called after this + * object was unregistered. + */ + struct rw_semaphore tgt_attr_rwsem; + struct kobject tgt_kobj; /* main targets/target kobject */ struct kobject *tgt_sess_kobj; /* target/sessions/ */ struct kobject *tgt_luns_kobj; /* target/luns/ */ @@ -1274,6 +1280,12 @@ struct scst_session { /* Set if sess_kobj was initialized */ unsigned int sess_kobj_initialized:1; + /* + * Used to protect sysfs attributes to be called after this + * object was unregistered. + */ + struct rw_semaphore sess_attr_rwsem; + struct kobject sess_kobj; /* kobject for this struct */ /* @@ -1793,6 +1805,12 @@ struct scst_device { /* Set if tgt_kobj was initialized */ unsigned int dev_kobj_initialized:1; + /* + * Used to protect sysfs attributes to be called after this + * object was unregistered. + */ + struct rw_semaphore dev_attr_rwsem; + struct kobject dev_kobj; /* kobject for this struct */ struct kobject *dev_exp_kobj; /* exported groups */ @@ -2008,8 +2026,10 @@ struct scst_aen { #endif /* - * Registers target template - * Returns 0 on success or appropriate error code otherwise + * Registers target template. + * Returns 0 on success or appropriate error code otherwise. + * + * Note: *vtt must be static! */ int __scst_register_target_template(struct scst_tgt_template *vtt, const char *version); @@ -2120,8 +2140,10 @@ void scst_unregister_session(struct scst_session *sess, int wait, void (*unreg_done_fn) (struct scst_session *sess)); /* - * Registers dev handler driver - * Returns 0 on success or appropriate error code otherwise + * Registers dev handler driver. + * Returns 0 on success or appropriate error code otherwise. + * + * Note: *dev_type must be static! */ int __scst_register_dev_driver(struct scst_dev_type *dev_type, const char *version); @@ -2136,8 +2158,10 @@ static inline int scst_register_dev_driver(struct scst_dev_type *dev_type) void scst_unregister_dev_driver(struct scst_dev_type *dev_type); /* - * Registers dev handler driver for virtual devices (eg VDISK) - * Returns 0 on success or appropriate error code otherwise + * Registers dev handler driver for virtual devices (eg VDISK). + * Returns 0 on success or appropriate error code otherwise. + * + * Note: *dev_type must be static! */ int __scst_register_virtual_dev_driver(struct scst_dev_type *dev_type, const char *version); diff --git a/scst/src/scst_mem.c b/scst/src/scst_mem.c index 23be97ac4..25baf0925 100644 --- a/scst/src/scst_mem.c +++ b/scst/src/scst_mem.c @@ -1438,8 +1438,6 @@ EXPORT_SYMBOL(sgv_pool_flush); static void sgv_pool_deinit_put(struct sgv_pool *pool) { - int i; - TRACE_ENTRY(); cancel_delayed_work_sync(&pool->sgv_purge_work); @@ -1452,12 +1450,6 @@ static void sgv_pool_deinit_put(struct sgv_pool *pool) spin_unlock_bh(&sgv_pools_lock); mutex_unlock(&sgv_pools_mutex); - for (i = 0; i < pool->max_caches; i++) { - if (pool->caches[i]) - kmem_cache_destroy(pool->caches[i]); - pool->caches[i] = NULL; - } - scst_sgv_sysfs_put(pool); /* pool can be dead here */ @@ -1541,8 +1533,16 @@ EXPORT_SYMBOL(sgv_pool_create); void sgv_pool_destroy(struct sgv_pool *pool) { + int i; + TRACE_ENTRY(); + for (i = 0; i < pool->max_caches; i++) { + if (pool->caches[i]) + kmem_cache_destroy(pool->caches[i]); + pool->caches[i] = NULL; + } + kfree(pool); TRACE_EXIT(); diff --git a/scst/src/scst_sysfs.c b/scst/src/scst_sysfs.c index c13a812de..c4d7a5905 100644 --- a/scst/src/scst_sysfs.c +++ b/scst/src/scst_sysfs.c @@ -280,14 +280,70 @@ static void scst_tgt_release(struct kobject *kobj) tgt = container_of(kobj, struct scst_tgt, tgt_kobj); + /* Let's make lockdep happy */ + up_write(&tgt->tgt_attr_rwsem); + scst_free_tgt(tgt); TRACE_EXIT(); return; } +static ssize_t scst_tgt_attr_show(struct kobject *kobj, struct attribute *attr, + char *buf) +{ + int res; + struct kobj_attribute *kobj_attr; + struct scst_tgt *tgt; + + tgt = container_of(kobj, struct scst_tgt, tgt_kobj); + + if (down_read_trylock(&tgt->tgt_attr_rwsem) == 0) { + res = -ENOENT; + goto out; + } + + kobj_attr = container_of(attr, struct kobj_attribute, attr); + + res = kobj_attr->show(kobj, kobj_attr, buf); + + up_read(&tgt->tgt_attr_rwsem); + +out: + return res; +} + +static ssize_t scst_tgt_attr_store(struct kobject *kobj, struct attribute *attr, + const char *buf, size_t count) +{ + int res; + struct kobj_attribute *kobj_attr; + struct scst_tgt *tgt; + + tgt = container_of(kobj, struct scst_tgt, tgt_kobj); + + if (down_read_trylock(&tgt->tgt_attr_rwsem) == 0) { + res = -ENOENT; + goto out; + } + + kobj_attr = container_of(attr, struct kobj_attribute, attr); + + res = kobj_attr->store(kobj, kobj_attr, buf, count); + + up_read(&tgt->tgt_attr_rwsem); + +out: + return res; +} + +static struct sysfs_ops scst_tgt_sysfs_ops = { + .show = scst_tgt_attr_show, + .store = scst_tgt_attr_store, +}; + static struct kobj_type tgt_ktype = { - .sysfs_ops = &scst_sysfs_ops, + .sysfs_ops = &scst_tgt_sysfs_ops, .release = scst_tgt_release, }; @@ -350,6 +406,8 @@ int scst_create_tgt_sysfs(struct scst_tgt *tgt) TRACE_ENTRY(); + init_rwsem(&tgt->tgt_attr_rwsem); + tgt->tgt_kobj_initialized = 1; retval = kobject_init_and_add(&tgt->tgt_kobj, &tgt_ktype, @@ -441,6 +499,8 @@ void scst_tgt_sysfs_put(struct scst_tgt *tgt) kobject_put(tgt->tgt_ini_grp_kobj); kobject_del(&tgt->tgt_kobj); + + down_write(&tgt->tgt_attr_rwsem); kobject_put(&tgt->tgt_kobj); } else scst_free_tgt(tgt); @@ -483,6 +543,9 @@ static void scst_sysfs_device_release(struct kobject *kobj) dev = container_of(kobj, struct scst_device, dev_kobj); + /* Let's make lockdep happy */ + up_write(&dev->dev_attr_rwsem); + scst_free_device(dev); TRACE_EXIT(); @@ -558,8 +621,61 @@ out: return; } +static ssize_t scst_dev_attr_show(struct kobject *kobj, struct attribute *attr, + char *buf) +{ + int res; + struct kobj_attribute *kobj_attr; + struct scst_device *dev; + + dev = container_of(kobj, struct scst_device, dev_kobj); + + if (down_read_trylock(&dev->dev_attr_rwsem) == 0) { + res = -ENOENT; + goto out; + } + + kobj_attr = container_of(attr, struct kobj_attribute, attr); + + res = kobj_attr->show(kobj, kobj_attr, buf); + + up_read(&dev->dev_attr_rwsem); + +out: + return res; +} + +static ssize_t scst_dev_attr_store(struct kobject *kobj, struct attribute *attr, + const char *buf, size_t count) +{ + int res; + struct kobj_attribute *kobj_attr; + struct scst_device *dev; + + dev = container_of(kobj, struct scst_device, dev_kobj); + + if (down_read_trylock(&dev->dev_attr_rwsem) == 0) { + res = -ENOENT; + goto out; + } + + kobj_attr = container_of(attr, struct kobj_attribute, attr); + + res = kobj_attr->store(kobj, kobj_attr, buf, count); + + up_read(&dev->dev_attr_rwsem); + +out: + return res; +} + +static struct sysfs_ops scst_dev_sysfs_ops = { + .show = scst_dev_attr_show, + .store = scst_dev_attr_store, +}; + static struct kobj_type scst_device_ktype = { - .sysfs_ops = &scst_sysfs_ops, + .sysfs_ops = &scst_dev_sysfs_ops, .release = scst_sysfs_device_release, .default_attrs = scst_device_attrs, }; @@ -570,6 +686,8 @@ int scst_create_device_sysfs(struct scst_device *dev) TRACE_ENTRY(); + init_rwsem(&dev->dev_attr_rwsem); + dev->dev_kobj_initialized = 1; retval = kobject_init_and_add(&dev->dev_kobj, &scst_device_ktype, @@ -618,6 +736,8 @@ void scst_device_sysfs_put(struct scst_device *dev) kobject_put(dev->dev_exp_kobj); } kobject_del(&dev->dev_kobj); + + down_write(&dev->dev_attr_rwsem); kobject_put(&dev->dev_kobj); } else scst_free_device(dev); @@ -671,14 +791,70 @@ static void scst_sysfs_session_release(struct kobject *kobj) sess = container_of(kobj, struct scst_session, sess_kobj); + /* Let's make lockdep happy */ + up_write(&sess->sess_attr_rwsem); + scst_release_session(sess); TRACE_EXIT(); return; } +static ssize_t scst_sess_attr_show(struct kobject *kobj, struct attribute *attr, + char *buf) +{ + int res; + struct kobj_attribute *kobj_attr; + struct scst_session *sess; + + sess = container_of(kobj, struct scst_session, sess_kobj); + + if (down_read_trylock(&sess->sess_attr_rwsem) == 0) { + res = -ENOENT; + goto out; + } + + kobj_attr = container_of(attr, struct kobj_attribute, attr); + + res = kobj_attr->show(kobj, kobj_attr, buf); + + up_read(&sess->sess_attr_rwsem); + +out: + return res; +} + +static ssize_t scst_sess_attr_store(struct kobject *kobj, struct attribute *attr, + const char *buf, size_t count) +{ + int res; + struct kobj_attribute *kobj_attr; + struct scst_session *sess; + + sess = container_of(kobj, struct scst_session, sess_kobj); + + if (down_read_trylock(&sess->sess_attr_rwsem) == 0) { + res = -ENOENT; + goto out; + } + + kobj_attr = container_of(attr, struct kobj_attribute, attr); + + res = kobj_attr->store(kobj, kobj_attr, buf, count); + + up_read(&sess->sess_attr_rwsem); + +out: + return res; +} + +static struct sysfs_ops scst_sess_sysfs_ops = { + .show = scst_sess_attr_show, + .store = scst_sess_attr_store, +}; + static struct kobj_type scst_session_ktype = { - .sysfs_ops = &scst_sysfs_ops, + .sysfs_ops = &scst_sess_sysfs_ops, .release = scst_sysfs_session_release, .default_attrs = scst_session_attrs, }; @@ -699,7 +875,7 @@ restart: if (!s->sess_kobj_initialized) continue; - if (strcmp(name, s->sess_kobj.name) == 0) { + if (strcmp(name, kobject_name(&s->sess_kobj)) == 0) { if (s == sess) continue; @@ -723,6 +899,8 @@ restart: } } + init_rwsem(&sess->sess_attr_rwsem); + sess->sess_kobj_initialized = 1; retval = kobject_init_and_add(&sess->sess_kobj, &scst_session_ktype, @@ -765,6 +943,8 @@ void scst_sess_sysfs_put(struct scst_session *sess) if (sess->sess_kobj_initialized) { kobject_del(&sess->sess_kobj); + + down_write(&sess->sess_attr_rwsem); kobject_put(&sess->sess_kobj); } else scst_release_session(sess); @@ -1854,6 +2034,14 @@ void __exit scst_sysfs_cleanup(void) kobject_put(&scst_sysfs_root_kobj); wait_for_completion(&scst_sysfs_root_release_completion); + /* + * There is a race, when in the release() schedule happens just after + * calling complete(), so if we exit and unload scst module immediately, + * there will be oops there. So let's give it a chance to quit + * gracefully. Unfortunately, current kobjects implementation + * doesn't allow better ways to handle it. + */ + msleep(3000); PRINT_INFO("%s", "Exiting SCST sysfs hierarchy done");