From f381dbb74c84248ec1ff41c605ae617c6cb2aa61 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Thu, 26 Nov 2009 18:39:13 +0000 Subject: [PATCH] Device sysfs locking reconsidered to remove recently introduced deadlock possibility. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@1361 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 3 + scst/src/dev_handlers/scst_vdisk.c | 269 ++++++++++++----------------- scst/src/scst_lib.c | 3 +- scst/src/scst_main.c | 37 ++-- scst/src/scst_sysfs.c | 27 +++ 5 files changed, 167 insertions(+), 172 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index 8234ba9cf..3785ac94f 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -1811,6 +1811,9 @@ struct scst_device { */ struct rw_semaphore dev_attr_rwsem; + /* Used to serialize all the device sysfs calls */ + struct mutex dev_sysfs_mutex; + struct kobject dev_kobj; /* kobject for this struct */ struct kobject *dev_exp_kobj; /* exported groups */ diff --git a/scst/src/dev_handlers/scst_vdisk.c b/scst/src/dev_handlers/scst_vdisk.c index 87fa19cd2..15ac6f773 100644 --- a/scst/src/dev_handlers/scst_vdisk.c +++ b/scst/src/dev_handlers/scst_vdisk.c @@ -277,12 +277,6 @@ struct vdev_type_funcs { void (*vdev_del) (struct scst_vdisk_dev *virt_dev); struct scst_vdisk_dev *(*vdev_find) (const char *name); - int (*parse_cmd) (struct vdev_type *vdt, char *p, int *action); - - /* Supposed to be called under scst_vdisk_mutex */ - int (*perform_cmd) (struct vdev_type *vdt, int action, char *p, - char *name); - int (*parse_option) (struct scst_vdisk_dev *virt_dev, char *p); int (*pre_register) (struct scst_vdisk_dev *virt_dev); @@ -404,6 +398,9 @@ static ssize_t vdisk_sysfs_filename_show(struct kobject *kobj, static ssize_t vdisk_sysfs_resync_size_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count); +static ssize_t vcdrom_sysfs_filename_store(struct kobject *kobj, + struct kobj_attribute *attr, const char *buf, size_t count); + static struct kobj_attribute vdisk_size_attr = __ATTR(size, S_IRUGO, vdisk_sysfs_size_show, NULL); static struct kobj_attribute vdisk_blocksize_attr = @@ -423,6 +420,10 @@ static struct kobj_attribute vdisk_filename_attr = static struct kobj_attribute vdisk_resync_size_attr = __ATTR(resync_size, S_IWUSR, NULL, vdisk_sysfs_resync_size_store); +static struct kobj_attribute vcdrom_filename_attr = + __ATTR(filename, S_IRUGO|S_IWUSR, vdisk_sysfs_filename_show, + vcdrom_sysfs_filename_store); + static const struct attribute *vdisk_fileio_attrs[] = { &vdisk_size_attr.attr, &vdisk_blocksize_attr.attr, @@ -457,12 +458,13 @@ static const struct attribute *vdisk_nullio_attrs[] = { static const struct attribute *vcdrom_attrs[] = { &vdisk_size_attr.attr, &vdisk_removable_attr.attr, - &vdisk_filename_attr.attr, + &vcdrom_filename_attr.attr, NULL, }; #endif /* CONFIG_SCST_PROC */ +/* Protects vdisks addition/deletion and related activities, like search */ static DEFINE_MUTEX(scst_vdisk_mutex); /* Both protected by scst_vdisk_mutex */ @@ -2923,12 +2925,16 @@ static void vdisk_report_registering(const struct scst_vdisk_dev *virt_dev) return; } -/* scst_vdisk_mutex supposed to be held */ -static int __vdisk_resync_size(struct scst_vdisk_dev *virt_dev) +static int vdisk_resync_size(struct scst_vdisk_dev *virt_dev) { loff_t file_size; int res = 0; + /* + * There's no need in any lock here, because SCST core serializes + * all device sysfs calls. + */ + if (!virt_dev->nullio) { res = vdisk_get_file_size(virt_dev->file_name, virt_dev->blockio, &file_size); @@ -2969,26 +2975,6 @@ out: return res; } -/* scst_vdisk_mutex supposed to be held */ -static int vdisk_resync_size(struct vdev_type *vdt, char *p, const char *name) -{ - struct scst_vdisk_dev *virt_dev; - int res; - - virt_dev = vdt->vfns.vdev_find(name); - if (virt_dev == NULL) { - PRINT_ERROR("Device %s not found", name); - res = -EINVAL; - goto out; - } - - res = __vdisk_resync_size(virt_dev); - -out: - TRACE_EXIT_RES(res); - return res; -} - static void vdev_init(struct vdev_type *vdt, struct scst_vdisk_dev *virt_dev) { memset(virt_dev, 0, sizeof(*virt_dev)); @@ -3393,37 +3379,6 @@ static struct scst_vdisk_dev *vcdrom_find(const char *name) return res; } -static int vdisk_parse_cmd(struct vdev_type *vdt, char *p, int *action) -{ - int res = -EINVAL; - - TRACE_ENTRY(); - - if (!strncmp("resync_size", p, 11)) { - res = 11; - p += res; - *action = VDISK_ACTION_RESYNC_SIZE; - } - - TRACE_EXIT_RES(res); - return res; -} - -/* scst_vdisk_mutex supposed to be held */ -static int vdisk_perform_cmd(struct vdev_type *vdt, int action, char *p, - char *name) -{ - int res = -EINVAL; - - TRACE_ENTRY(); - - if (action == VDISK_ACTION_RESYNC_SIZE) - res = vdisk_resync_size(vdt, p, name); - - TRACE_EXIT_RES(res); - return res; -} - static void vcdrom_init(struct vdev_type *vdt, struct scst_vdisk_dev *virt_dev) { @@ -3460,29 +3415,48 @@ static int vcdrom_pre_register(struct scst_vdisk_dev *virt_dev) return res; } -/* scst_vdisk_mutex supposed to be held */ -static int vcdrom_change(struct vdev_type *vdt, char *p, const char *name) +static int vcdrom_change(struct scst_vdisk_dev *virt_dev, + const char *buffer, int length) { loff_t err; - struct scst_vdisk_dev *virt_dev; - char *file_name, *fn = NULL, *old_fn; - int len; + char *old_fn, *i_buf, *p, *pp; + const char *file_name = NULL; int res = 0; - virt_dev = vdt->vfns.vdev_find(name); - if (virt_dev == NULL) { - PRINT_ERROR("Virtual device with name " - "%s not found", name); - res = -EINVAL; + TRACE_ENTRY(); + + /* + * There's no need in any lock here, because SCST core serializes + * all device sysfs calls. + */ + + i_buf = kmalloc(length+1, GFP_KERNEL); + if (i_buf == NULL) { + PRINT_ERROR("Unable to alloc intermediate buffer with size %d", + length+1); + res = -ENOMEM; goto out; } + memcpy(i_buf, buffer, length); + i_buf[length] = '\0'; + p = i_buf; + while (isspace(*p) && *p != '\0') p++; file_name = p; - while (!isspace(*p) && *p != '\0') - p++; - *p++ = '\0'; + p = &i_buf[length-1]; + pp = p; + while (isspace(*p) && *p != '\0') { + pp = p; + p--; + } + *pp = '\0'; + + res = scst_suspend_activity(true); + if (res != 0) + goto out; + if (*file_name == '\0') { virt_dev->cdrom_empty = 1; TRACE_DBG("%s", "No media"); @@ -3490,20 +3464,20 @@ static int vcdrom_change(struct vdev_type *vdt, char *p, const char *name) PRINT_ERROR("File path \"%s\" is not " "absolute", file_name); res = -EINVAL; - goto out; + goto out_resume; } else virt_dev->cdrom_empty = 0; old_fn = virt_dev->file_name; if (!virt_dev->cdrom_empty) { - len = strlen(file_name) + 1; - fn = kmalloc(len, GFP_KERNEL); + int len = strlen(file_name) + 1; + char *fn = kmalloc(len, GFP_KERNEL); if (fn == NULL) { TRACE(TRACE_OUT_OF_MEM, "%s", "Allocation of file_name failed"); res = -ENOMEM; - goto out; + goto out_resume; } strncpy(fn, file_name, len); @@ -3518,15 +3492,11 @@ static int vcdrom_change(struct vdev_type *vdt, char *p, const char *name) virt_dev->file_name = NULL; } - res = scst_suspend_activity(true); - if (res != 0) - goto out_free; - if (virt_dev->prevent_allow_medium_removal) { PRINT_ERROR("Prevent medium removal for " - "virtual device with name %s", name); + "virtual device with name %s", virt_dev->name); res = -EINVAL; - goto out_free_resume; + goto out_free; } virt_dev->file_size = err; @@ -3556,52 +3526,40 @@ out_resume: scst_resume_activity(); out: + TRACE_EXIT_RES(res); return res; out_free: + kfree(virt_dev->file_name); virt_dev->file_name = old_fn; - kfree(fn); - goto out; - -out_free_resume: - virt_dev->file_name = old_fn; - kfree(fn); goto out_resume; } -static int vcdrom_parse_cmd(struct vdev_type *vdt, char *p, int *action) -{ - int res = -EINVAL; - - TRACE_ENTRY(); - - if (!strncmp("change", p, 6)) { - res = 6; - p += res; - *action = VCDROM_ACTION_CHANGE; - } - - TRACE_EXIT_RES(res); - return res; -} - -/* scst_vdisk_mutex supposed to be held */ -static int vcdrom_perform_cmd(struct vdev_type *vdt, int action, char *p, - char *name) -{ - int res = -EINVAL; - - TRACE_ENTRY(); - - if (action == VCDROM_ACTION_CHANGE) - res = vcdrom_change(vdt, p, name); - - TRACE_EXIT_RES(res); - return res; -} - #ifndef CONFIG_SCST_PROC +static ssize_t vcdrom_sysfs_filename_store(struct kobject *kobj, + struct kobj_attribute *attr, const char *buf, size_t count) +{ + int res; + struct scst_device *dev; + struct scst_vdisk_dev *virt_dev; + + TRACE_ENTRY(); + + dev = container_of(kobj, struct scst_device, dev_kobj); + virt_dev = (struct scst_vdisk_dev *)dev->dh_priv; + + res = vcdrom_change(virt_dev, buf, count); + if (res != 0) + goto out; + + res = count; + +out: + TRACE_EXIT_RES(res); + return res; +} + static int vdisk_mgmt_cmd(const char *buffer, int length, struct vdev_type *vdt) { @@ -3618,6 +3576,7 @@ static int vdisk_mgmt_cmd(const char *buffer, int length, if (i_buf == NULL) { PRINT_ERROR("Unable to alloc intermediate buffer with size %d", length+1); + res = -ENOMEM; goto out; } @@ -3639,15 +3598,9 @@ static int vdisk_mgmt_cmd(const char *buffer, int length, p += 5; action = VDEV_ACTION_CLOSE; } else { - int n; - n = vdt->vfns.parse_cmd(vdt, p, &action); - if (n > 0) - p += n; - else { - PRINT_ERROR("Unknown action \"%s\"", p); - res = -EINVAL; - goto out_up; - } + PRINT_ERROR("Unknown action \"%s\"", p); + res = -EINVAL; + goto out_up; } if (!isspace(*p)) { @@ -3681,11 +3634,9 @@ static int vdisk_mgmt_cmd(const char *buffer, int length, res = vdt->vfns.vdev_close(vdt, name); if (res != 0) goto out_up; - } else { - res = vdt->vfns.perform_cmd(vdt, action, p, name); - if (res != 0) - goto out_up; - } + } else + sBUG(); + res = length; out_up: @@ -3880,20 +3831,12 @@ static ssize_t vdisk_sysfs_resync_size_store(struct kobject *kobj, dev = container_of(kobj, struct scst_device, dev_kobj); virt_dev = (struct scst_vdisk_dev *)dev->dh_priv; - if (mutex_lock_interruptible(&scst_vdisk_mutex) != 0) { - res = -EINTR; - goto out; - } - - res = __vdisk_resync_size(virt_dev); + res = vdisk_resync_size(virt_dev); if (res != 0) - goto out_unlock; + goto out; res = count; -out_unlock: - mutex_unlock(&scst_vdisk_mutex); - out: TRACE_EXIT_RES(res); return res; @@ -3993,6 +3936,7 @@ static int vdisk_proc_mgmt_cmd(const char *buffer, int length, if (i_buf == NULL) { PRINT_ERROR("Unable to alloc intermediate buffer with size %d", length+1); + res = -ENOMEM; goto out; } @@ -4226,7 +4170,7 @@ static int vdisk_proc_mgmt_cmd(const char *buffer, int length, goto out_up; } - res = __vdisk_resync_size(virt_dev); + res = vdisk_resync_size(virt_dev); if (res != 0) goto out_up; } @@ -4296,6 +4240,26 @@ out: return res; } +/* scst_vdisk_mutex supposed to be held */ +static int vcdrom_proc_change(struct vdev_type *vdt, char *p, const char *name) +{ + struct scst_vdisk_dev *virt_dev; + int res; + + virt_dev = vdt->vfns.vdev_find(name); + if (virt_dev == NULL) { + PRINT_ERROR("Virtual device with name " + "%s not found", name); + res = -EINVAL; + goto out; + } + + res = vcdrom_change(virt_dev, p, strlen(p) + 1); + +out: + return res; +} + static int vcdrom_proc_mgmt_cmd(const char *buffer, int length, struct scst_dev_type *dev_type) { @@ -4312,6 +4276,7 @@ static int vcdrom_proc_mgmt_cmd(const char *buffer, int length, if (i_buf == NULL) { PRINT_ERROR("Unable to alloc intermediate buffer with size %d", length+1); + res = -ENOMEM; goto out; } @@ -4365,7 +4330,7 @@ static int vcdrom_proc_mgmt_cmd(const char *buffer, int length, goto out_up; } else if (action == 1) { /* change */ - res = vcdrom_change(&vcdrom_type.parent_vdt, p, name); + res = vcdrom_proc_change(&vcdrom_type.parent_vdt, p, name); if (res != 0) goto out_up; } else { @@ -4558,7 +4523,7 @@ static void __init init_fileio_type(void) vdt->vdt_name = "FILEIO"; vdt->help_string = "Usage:\n" - " echo \"open|close|resync_size NAME [FILE_NAME " + " echo \"open|close NAME [FILE_NAME " "[BLOCK_SIZE] [WRITE_THROUGH] [READ_ONLY] [O_DIRECT] " "[NV_CACHE] [REMOVABLE]]\" >mgmt\n"; @@ -4566,8 +4531,6 @@ static void __init init_fileio_type(void) vdt->vfns.vdev_add = vdisk_add; vdt->vfns.vdev_find = vdisk_find; - vdt->vfns.parse_cmd = vdisk_parse_cmd; - vdt->vfns.perform_cmd = vdisk_perform_cmd; vdt->vfns.parse_option = vdisk_fileio_parse_option; vdt->vfns.pre_register = vdisk_fileio_pre_register; @@ -4582,7 +4545,7 @@ static void __init init_blockio_type(void) vdt->vdt_name = "BLOCKIO"; vdt->help_string = "Usage:\n" - " echo \"open|close|resync_size NAME [DEVICE_NAME " + " echo \"open|close NAME [DEVICE_NAME " "[BLOCK_SIZE] [READ_ONLY] [REMOVABLE]]\" >mgmt\n"; blockio_type.parent_vdt_vfns = vdt->vfns; @@ -4590,8 +4553,6 @@ static void __init init_blockio_type(void) vdt->vfns.vdev_init = vdisk_blockio_init; vdt->vfns.vdev_add = vdisk_add; vdt->vfns.vdev_find = vdisk_find; - vdt->vfns.parse_cmd = vdisk_parse_cmd; - vdt->vfns.perform_cmd = vdisk_perform_cmd; vdt->vfns.parse_option = vdisk_parse_option; vdt->vfns.pre_register = vdisk_blockio_pre_register; @@ -4613,9 +4574,7 @@ static void __init init_nullio_type(void) vdt->vfns.vdev_init = vdisk_nullio_init; vdt->vfns.vdev_add = vdisk_add; vdt->vfns.vdev_find = vdisk_find; - vdt->vfns.parse_cmd = vdisk_parse_cmd; vdt->vfns.parse_option = vdisk_parse_option; - vdt->vfns.perform_cmd = vdisk_perform_cmd; return; } @@ -4634,8 +4593,6 @@ static void __init init_vcdrom_type(void) vdt->vfns.vdev_init = vcdrom_init; vdt->vfns.vdev_add = vcdrom_add; vdt->vfns.vdev_find = vcdrom_find; - vdt->vfns.parse_cmd = vcdrom_parse_cmd; - vdt->vfns.perform_cmd = vcdrom_perform_cmd; vdt->vfns.pre_register = vcdrom_pre_register; return; diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 419ef8f5d..a49babde5 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -1334,7 +1334,6 @@ out: return res; } -/* Called under scst_mutex and suspended activity */ void scst_free_device(struct scst_device *dev) { TRACE_ENTRY(); @@ -2377,7 +2376,7 @@ void scst_free_session(struct scst_session *sess) mutex_unlock(&scst_mutex); - scst_sess_sysfs_put(sess); + scst_sess_sysfs_put(sess); /* must not be called under scst_mutex */ TRACE_EXIT(); return; diff --git a/scst/src/scst_main.c b/scst/src/scst_main.c index 36626d891..02b857617 100644 --- a/scst/src/scst_main.c +++ b/scst/src/scst_main.c @@ -468,7 +468,7 @@ out_resume_free: scst_resume_activity(); out_free_tgt_err: - scst_tgt_sysfs_put(tgt); + scst_tgt_sysfs_put(tgt); /* must not be called under scst_mutex */ tgt = NULL; out_err: @@ -544,7 +544,7 @@ again: del_timer_sync(&tgt->retry_timer); - scst_tgt_sysfs_put(tgt); + scst_tgt_sysfs_put(tgt); /* must not be called under scst_mutex */ PRINT_INFO("Target %p for template %s unregistered successfully", tgt, vtt->name); @@ -805,8 +805,10 @@ out_free: #endif out_free_dev: - scst_device_sysfs_put(dev); - goto out_up; + mutex_unlock(&scst_mutex); + scst_resume_activity(); + scst_device_sysfs_put(dev); /* must not be called under scst_mutex */ + goto out_err; } static void scst_unregister_device(struct scsi_device *scsidp) @@ -828,7 +830,7 @@ static void scst_unregister_device(struct scsi_device *scsidp) } if (dev == NULL) { PRINT_ERROR("%s", "Target device not found"); - goto out_unblock; + goto out_resume; } list_del(&dev->dev_list_entry); @@ -844,18 +846,23 @@ static void scst_unregister_device(struct scsi_device *scsidp) put_disk(dev->rq_disk); #endif - scst_device_sysfs_put(dev); + mutex_unlock(&scst_mutex); + scst_resume_activity(); + + scst_device_sysfs_put(dev); /* must not be called under scst_mutex */ PRINT_INFO("Detached from scsi%d, channel %d, id %d, lun %d, type %d", scsidp->host->host_no, scsidp->channel, scsidp->id, scsidp->lun, scsidp->type); -out_unblock: - mutex_unlock(&scst_mutex); - scst_resume_activity(); - +out: TRACE_EXIT(); return; + +out_resume: + mutex_unlock(&scst_mutex); + scst_resume_activity(); + goto out; } static int scst_dev_handler_check(struct scst_dev_type *dev_handler) @@ -970,8 +977,10 @@ out_free_del: list_del(&dev->dev_list_entry); out_release: - scst_device_sysfs_put(dev); - goto out_up; + mutex_unlock(&scst_mutex); + scst_resume_activity(); + scst_device_sysfs_put(dev); /* must not be called under scst_mutex */ + goto out; } EXPORT_SYMBOL(scst_register_virtual_device); @@ -1009,12 +1018,12 @@ void scst_unregister_virtual_device(int id) PRINT_INFO("Detached from virtual device %s (id %d)", dev->virt_name, dev->virt_id); - scst_device_sysfs_put(dev); - out_unblock: mutex_unlock(&scst_mutex); scst_resume_activity(); + scst_device_sysfs_put(dev); /* must not be called under scst_mutex */ + TRACE_EXIT(); return; } diff --git a/scst/src/scst_sysfs.c b/scst/src/scst_sysfs.c index c4d7a5905..85c17408e 100644 --- a/scst/src/scst_sysfs.c +++ b/scst/src/scst_sysfs.c @@ -484,6 +484,10 @@ out_nomem: goto out; } +/* + * Must not be called under scst_mutex or there can be a deadlock with + * tgt_attr_rwsem + */ void scst_tgt_sysfs_put(struct scst_tgt *tgt) { if (tgt->tgt_kobj_initialized) { @@ -635,10 +639,17 @@ static ssize_t scst_dev_attr_show(struct kobject *kobj, struct attribute *attr, goto out; } + if (mutex_lock_interruptible(&dev->dev_sysfs_mutex) != 0) { + res = -EINTR; + goto out; + } + kobj_attr = container_of(attr, struct kobj_attribute, attr); res = kobj_attr->show(kobj, kobj_attr, buf); + mutex_unlock(&dev->dev_sysfs_mutex); + up_read(&dev->dev_attr_rwsem); out: @@ -659,10 +670,17 @@ static ssize_t scst_dev_attr_store(struct kobject *kobj, struct attribute *attr, goto out; } + if (mutex_lock_interruptible(&dev->dev_sysfs_mutex) != 0) { + res = -EINTR; + goto out; + } + kobj_attr = container_of(attr, struct kobj_attribute, attr); res = kobj_attr->store(kobj, kobj_attr, buf, count); + mutex_unlock(&dev->dev_sysfs_mutex); + up_read(&dev->dev_attr_rwsem); out: @@ -687,6 +705,7 @@ int scst_create_device_sysfs(struct scst_device *dev) TRACE_ENTRY(); init_rwsem(&dev->dev_attr_rwsem); + mutex_init(&dev->dev_sysfs_mutex); dev->dev_kobj_initialized = 1; @@ -726,6 +745,10 @@ out: return retval; } +/* + * Must not be called under scst_mutex or there can be a deadlock with + * dev_attr_rwsem + */ void scst_device_sysfs_put(struct scst_device *dev) { TRACE_ENTRY(); @@ -937,6 +960,10 @@ out_free: return retval; } +/* + * Must not be called under scst_mutex or there can be a deadlock with + * sess_attr_rwsem + */ void scst_sess_sysfs_put(struct scst_session *sess) { TRACE_ENTRY();