From d2ac838e4cd7e5e9891ecc094d626734b0245c99 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Mon, 7 May 2018 11:37:58 -0400 Subject: loop: add recursion validation to LOOP_CHANGE_FD Refactor the validation code used in LOOP_SET_FD so it is also used in LOOP_CHANGE_FD. Otherwise it is possible to construct a set of loop devices that all refer to each other. This can lead to a infinite loop in starting with "while (is_loop_device(f)) .." in loop_set_fd(). Fix this by refactoring out the validation code and using it for LOOP_CHANGE_FD as well as LOOP_SET_FD. Reported-by: syzbot+4349872271ece473a7c91190b68b4bac7c5dbc87@syzkaller.appspotmail.com Reported-by: syzbot+40bd32c4d9a3cc12a339@syzkaller.appspotmail.com Reported-by: syzbot+769c54e66f994b041be7@syzkaller.appspotmail.com Reported-by: syzbot+0a89a9ce473936c57065@syzkaller.appspotmail.com Signed-off-by: Theodore Ts'o Signed-off-by: Jens Axboe --- drivers/block/loop.c | 68 +++++++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 30 deletions(-) (limited to 'drivers') diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 4838b0dbaad3..f8f3ca6e77fd 100644 --- a/drivers/block/loop.c +++ b/drivers/block/loop.c @@ -644,6 +644,36 @@ static void loop_reread_partitions(struct loop_device *lo, __func__, lo->lo_number, lo->lo_file_name, rc); } +static inline int is_loop_device(struct file *file) +{ + struct inode *i = file->f_mapping->host; + + return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR; +} + +static int loop_validate_file(struct file *file, struct block_device *bdev) +{ + struct inode *inode = file->f_mapping->host; + struct file *f = file; + + /* Avoid recursion */ + while (is_loop_device(f)) { + struct loop_device *l; + + if (f->f_mapping->host->i_bdev == bdev) + return -EBADF; + + l = f->f_mapping->host->i_bdev->bd_disk->private_data; + if (l->lo_state == Lo_unbound) { + return -EINVAL; + } + f = l->lo_backing_file; + } + if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode)) + return -EINVAL; + return 0; +} + /* * loop_change_fd switched the backing store of a loopback device to * a new file. This is useful for operating system installers to free up @@ -673,14 +703,15 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, if (!file) goto out; + error = loop_validate_file(file, bdev); + if (error) + goto out_putf; + inode = file->f_mapping->host; old_file = lo->lo_backing_file; error = -EINVAL; - if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode)) - goto out_putf; - /* size of the new backing store needs to be the same */ if (get_loop_size(lo, file) != get_loop_size(lo, old_file)) goto out_putf; @@ -706,13 +737,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev, return error; } -static inline int is_loop_device(struct file *file) -{ - struct inode *i = file->f_mapping->host; - - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR; -} - /* loop sysfs attributes */ static ssize_t loop_attr_show(struct device *dev, char *page, @@ -878,7 +902,7 @@ static int loop_prepare_queue(struct loop_device *lo) static int loop_set_fd(struct loop_device *lo, fmode_t mode, struct block_device *bdev, unsigned int arg) { - struct file *file, *f; + struct file *file; struct inode *inode; struct address_space *mapping; int lo_flags = 0; @@ -897,29 +921,13 @@ static int loop_set_fd(struct loop_device *lo, fmode_t mode, if (lo->lo_state != Lo_unbound) goto out_putf; - /* Avoid recursion */ - f = file; - while (is_loop_device(f)) { - struct loop_device *l; - - if (f->f_mapping->host->i_bdev == bdev) - goto out_putf; - - l = f->f_mapping->host->i_bdev->bd_disk->private_data; - if (l->lo_state == Lo_unbound) { - error = -EINVAL; - goto out_putf; - } - f = l->lo_backing_file; - } + error = loop_validate_file(file, bdev); + if (error) + goto out_putf; mapping = file->f_mapping; inode = mapping->host; - error = -EINVAL; - if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode)) - goto out_putf; - if (!(file->f_mode & FMODE_WRITE) || !(mode & FMODE_WRITE) || !file->f_op->write_iter) lo_flags |= LO_FLAGS_READ_ONLY; -- cgit v1.2.3 From 21ff1399ad95811629e13fa70e2386f3bba85fe5 Mon Sep 17 00:00:00 2001 From: Wei Yongjun Date: Tue, 5 Jun 2018 09:15:12 +0000 Subject: lightnvm: pblk: make symbol write_buffer_size static Fixes the following sparse warning: drivers/lightnvm/pblk-init.c:23:14: warning: symbol 'write_buffer_size' was not declared. Should it be static? Signed-off-by: Wei Yongjun Signed-off-by: Jens Axboe --- drivers/lightnvm/pblk-init.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c index ce561f5d48ce..491df0fa0835 100644 --- a/drivers/lightnvm/pblk-init.c +++ b/drivers/lightnvm/pblk-init.c @@ -20,7 +20,7 @@ #include "pblk.h" -unsigned int write_buffer_size; +static unsigned int write_buffer_size; module_param(write_buffer_size, uint, 0644); MODULE_PARM_DESC(write_buffer_size, "number of entries in a write buffer"); -- cgit v1.2.3 From 0ec6937e8e1251db1f2f54259ba5c16dbdb2b943 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Tue, 5 Jun 2018 16:14:56 +0100 Subject: lightnvm: pblk: fix resource leak of invalid_bitmap Currently the error exit path when the emeta could not be interpreted is via fail_free_ws and this fails to free invalid_bitmap. Fix this by adding another exit label and exiting via this to kfree invalid_bitmap. Detected by CoverityScan, CID#1469659 ("Resource leak") Fixes: 48b8d20895f8 ("lightnvm: pblk: garbage collect lines with failed writes") Signed-off-by: Colin Ian King Signed-off-by: Jens Axboe --- drivers/lightnvm/pblk-gc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c index df88f1bdd921..6a4883e40cc0 100644 --- a/drivers/lightnvm/pblk-gc.c +++ b/drivers/lightnvm/pblk-gc.c @@ -203,7 +203,7 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) if (!lba_list) { pr_err("pblk: could not interpret emeta (line %d)\n", line->id); - goto fail_free_ws; + goto fail_free_invalid_bitmap; } } @@ -280,6 +280,7 @@ fail_free_gc_rq: kfree(gc_rq); fail_free_lba_list: pblk_mfree(lba_list, l_mg->emeta_alloc_type); +fail_free_invalid_bitmap: kfree(invalid_bitmap); fail_free_ws: kfree(line_ws); -- cgit v1.2.3 From ee57a05cf86f9789cbac5e9de8e720458338533b Mon Sep 17 00:00:00 2001 From: Kevin Vigor Date: Mon, 4 Jun 2018 10:40:12 -0600 Subject: nbd: Consistently use request pointer in debug messages. Existing dev_dbg messages sometimes identify request using request pointer, sometimes using nbd_cmd pointer. This makes it hard to follow request flow. Consistently use request pointer instead. Reviewed-by: Josef Bacik Signed-off-by: Kevin Vigor Signed-off-by: Jens Axboe --- drivers/block/nbd.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers') diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index 3ed1ef8ee528..b5afa11ca465 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -275,7 +275,7 @@ static void nbd_complete_rq(struct request *req) { struct nbd_cmd *cmd = blk_mq_rq_to_pdu(req); - dev_dbg(nbd_to_dev(cmd->nbd), "request %p: %s\n", cmd, + dev_dbg(nbd_to_dev(cmd->nbd), "request %p: %s\n", req, cmd->status ? "failed" : "done"); blk_mq_end_request(req, cmd->status); @@ -482,7 +482,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index) memcpy(request.handle, &tag, sizeof(tag)); dev_dbg(nbd_to_dev(nbd), "request %p: sending control (%s@%llu,%uB)\n", - cmd, nbdcmd_to_ascii(type), + req, nbdcmd_to_ascii(type), (unsigned long long)blk_rq_pos(req) << 9, blk_rq_bytes(req)); result = sock_xmit(nbd, index, 1, &from, (type == NBD_CMD_WRITE) ? MSG_MORE : 0, &sent); @@ -518,7 +518,7 @@ send_pages: int flags = is_last ? 0 : MSG_MORE; dev_dbg(nbd_to_dev(nbd), "request %p: sending %d bytes data\n", - cmd, bvec.bv_len); + req, bvec.bv_len); iov_iter_bvec(&from, ITER_BVEC | WRITE, &bvec, 1, bvec.bv_len); if (skip) { @@ -610,7 +610,7 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) return cmd; } - dev_dbg(nbd_to_dev(nbd), "request %p: got reply\n", cmd); + dev_dbg(nbd_to_dev(nbd), "request %p: got reply\n", req); if (rq_data_dir(req) != WRITE) { struct req_iterator iter; struct bio_vec bvec; @@ -637,7 +637,7 @@ static struct nbd_cmd *nbd_read_stat(struct nbd_device *nbd, int index) return ERR_PTR(-EIO); } dev_dbg(nbd_to_dev(nbd), "request %p: got %d bytes data\n", - cmd, bvec.bv_len); + req, bvec.bv_len); } } else { /* See the comment in nbd_queue_rq. */ -- cgit v1.2.3 From 07ce213f63fbc389c8fcaefab6267b3bc0a12796 Mon Sep 17 00:00:00 2001 From: Josef Bacik Date: Tue, 5 Jun 2018 11:41:23 -0400 Subject: nbd: set discard_alignment to the granularity Technically we should be able to get away with 0 as the discard_alignment, but there's no way currently for the protocol to indicate different alignments, and in real life most disks have discard_alignment == discard_granularity. Just set our alignment to our blocksize to make sure discards will actually work properly with 4k drives. Signed-off-by: Josef Bacik Signed-off-by: Jens Axboe --- drivers/block/nbd.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers') diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c index b5afa11ca465..3b7083b8ecbb 100644 --- a/drivers/block/nbd.c +++ b/drivers/block/nbd.c @@ -246,6 +246,7 @@ static void nbd_size_update(struct nbd_device *nbd) if (config->flags & NBD_FLAG_SEND_TRIM) { nbd->disk->queue->limits.discard_granularity = config->blksize; + nbd->disk->queue->limits.discard_alignment = config->blksize; blk_queue_max_discard_sectors(nbd->disk->queue, UINT_MAX); } blk_queue_logical_block_size(nbd->disk->queue, config->blksize); @@ -1062,6 +1063,7 @@ static void nbd_config_put(struct nbd_device *nbd) nbd->tag_set.timeout = 0; nbd->disk->queue->limits.discard_granularity = 0; + nbd->disk->queue->limits.discard_alignment = 0; blk_queue_max_discard_sectors(nbd->disk->queue, UINT_MAX); blk_queue_flag_clear(QUEUE_FLAG_DISCARD, nbd->disk->queue); @@ -1516,6 +1518,7 @@ static int nbd_dev_add(int index) blk_queue_flag_set(QUEUE_FLAG_NONROT, disk->queue); blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, disk->queue); disk->queue->limits.discard_granularity = 0; + disk->queue->limits.discard_alignment = 0; blk_queue_max_discard_sectors(disk->queue, 0); blk_queue_max_segment_size(disk->queue, UINT_MAX); blk_queue_max_segments(disk->queue, USHRT_MAX); -- cgit v1.2.3 From 2a2a4c510b761e800098992cac61281c86527e5d Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Thu, 7 Jun 2018 14:42:06 -0600 Subject: dm: use bioset_init_from_src() to copy bio_set We can't just copy and clear a bio_set, use the bio helper to setup a new bio_set with the settings from another one. Fixes: 6f1c819c219f ("dm: convert to bioset_init()/mempool_init()") Reported-by: Venkat R.B Tested-by: Venkat R.B Tested-by: Li Wang Reviewed-by: Mike Snitzer Signed-off-by: Jens Axboe --- drivers/md/dm.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 98dff36b89a3..20a8d63754bf 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1953,9 +1953,10 @@ static void free_dev(struct mapped_device *md) kvfree(md); } -static void __bind_mempools(struct mapped_device *md, struct dm_table *t) +static int __bind_mempools(struct mapped_device *md, struct dm_table *t) { struct dm_md_mempools *p = dm_table_get_md_mempools(t); + int ret = 0; if (dm_table_bio_based(t)) { /* @@ -1982,13 +1983,16 @@ static void __bind_mempools(struct mapped_device *md, struct dm_table *t) bioset_initialized(&md->bs) || bioset_initialized(&md->io_bs)); - md->bs = p->bs; - memset(&p->bs, 0, sizeof(p->bs)); - md->io_bs = p->io_bs; - memset(&p->io_bs, 0, sizeof(p->io_bs)); + ret = bioset_init_from_src(&md->bs, &p->bs); + if (ret) + goto out; + ret = bioset_init_from_src(&md->io_bs, &p->io_bs); + if (ret) + bioset_exit(&md->bs); out: /* mempool bind completed, no longer need any mempools in the table */ dm_table_free_md_mempools(t); + return ret; } /* @@ -2033,6 +2037,7 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t, struct request_queue *q = md->queue; bool request_based = dm_table_request_based(t); sector_t size; + int ret; lockdep_assert_held(&md->suspend_lock); @@ -2068,7 +2073,11 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t, md->immutable_target = dm_table_get_immutable_target(t); } - __bind_mempools(md, t); + ret = __bind_mempools(md, t); + if (ret) { + old_map = ERR_PTR(ret); + goto out; + } old_map = rcu_dereference_protected(md->map, lockdep_is_held(&md->suspend_lock)); rcu_assign_pointer(md->map, (void *)t); @@ -2078,6 +2087,7 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t, if (old_map) dm_sync_table(md); +out: return old_map; } -- cgit v1.2.3 From 28dec870aaf704af1421ac014f7f8abf4cac7c69 Mon Sep 17 00:00:00 2001 From: Kent Overstreet Date: Thu, 7 Jun 2018 20:52:54 -0400 Subject: md: Unify mddev destruction paths Previously, mddev_put() had a couple different paths for freeing a mddev, due to the fact that the kobject wasn't initialized when the mddev was first allocated. If we move the kobject_init() to when it's first allocated and just use kobject_add() later, we can clean all this up. This also removes a hack in mddev_put() to avoid freeing biosets under a spinlock, which involved copying biosets on the stack after the reset bioset_init() changes. Signed-off-by: Kent Overstreet Signed-off-by: Jens Axboe --- drivers/md/md.c | 53 ++++++++++++++++++----------------------------------- 1 file changed, 18 insertions(+), 35 deletions(-) (limited to 'drivers') diff --git a/drivers/md/md.c b/drivers/md/md.c index fc692b7128bb..22203eba1e6e 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -84,6 +84,8 @@ static void autostart_arrays(int part); static LIST_HEAD(pers_list); static DEFINE_SPINLOCK(pers_lock); +static struct kobj_type md_ktype; + struct md_cluster_operations *md_cluster_ops; EXPORT_SYMBOL(md_cluster_ops); struct module *md_cluster_mod; @@ -510,11 +512,6 @@ static void mddev_delayed_delete(struct work_struct *ws); static void mddev_put(struct mddev *mddev) { - struct bio_set bs, sync_bs; - - memset(&bs, 0, sizeof(bs)); - memset(&sync_bs, 0, sizeof(sync_bs)); - if (!atomic_dec_and_lock(&mddev->active, &all_mddevs_lock)) return; if (!mddev->raid_disks && list_empty(&mddev->disks) && @@ -522,30 +519,23 @@ static void mddev_put(struct mddev *mddev) /* Array is not configured at all, and not held active, * so destroy it */ list_del_init(&mddev->all_mddevs); - bs = mddev->bio_set; - sync_bs = mddev->sync_set; - memset(&mddev->bio_set, 0, sizeof(mddev->bio_set)); - memset(&mddev->sync_set, 0, sizeof(mddev->sync_set)); - if (mddev->gendisk) { - /* We did a probe so need to clean up. Call - * queue_work inside the spinlock so that - * flush_workqueue() after mddev_find will - * succeed in waiting for the work to be done. - */ - INIT_WORK(&mddev->del_work, mddev_delayed_delete); - queue_work(md_misc_wq, &mddev->del_work); - } else - kfree(mddev); + + /* + * Call queue_work inside the spinlock so that + * flush_workqueue() after mddev_find will succeed in waiting + * for the work to be done. + */ + INIT_WORK(&mddev->del_work, mddev_delayed_delete); + queue_work(md_misc_wq, &mddev->del_work); } spin_unlock(&all_mddevs_lock); - bioset_exit(&bs); - bioset_exit(&sync_bs); } static void md_safemode_timeout(struct timer_list *t); void mddev_init(struct mddev *mddev) { + kobject_init(&mddev->kobj, &md_ktype); mutex_init(&mddev->open_mutex); mutex_init(&mddev->reconfig_mutex); mutex_init(&mddev->bitmap_info.mutex); @@ -5215,6 +5205,8 @@ static void md_free(struct kobject *ko) put_disk(mddev->gendisk); percpu_ref_exit(&mddev->writes_pending); + bioset_exit(&mddev->bio_set); + bioset_exit(&mddev->sync_set); kfree(mddev); } @@ -5348,8 +5340,7 @@ static int md_alloc(dev_t dev, char *name) mutex_lock(&mddev->open_mutex); add_disk(disk); - error = kobject_init_and_add(&mddev->kobj, &md_ktype, - &disk_to_dev(disk)->kobj, "%s", "md"); + error = kobject_add(&mddev->kobj, &disk_to_dev(disk)->kobj, "%s", "md"); if (error) { /* This isn't possible, but as kobject_init_and_add is marked * __must_check, we must do something with the result @@ -5506,7 +5497,7 @@ int md_run(struct mddev *mddev) if (!bioset_initialized(&mddev->sync_set)) { err = bioset_init(&mddev->sync_set, BIO_POOL_SIZE, 0, BIOSET_NEED_BVECS); if (err) - goto abort; + return err; } spin_lock(&pers_lock); @@ -5519,8 +5510,7 @@ int md_run(struct mddev *mddev) else pr_warn("md: personality for level %s is not loaded!\n", mddev->clevel); - err = -EINVAL; - goto abort; + return -EINVAL; } spin_unlock(&pers_lock); if (mddev->level != pers->level) { @@ -5533,8 +5523,7 @@ int md_run(struct mddev *mddev) pers->start_reshape == NULL) { /* This personality cannot handle reshaping... */ module_put(pers->owner); - err = -EINVAL; - goto abort; + return -EINVAL; } if (pers->sync_request) { @@ -5603,7 +5592,7 @@ int md_run(struct mddev *mddev) mddev->private = NULL; module_put(pers->owner); bitmap_destroy(mddev); - goto abort; + return err; } if (mddev->queue) { bool nonrot = true; @@ -5665,12 +5654,6 @@ int md_run(struct mddev *mddev) sysfs_notify_dirent_safe(mddev->sysfs_action); sysfs_notify(&mddev->kobj, NULL, "degraded"); return 0; - -abort: - bioset_exit(&mddev->bio_set); - bioset_exit(&mddev->sync_set); - - return err; } EXPORT_SYMBOL_GPL(md_run); -- cgit v1.2.3 From f39ae4719b1c33d048aa4d3c284d82ecf252742b Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Thu, 31 May 2018 18:23:48 +0200 Subject: nvmet: return all zeroed buffer when we can't find an active namespace Quote from Figure 106 in NVMe 1.3a: The Identify Namespace data structure is returned to the host for the namespace specified in the Namespace Identifier (CDW1.NSID) field if it is an active NSID. If the specified namespace is not an active NSID, then the controller returns a zero filled data structure. Signed-off-by: Christoph Hellwig Reviewed-by: Sagi Grimberg Reviewed-by: Max Gurtovoy Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/target/admin-cmd.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c index ead8fbe6922e..962532842769 100644 --- a/drivers/nvme/target/admin-cmd.c +++ b/drivers/nvme/target/admin-cmd.c @@ -270,8 +270,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) struct nvme_id_ns *id; u16 status = 0; - ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid); - if (!ns) { + if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) { status = NVME_SC_INVALID_NS | NVME_SC_DNR; goto out; } @@ -279,9 +278,14 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) id = kzalloc(sizeof(*id), GFP_KERNEL); if (!id) { status = NVME_SC_INTERNAL; - goto out_put_ns; + goto out; } + /* return an all zeroed buffer if we can't find an active namespace */ + ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid); + if (!ns) + goto done; + /* * nuse = ncap = nsze isn't always true, but we have no way to find * that out from the underlying device. @@ -306,11 +310,10 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) id->lbaf[0].ds = ns->blksize_shift; + nvmet_put_namespace(ns); +done: status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id)); - kfree(id); -out_put_ns: - nvmet_put_namespace(ns); out: nvmet_req_complete(req, status); } -- cgit v1.2.3 From 12a0b662210702c6b0ce9f66f0c177ff1dea99cb Mon Sep 17 00:00:00 2001 From: Johannes Thumshirn Date: Fri, 1 Jun 2018 09:11:20 +0200 Subject: nvme: don't hold nvmf_transports_rwsem for more than transport lookups Only take nvmf_transports_rwsem when doing a lookup of registered transports, so that a blocking ->create_ctrl doesn't prevent other actions on /dev/nvme-fabrics. Signed-off-by: Johannes Thumshirn [hch: increased lock hold time a bit to be safe, added a comment and updated the changelog] Signed-off-by: Christoph Hellwig Reviewed-by: Sagi Grimberg Reviewed-by: Hannes Reinecke Signed-off-by: Jens Axboe --- drivers/nvme/host/fabrics.c | 3 ++- drivers/nvme/host/fabrics.h | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c index 5f5f7067c41d..fa32c1216409 100644 --- a/drivers/nvme/host/fabrics.c +++ b/drivers/nvme/host/fabrics.c @@ -952,6 +952,7 @@ nvmf_create_ctrl(struct device *dev, const char *buf, size_t count) ret = -EBUSY; goto out_unlock; } + up_read(&nvmf_transports_rwsem); ret = nvmf_check_required_opts(opts, ops->required_opts); if (ret) @@ -968,11 +969,11 @@ nvmf_create_ctrl(struct device *dev, const char *buf, size_t count) } module_put(ops->module); - up_read(&nvmf_transports_rwsem); return ctrl; out_module_put: module_put(ops->module); + goto out_free_opts; out_unlock: up_read(&nvmf_transports_rwsem); out_free_opts: diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h index 0cf0460a5c92..7491a0bbf711 100644 --- a/drivers/nvme/host/fabrics.h +++ b/drivers/nvme/host/fabrics.h @@ -124,6 +124,9 @@ struct nvmf_ctrl_options { * 1. At minimum, 'required_opts' and 'allowed_opts' should * be set to the same enum parsing options defined earlier. * 2. create_ctrl() must be defined (even if it does nothing) + * 3. struct nvmf_transport_ops must be statically allocated in the + * modules .bss section so that a pure module_get on @module + * prevents the memory from beeing freed. */ struct nvmf_transport_ops { struct list_head entry; -- cgit v1.2.3 From d4c68c7a8e2aa060d3c245052c76e74dd20b141b Mon Sep 17 00:00:00 2001 From: Steve Wise Date: Tue, 5 Jun 2018 10:16:41 -0700 Subject: nvme-rdma: correctly check for target keyed sgl support The code was checking bit 20 instead of bit 2. Also fixed the log entry. Reviewed-by: Sagi Grimberg Signed-off-by: Steve Wise Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/rdma.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index 7b3f08410430..2aba03876d84 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1951,8 +1951,9 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev, } /* sanity check keyed sgls */ - if (!(ctrl->ctrl.sgls & (1 << 20))) { - dev_err(ctrl->ctrl.device, "Mandatory keyed sgls are not support\n"); + if (!(ctrl->ctrl.sgls & (1 << 2))) { + dev_err(ctrl->ctrl.device, + "Mandatory keyed sgls are not supported!\n"); ret = -EINVAL; goto out_remove_admin_queue; } -- cgit v1.2.3 From 9ba2a5cb88969c847015905db7f1627ae3c82f73 Mon Sep 17 00:00:00 2001 From: Sagi Grimberg Date: Wed, 6 Jun 2018 15:27:48 +0300 Subject: nvmet: filter newlines from user input We should avoid consuming the newlines in traddr, trsvcid and device_path. Add minimal processing to make sure they are gone. Reviewed-by: Johannes Thumshirn Signed-off-by: Sagi Grimberg Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/target/configfs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c index ad9ff27234b5..d3f3b3ec4d1a 100644 --- a/drivers/nvme/target/configfs.c +++ b/drivers/nvme/target/configfs.c @@ -137,8 +137,10 @@ static ssize_t nvmet_addr_traddr_store(struct config_item *item, pr_err("Disable the address before modifying\n"); return -EACCES; } - return snprintf(port->disc_addr.traddr, - sizeof(port->disc_addr.traddr), "%s", page); + + if (sscanf(page, "%s\n", port->disc_addr.traddr) != 1) + return -EINVAL; + return count; } CONFIGFS_ATTR(nvmet_, addr_traddr); @@ -208,8 +210,10 @@ static ssize_t nvmet_addr_trsvcid_store(struct config_item *item, pr_err("Disable the address before modifying\n"); return -EACCES; } - return snprintf(port->disc_addr.trsvcid, - sizeof(port->disc_addr.trsvcid), "%s", page); + + if (sscanf(page, "%s\n", port->disc_addr.trsvcid) != 1) + return -EINVAL; + return count; } CONFIGFS_ATTR(nvmet_, addr_trsvcid); @@ -288,7 +292,7 @@ static ssize_t nvmet_ns_device_path_store(struct config_item *item, kfree(ns->device_path); ret = -ENOMEM; - ns->device_path = kstrdup(page, GFP_KERNEL); + ns->device_path = kstrndup(page, strcspn(page, "\n"), GFP_KERNEL); if (!ns->device_path) goto out_unlock; -- cgit v1.2.3 From 0bc88192033a6e652e3fb1adfd6d1b66be33951e Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:04 -0600 Subject: nvme-pci: remove unnecessary nested locking The nvme pci driver no longer handles completions under the cq lock, so the nested locking is not necessary. Signed-off-by: Keith Busch Reviewed-by: Jens Axboe Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index e526437bacbf..a7bed8dccd61 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2012,13 +2012,7 @@ static void nvme_del_cq_end(struct request *req, blk_status_t error) if (!error) { unsigned long flags; - /* - * We might be called with the AQ cq_lock held - * and the I/O queue cq_lock should always - * nest inside the AQ one. - */ - spin_lock_irqsave_nested(&nvmeq->cq_lock, flags, - SINGLE_DEPTH_NESTING); + spin_lock_irqsave(&nvmeq->cq_lock, flags); nvme_process_cq(nvmeq, &start, &end, -1); spin_unlock_irqrestore(&nvmeq->cq_lock, flags); -- cgit v1.2.3 From 397c699fb096e7a822990990c17c6b43e829cfc4 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:05 -0600 Subject: nvme-pci: remove unnecessary completion doorbell check The nvme pci driver never unmaps the doorbell registers while the requests are active, so we can always safely update the completion queue head. Signed-off-by: Keith Busch Reviewed-by: Jens Axboe Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index a7bed8dccd61..4963a407e728 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -920,11 +920,9 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq) { u16 head = nvmeq->cq_head; - if (likely(nvmeq->cq_vector >= 0)) { - if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db, - nvmeq->dbbuf_cq_ei)) - writel(head, nvmeq->q_db + nvmeq->dev->db_stride); - } + if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db, + nvmeq->dbbuf_cq_ei)) + writel(head, nvmeq->q_db + nvmeq->dev->db_stride); } static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx) -- cgit v1.2.3 From ded45505dbfdcaf1e49ae0349e5dafb59c9efbe5 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:06 -0600 Subject: nvme-pci: queue creation fixes We've been ignoring NVMe error status on queue creations. Fortunately they are uncommon, but we should handle these anyway. This patch adds checks for the a positive error return value that indicates an NVMe status. If we do see a negative return, the controller isn't usable, so this patch returns immediately in since we can't unwind that failure. Signed-off-by: Keith Busch Reviewed-by: Jens Axboe Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 4963a407e728..7a42ccad3864 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1475,11 +1475,13 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid) */ vector = dev->num_vecs == 1 ? 0 : qid; result = adapter_alloc_cq(dev, qid, nvmeq, vector); - if (result < 0) - goto out; + if (result) + return result; result = adapter_alloc_sq(dev, qid, nvmeq); if (result < 0) + return result; + else if (result) goto release_cq; /* @@ -1501,7 +1503,6 @@ release_sq: adapter_delete_sq(dev, qid); release_cq: adapter_delete_cq(dev, qid); -out: return result; } -- cgit v1.2.3 From fe76fcfb91a97eccf20cec17ff05a27b8d5b0801 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:07 -0600 Subject: nvme-pci: remove HMB teardown on reset The controller is required to disable its host memory buffer use on controller reset. We don't need to submit an admin command to delete it, so this patch skips sending that command so we don't need to worry about handling a timeout. Signed-off-by: Keith Busch Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 8 -------- 1 file changed, 8 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 7a42ccad3864..7f8b1bd03db4 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2224,14 +2224,6 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) nvme_stop_queues(&dev->ctrl); if (!dead && dev->ctrl.queue_count > 0) { - /* - * If the controller is still alive tell it to stop using the - * host memory buffer. In theory the shutdown / reset should - * make sure that it doesn't access the host memoery anymore, - * but I'd rather be safe than sorry.. - */ - if (dev->host_mem_descs) - nvme_set_host_mem(dev, 0); nvme_disable_io_queues(dev); nvme_disable_admin_queue(dev, shutdown); } -- cgit v1.2.3 From 1d39e6928cbd0eb737c51545210b5186d5551ba1 Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:08 -0600 Subject: nvme-pci: unquiesce dead controller queues This patch ensures the nvme namsepace request queues are not quiesced on a surprise removal. It's possible the queues were previously killed in a failed reset, so the queues need to be unquiesced to ensure all requests are flushed to completion. Signed-off-by: Keith Busch Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index 7f8b1bd03db4..d935aba0288f 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -2599,7 +2599,7 @@ static void nvme_remove(struct pci_dev *pdev) if (!pci_device_is_present(pdev)) { nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DEAD); - nvme_dev_disable(dev, false); + nvme_dev_disable(dev, true); } flush_work(&dev->ctrl.reset_work); -- cgit v1.2.3 From 69f4eb9ff79556c1a3daf5af5573594c196f30cc Mon Sep 17 00:00:00 2001 From: Keith Busch Date: Wed, 6 Jun 2018 08:13:09 -0600 Subject: nvme-pci: make CMB SQ mod-param read-only A controller reset after a run time change of the CMB module parameter breaks the driver. An 'on -> off' will have the driver use NULL for the host memory queue, and 'off -> on' will use mismatched queue depth between the device and the host. We could fix both, but there isn't really a good reason to change this at run time anyway, compared to at module load time, so this patch makes parameter read-only after after modprobe. Signed-off-by: Keith Busch Reviewed-by: Sagi Grimberg Reviewed-by: Johannes Thumshirn Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index d935aba0288f..cd7aec58a301 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -42,7 +42,7 @@ static int use_threaded_interrupts; module_param(use_threaded_interrupts, int, 0); static bool use_cmb_sqes = true; -module_param(use_cmb_sqes, bool, 0644); +module_param(use_cmb_sqes, bool, 0444); MODULE_PARM_DESC(use_cmb_sqes, "use controller's memory buffer for I/O SQes"); static unsigned int max_host_mem_size_mb = 128; -- cgit v1.2.3 From 77016199f11eacd7b23e2faeb4d0f36166e3530b Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Thu, 7 Jun 2018 11:27:41 +0300 Subject: nvme: cleanup double shift issue The problem here is that set_bit() and test_bit() take a bit number so we should be passing 0 but instead we're passing (1 << 0) which leads to a double shift. It doesn't cause a runtime bug in the current code because it's done consistently and we only set that one bit. I decided to just re-use NVME_AER_NOTICE_NS_CHANGED instead of introducing a new define for this. Signed-off-by: Dan Carpenter Reviewed-by: Sagi Grimberg Signed-off-by: Christoph Hellwig Signed-off-by: Jens Axboe --- drivers/nvme/host/core.c | 4 ++-- drivers/nvme/host/nvme.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 04a20da76786..e2dcd14012b1 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3245,7 +3245,7 @@ static void nvme_scan_work(struct work_struct *work) WARN_ON_ONCE(!ctrl->tagset); - if (test_and_clear_bit(EVENT_NS_CHANGED, &ctrl->events)) { + if (test_and_clear_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events)) { if (nvme_scan_changed_ns_log(ctrl)) goto out_sort_namespaces; dev_info(ctrl->device, "rescanning namespaces.\n"); @@ -3386,7 +3386,7 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result) { switch ((result & 0xff00) >> 8) { case NVME_AER_NOTICE_NS_CHANGED: - set_bit(EVENT_NS_CHANGED, &ctrl->events); + set_bit(NVME_AER_NOTICE_NS_CHANGED, &ctrl->events); nvme_queue_scan(ctrl); break; case NVME_AER_NOTICE_FW_ACT_STARTING: diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index de24fe77c80b..34df07d44f80 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -194,7 +194,6 @@ struct nvme_ctrl { struct delayed_work ka_work; struct nvme_command ka_cmd; struct work_struct fw_act_work; -#define EVENT_NS_CHANGED (1 << 0) unsigned long events; /* Power saving configuration */ -- cgit v1.2.3