* [PATCH] scsi_transport_sas: fix BSG ioctl memory corruption
From: Omar Sandoval @ 2017-02-21 18:03 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: Christoph Hellwig, kernel-team
From: Omar Sandoval <osandov@fb.com>
The end_device and sas_host devices support BSG ioctls, but the
request_queue allocated for them isn't set up to allocate the struct
scsi_request payload. This leads to memory corruption in the call to
scsi_req_init() in bsg_map_hdr(), since it will memset past the end of
the allocated request. Fix it by setting ->cmd_size on the allocated
request_queue.
Fixes: 82ed4db499b8 ("block: split scsi_request out of struct request")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
I don't know what sg ioctls these devices actually support, but I tested
this with sg_inq, which previously caused KASAN splats.
drivers/scsi/scsi_transport_sas.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 126a5ee00987..f94535130a34 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -227,27 +227,31 @@ static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
return 0;
}
+ q = blk_alloc_queue(GFP_KERNEL);
+ if (!q)
+ return -ENOMEM;
+ q->cmd_size = sizeof(struct scsi_request);
+
if (rphy) {
- q = blk_init_queue(sas_non_host_smp_request, NULL);
+ q->request_fn = sas_non_host_smp_request;
dev = &rphy->dev;
name = dev_name(dev);
release = NULL;
} else {
- q = blk_init_queue(sas_host_smp_request, NULL);
+ q->request_fn = sas_host_smp_request;
dev = &shost->shost_gendev;
snprintf(namebuf, sizeof(namebuf),
"sas_host%d", shost->host_no);
name = namebuf;
release = sas_host_release;
}
- if (!q)
- return -ENOMEM;
+ error = blk_init_allocated_queue(q);
+ if (error)
+ goto out_cleanup_queue;
error = bsg_register_queue(q, dev, name, release);
- if (error) {
- blk_cleanup_queue(q);
- return -ENOMEM;
- }
+ if (error)
+ goto out_cleanup_queue;
if (rphy)
rphy->q = q;
@@ -261,6 +265,10 @@ static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)
queue_flag_set_unlocked(QUEUE_FLAG_BIDI, q);
return 0;
+
+out_cleanup_queue:
+ blk_cleanup_queue(q);
+ return error;
}
static void sas_bsg_remove(struct Scsi_Host *shost, struct sas_rphy *rphy)
--
2.11.1
^ permalink raw reply related
* Re: [PATCH 0/13 v2] block: Fix block device shutdown related races
From: Jens Axboe @ 2017-02-21 17:19 UTC (permalink / raw)
To: Jan Kara
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
On 02/21/2017 10:09 AM, Jan Kara wrote:
> Hello,
>
> this is a second revision of the patch set to fix several different races and
> issues I've found when testing device shutdown and reuse. The first three
> patches are fixes to problems in my previous series fixing BDI lifetime issues.
> Patch 4 fixes issues with reuse of BDI name with scsi devices. With it I cannot
> reproduce the BDI name reuse issues using Omar's stress test using scsi_debug
> so it can be used as a replacement of Dan's patches. Patches 5-11 fix oops that
> is triggered by __blkdev_put() calling inode_detach_wb() too early (the problem
> reported by Thiago). Patches 12 and 13 fix oops due to a bug in gendisk code
> where get_gendisk() can return already freed gendisk structure (again triggered
> by Omar's stress test).
>
> People, please have a look at patches. They are mostly simple however the
> interactions are rather complex so I may have missed something. Also I'm
> happy for any additional testing these patches can get - I've stressed them
> with Omar's script, tested memcg writeback, tested static (not udev managed)
> device inodes.
>
> Jens, I think at least patches 1-3 should go in together with my fixes you
> already have in your tree (or shortly after them). It is up to you whether
> you decide to delay my first fixes or pick these up quickly. Patch 4 is
> (IMHO a cleaner) replacement of Dan's patches so consider whether you want
> to use it instead of those patches.
I have applied 1-3 to my for-linus branch, which will go in after
the initial pull request has been pulled by Linus. Consider fixing up
#4 so it applies, I like it.
I'll review the rest later this week.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 0/13 v2] block: Fix block device shutdown related races
From: Jan Kara @ 2017-02-21 17:19 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
On Tue 21-02-17 18:09:45, Jan Kara wrote:
> Hello,
>
> this is a second revision of the patch set to fix several different races and
> issues I've found when testing device shutdown and reuse. The first three
> patches are fixes to problems in my previous series fixing BDI lifetime issues.
> Patch 4 fixes issues with reuse of BDI name with scsi devices. With it I cannot
> reproduce the BDI name reuse issues using Omar's stress test using scsi_debug
> so it can be used as a replacement of Dan's patches. Patches 5-11 fix oops that
> is triggered by __blkdev_put() calling inode_detach_wb() too early (the problem
> reported by Thiago). Patches 12 and 13 fix oops due to a bug in gendisk code
> where get_gendisk() can return already freed gendisk structure (again triggered
> by Omar's stress test).
>
> People, please have a look at patches. They are mostly simple however the
> interactions are rather complex so I may have missed something. Also I'm
> happy for any additional testing these patches can get - I've stressed them
> with Omar's script, tested memcg writeback, tested static (not udev managed)
> device inodes.
>
> Jens, I think at least patches 1-3 should go in together with my fixes you
> already have in your tree (or shortly after them). It is up to you whether
> you decide to delay my first fixes or pick these up quickly. Patch 4 is
> (IMHO a cleaner) replacement of Dan's patches so consider whether you want
> to use it instead of those patches.
I forgot to add that the patches are also available from my git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git bdi
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: Manual driver binding and unbinding broken for SCSI
From: Jan Kara @ 2017-02-21 17:14 UTC (permalink / raw)
To: Omar Sandoval
Cc: James Bottomley, Dan Williams, Jan Kara, Martin K. Petersen,
Jens Axboe, linux-scsi, linux-block
In-Reply-To: <20170220021958.GA26238@vader>
On Sun 19-02-17 18:19:58, Omar Sandoval wrote:
> On Fri, Feb 17, 2017 at 04:43:56PM -0800, James Bottomley wrote:
> > This seems to be related to a 0day test we got on the block tree,
> > details here:
> >
> > http://marc.info/?t=148624068800001
> >
> > I root caused the above to something not being released when it should
> > be, so it looks like you have the same problem. It seems to be a
> > recent commit in the block tree, so could you bisect it since you have
> > a nice reproducer?
>
> These appear to actually be two separate issues.
>
> The unbind followed by bind crash only happens with scsi-mq. It reproes
> since at least 4.0.
>
> The unbind followed by a new device coming up crash happens both with
> and without scsi-mq. The earliest version I was able to check for that
> was 4.6, which did reproduce.
>
> I'll see if I can get some more info on these two issues separately.
Actually, the second issue is only a warning right? And if I understand the
issue correctly, it should be fixed by either Dan's patches in linux-block
or my patch 4 in the series which matches your test results. So that is
dealt with. I have no idea about the first issue though.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [PATCH 09/13] bdi: Do not wait for cgwbs release in bdi_unregister()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Currently we wait for all cgwbs to get released in cgwb_bdi_destroy()
(called from bdi_unregister()). That is however unnecessary now when
cgwb->bdi is a proper refcounted reference (thus bdi cannot get
released before all cgwbs are released) and when cgwb_bdi_destroy()
shuts down writeback directly.
Signed-off-by: Jan Kara <jack@suse.cz>
---
include/linux/backing-dev-defs.h | 1 -
mm/backing-dev.c | 18 +-----------------
2 files changed, 1 insertion(+), 18 deletions(-)
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 8fb3dcdebc80..7bd5ba9890b0 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -163,7 +163,6 @@ struct backing_dev_info {
#ifdef CONFIG_CGROUP_WRITEBACK
struct radix_tree_root cgwb_tree; /* radix tree of active cgroup wbs */
struct rb_root cgwb_congested_tree; /* their congested states */
- atomic_t usage_cnt; /* counts both cgwbs and cgwb_contested's */
#else
struct bdi_writeback_congested *wb_congested;
#endif
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c9623b410170..31cdee91e826 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -393,11 +393,9 @@ static void wb_exit(struct bdi_writeback *wb)
/*
* cgwb_lock protects bdi->cgwb_tree, bdi->cgwb_congested_tree,
* blkcg->cgwb_list, and memcg->cgwb_list. bdi->cgwb_tree is also RCU
- * protected. cgwb_release_wait is used to wait for the completion of cgwb
- * releases from bdi destruction path.
+ * protected.
*/
static DEFINE_SPINLOCK(cgwb_lock);
-static DECLARE_WAIT_QUEUE_HEAD(cgwb_release_wait);
/**
* wb_congested_get_create - get or create a wb_congested
@@ -492,7 +490,6 @@ static void cgwb_release_workfn(struct work_struct *work)
{
struct bdi_writeback *wb = container_of(work, struct bdi_writeback,
release_work);
- struct backing_dev_info *bdi = wb->bdi;
wb_shutdown(wb);
@@ -503,9 +500,6 @@ static void cgwb_release_workfn(struct work_struct *work)
percpu_ref_exit(&wb->refcnt);
wb_exit(wb);
kfree_rcu(wb, rcu);
-
- if (atomic_dec_and_test(&bdi->usage_cnt))
- wake_up_all(&cgwb_release_wait);
}
static void cgwb_release(struct percpu_ref *refcnt)
@@ -595,7 +589,6 @@ static int cgwb_create(struct backing_dev_info *bdi,
/* we might have raced another instance of this function */
ret = radix_tree_insert(&bdi->cgwb_tree, memcg_css->id, wb);
if (!ret) {
- atomic_inc(&bdi->usage_cnt);
list_add_tail_rcu(&wb->bdi_node, &bdi->wb_list);
list_add(&wb->memcg_node, memcg_cgwb_list);
list_add(&wb->blkcg_node, blkcg_cgwb_list);
@@ -685,7 +678,6 @@ static int cgwb_bdi_init(struct backing_dev_info *bdi)
INIT_RADIX_TREE(&bdi->cgwb_tree, GFP_ATOMIC);
bdi->cgwb_congested_tree = RB_ROOT;
- atomic_set(&bdi->usage_cnt, 1);
ret = wb_init(&bdi->wb, bdi, 1, GFP_KERNEL);
if (!ret) {
@@ -726,14 +718,6 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
}
spin_unlock_irq(&cgwb_lock);
-
- /*
- * All cgwb's and their congested states must be shutdown and
- * released before returning. Drain the usage counter to wait for
- * all cgwb's and cgwb_congested's ever created on @bdi.
- */
- atomic_dec(&bdi->usage_cnt);
- wait_event(cgwb_release_wait, !atomic_read(&bdi->usage_cnt));
}
/**
--
2.10.2
^ permalink raw reply related
* [PATCH 13/13] block: Fix oops scsi_disk_get()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
When device open races with device shutdown, we can get the following
oops in scsi_disk_get():
[11863.044351] general protection fault: 0000 [#1] SMP
[11863.045561] Modules linked in: scsi_debug xfs libcrc32c netconsole btrfs raid6_pq zlib_deflate lzo_compress xor [last unloaded: loop]
[11863.047853] CPU: 3 PID: 13042 Comm: hald-probe-stor Tainted: G W 4.10.0-rc2-xen+ #35
[11863.048030] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[11863.048030] task: ffff88007f438200 task.stack: ffffc90000fd0000
[11863.048030] RIP: 0010:scsi_disk_get+0x43/0x70
[11863.048030] RSP: 0018:ffffc90000fd3a08 EFLAGS: 00010202
[11863.048030] RAX: 6b6b6b6b6b6b6b6b RBX: ffff88007f56d000 RCX: 0000000000000000
[11863.048030] RDX: 0000000000000001 RSI: 0000000000000004 RDI: ffffffff81a8d880
[11863.048030] RBP: ffffc90000fd3a18 R08: 0000000000000000 R09: 0000000000000001
[11863.059217] R10: 0000000000000000 R11: 0000000000000000 R12: 00000000fffffffa
[11863.059217] R13: ffff880078872800 R14: ffff880070915540 R15: 000000000000001d
[11863.059217] FS: 00007f2611f71800(0000) GS:ffff88007f0c0000(0000) knlGS:0000000000000000
[11863.059217] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[11863.059217] CR2: 000000000060e048 CR3: 00000000778d4000 CR4: 00000000000006e0
[11863.059217] Call Trace:
[11863.059217] ? disk_get_part+0x22/0x1f0
[11863.059217] sd_open+0x39/0x130
[11863.059217] __blkdev_get+0x69/0x430
[11863.059217] ? bd_acquire+0x7f/0xc0
[11863.059217] ? bd_acquire+0x96/0xc0
[11863.059217] ? blkdev_get+0x350/0x350
[11863.059217] blkdev_get+0x126/0x350
[11863.059217] ? _raw_spin_unlock+0x2b/0x40
[11863.059217] ? bd_acquire+0x7f/0xc0
[11863.059217] ? blkdev_get+0x350/0x350
[11863.059217] blkdev_open+0x65/0x80
...
As you can see RAX value is already poisoned showing that gendisk we got
is already freed. The problem is that get_gendisk() looks up device
number in ext_devt_idr and then does get_disk() which does kobject_get()
on the disks kobject. However the disk gets removed from ext_devt_idr
only in disk_release() (through blk_free_devt()) at which moment it has
already 0 refcount and is already on its way to be freed. Indeed we've
got a warning from kobject_get() about 0 refcount shortly before the
oops.
We fix the problem by using kobject_get_unless_zero() in get_disk() so
that get_disk() cannot get reference on a disk that is already being
freed.
Tested-by: Lekshmi Pillai <lekshmicpillai@in.ibm.com>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
block/genhd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/genhd.c b/block/genhd.c
index 68c613edb93a..2baacfea7b5e 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1350,7 +1350,7 @@ struct kobject *get_disk(struct gendisk *disk)
owner = disk->fops->owner;
if (owner && !try_module_get(owner))
return NULL;
- kobj = kobject_get(&disk_to_dev(disk)->kobj);
+ kobj = kobject_get_unless_zero(&disk_to_dev(disk)->kobj);
if (kobj == NULL) {
module_put(owner);
return NULL;
--
2.10.2
^ permalink raw reply related
* [PATCH 12/13] kobject: Export kobject_get_unless_zero()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara, Greg Kroah-Hartman
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Make the function available for outside use and fortify it against NULL
kobject.
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
include/linux/kobject.h | 2 ++
lib/kobject.c | 5 ++++-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index e6284591599e..ca85cb80e99a 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -108,6 +108,8 @@ extern int __must_check kobject_rename(struct kobject *, const char *new_name);
extern int __must_check kobject_move(struct kobject *, struct kobject *);
extern struct kobject *kobject_get(struct kobject *kobj);
+extern struct kobject * __must_check kobject_get_unless_zero(
+ struct kobject *kobj);
extern void kobject_put(struct kobject *kobj);
extern const void *kobject_namespace(struct kobject *kobj);
diff --git a/lib/kobject.c b/lib/kobject.c
index 445dcaeb0f56..763d70a18941 100644
--- a/lib/kobject.c
+++ b/lib/kobject.c
@@ -601,12 +601,15 @@ struct kobject *kobject_get(struct kobject *kobj)
}
EXPORT_SYMBOL(kobject_get);
-static struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj)
+struct kobject * __must_check kobject_get_unless_zero(struct kobject *kobj)
{
+ if (!kobj)
+ return NULL;
if (!kref_get_unless_zero(&kobj->kref))
kobj = NULL;
return kobj;
}
+EXPORT_SYMBOL(kobject_get_unless_zero);
/*
* kobject_cleanup - free kobject resources.
--
2.10.2
^ permalink raw reply related
* [PATCH 11/13] block: Fix oops in locked_inode_to_wb_and_lock_list()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
When block device is closed, we call inode_detach_wb() in __blkdev_put()
which sets inode->i_wb to NULL. That is contrary to expectations that
inode->i_wb stays valid once set during the whole inode's lifetime and
leads to oops in wb_get() in locked_inode_to_wb_and_lock_list() because
inode_to_wb() returned NULL.
The reason why we called inode_detach_wb() is not valid anymore though.
BDI is guaranteed to stay along until we call bdi_put() from
bdev_evict_inode() so we can postpone calling inode_detach_wb() to that
moment.
Also add a warning to catch if someone uses inode_detach_wb() in a
dangerous way.
Reported-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/block_dev.c | 8 ++------
include/linux/writeback.h | 1 +
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 68e855fdce58..a0a8a000bdde 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -884,6 +884,8 @@ static void bdev_evict_inode(struct inode *inode)
spin_lock(&bdev_lock);
list_del_init(&bdev->bd_list);
spin_unlock(&bdev_lock);
+ /* Detach inode from wb early as bdi_put() may free bdi->wb */
+ inode_detach_wb(inode);
if (bdev->bd_bdi != &noop_backing_dev_info)
bdi_put(bdev->bd_bdi);
}
@@ -1874,12 +1876,6 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
kill_bdev(bdev);
bdev_write_inode(bdev);
- /*
- * Detaching bdev inode from its wb in __destroy_inode()
- * is too late: the queue which embeds its bdi (along with
- * root wb) can be gone as soon as we put_disk() below.
- */
- inode_detach_wb(bdev->bd_inode);
}
if (bdev->bd_contains == bdev) {
if (disk->fops->release)
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 5527d910ba3d..f1af3f67ce5a 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -237,6 +237,7 @@ static inline void inode_attach_wb(struct inode *inode, struct page *page)
static inline void inode_detach_wb(struct inode *inode)
{
if (inode->i_wb) {
+ WARN_ON_ONCE(!(inode->i_state & I_CLEAR));
wb_put(inode->i_wb);
inode->i_wb = NULL;
}
--
2.10.2
^ permalink raw reply related
* [PATCH 10/13] bdi: Rename cgwb_bdi_destroy() to cgwb_bdi_unregister()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Rename cgwb_bdi_destroy() to cgwb_bdi_unregister() as it gets called
from bdi_unregister() which is not necessarily called from bdi_destroy()
and thus the name is somewhat misleading.
Signed-off-by: Jan Kara <jack@suse.cz>
---
mm/backing-dev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 31cdee91e826..b1b50cce3f36 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -687,7 +687,7 @@ static int cgwb_bdi_init(struct backing_dev_info *bdi)
return ret;
}
-static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
+static void cgwb_bdi_unregister(struct backing_dev_info *bdi)
{
struct radix_tree_iter iter;
struct rb_node *rbn;
@@ -777,7 +777,7 @@ static int cgwb_bdi_init(struct backing_dev_info *bdi)
static void cgwb_remove_from_bdi_list(struct bdi_writeback *wb) { }
-static void cgwb_bdi_destroy(struct backing_dev_info *bdi) { }
+static void cgwb_bdi_unregister(struct backing_dev_info *bdi) { }
#endif /* CONFIG_CGROUP_WRITEBACK */
@@ -885,7 +885,7 @@ void bdi_unregister(struct backing_dev_info *bdi)
/* make sure nobody finds us on the bdi_list anymore */
bdi_remove_from_list(bdi);
wb_shutdown(&bdi->wb);
- cgwb_bdi_destroy(bdi);
+ cgwb_bdi_unregister(bdi);
if (bdi->dev) {
bdi_debug_unregister(bdi);
--
2.10.2
^ permalink raw reply related
* [PATCH 08/13] bdi: Shutdown writeback on all cgwbs in cgwb_bdi_destroy()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Currently we waited for all cgwbs to get freed in cgwb_bdi_destroy()
which also means that writeback has been shutdown on them. Since this
wait is going away, directly shutdown writeback on cgwbs from
cgwb_bdi_destroy() to avoid live writeback structures after
bdi_unregister() has finished.
Signed-off-by: Jan Kara <jack@suse.cz>
---
mm/backing-dev.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 54b9e934eef4..c9623b410170 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -700,6 +700,7 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
struct radix_tree_iter iter;
struct rb_node *rbn;
void **slot;
+ struct wb_writeback *wb;
WARN_ON(test_bit(WB_registered, &bdi->wb.state));
@@ -716,6 +717,14 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
congested->__bdi = NULL; /* mark @congested unlinked */
}
+ while (!list_empty(&bdi->wb_list)) {
+ wb = list_first_entry(&bdi->wb_list, struct bdi_writeback,
+ bdi_node);
+ spin_unlock_irq(&cgwb_lock);
+ wb_shutdown(wb);
+ spin_lock_irq(&cgwb_lock);
+ }
+
spin_unlock_irq(&cgwb_lock);
/*
--
2.10.2
^ permalink raw reply related
* [PATCH 07/13] bdi: Move removal from bdi->wb_list into wb_shutdown()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Currently the removal from bdi->wb_list happens directly in
cgwb_release_workfn(). Move it to wb_shutdown() which is functionally
equivalent and more logical (the list gets only used for distributing
writeback works among bdi_writeback structures). It will also allow us
to simplify writeback shutdown in cgwb_bdi_destroy().
Signed-off-by: Jan Kara <jack@suse.cz>
---
mm/backing-dev.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index d7aaf2517c30..54b9e934eef4 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -345,6 +345,8 @@ static int wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi,
return err;
}
+static void cgwb_remove_from_bdi_list(struct bdi_writeback *wb);
+
/*
* Remove bdi from the global list and shutdown any threads we have running
*/
@@ -358,6 +360,7 @@ static void wb_shutdown(struct bdi_writeback *wb)
}
spin_unlock_bh(&wb->work_lock);
+ cgwb_remove_from_bdi_list(wb);
/*
* Drain work list and shutdown the delayed_work. !WB_registered
* tells wb_workfn() that @wb is dying and its work_list needs to
@@ -491,10 +494,6 @@ static void cgwb_release_workfn(struct work_struct *work)
release_work);
struct backing_dev_info *bdi = wb->bdi;
- spin_lock_irq(&cgwb_lock);
- list_del_rcu(&wb->bdi_node);
- spin_unlock_irq(&cgwb_lock);
-
wb_shutdown(wb);
css_put(wb->memcg_css);
@@ -526,6 +525,13 @@ static void cgwb_kill(struct bdi_writeback *wb)
percpu_ref_kill(&wb->refcnt);
}
+static void cgwb_remove_from_bdi_list(struct bdi_writeback *wb)
+{
+ spin_lock_irq(&cgwb_lock);
+ list_del_rcu(&wb->bdi_node);
+ spin_unlock_irq(&cgwb_lock);
+}
+
static int cgwb_create(struct backing_dev_info *bdi,
struct cgroup_subsys_state *memcg_css, gfp_t gfp)
{
@@ -776,6 +782,8 @@ static int cgwb_bdi_init(struct backing_dev_info *bdi)
return 0;
}
+static void cgwb_remove_from_bdi_list(struct bdi_writeback *wb) { }
+
static void cgwb_bdi_destroy(struct backing_dev_info *bdi) { }
#endif /* CONFIG_CGROUP_WRITEBACK */
--
2.10.2
^ permalink raw reply related
* [PATCH 06/13] bdi: Make wb->bdi a proper reference
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Make wb->bdi a proper refcounted reference to bdi for all bdi_writeback
structures except for the one embedded inside struct backing_dev_info.
That will allow us to simplify bdi unregistration.
Signed-off-by: Jan Kara <jack@suse.cz>
---
mm/backing-dev.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index c324eae17f0d..d7aaf2517c30 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -294,6 +294,8 @@ static int wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi,
memset(wb, 0, sizeof(*wb));
+ if (wb != &bdi->wb)
+ bdi_get(bdi);
wb->bdi = bdi;
wb->last_old_flush = jiffies;
INIT_LIST_HEAD(&wb->b_dirty);
@@ -314,8 +316,10 @@ static int wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi,
wb->dirty_sleep = jiffies;
wb->congested = wb_congested_get_create(bdi, blkcg_id, gfp);
- if (!wb->congested)
- return -ENOMEM;
+ if (!wb->congested) {
+ err = -ENOMEM;
+ goto out_put_bdi;
+ }
err = fprop_local_init_percpu(&wb->completions, gfp);
if (err)
@@ -335,6 +339,9 @@ static int wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi,
fprop_local_destroy_percpu(&wb->completions);
out_put_cong:
wb_congested_put(wb->congested);
+out_put_bdi:
+ if (wb != &bdi->wb)
+ bdi_put(bdi);
return err;
}
@@ -372,6 +379,8 @@ static void wb_exit(struct bdi_writeback *wb)
fprop_local_destroy_percpu(&wb->completions);
wb_congested_put(wb->congested);
+ if (wb != &wb->bdi->wb)
+ bdi_put(wb->bdi);
}
#ifdef CONFIG_CGROUP_WRITEBACK
--
2.10.2
^ permalink raw reply related
* [PATCH 05/13] bdi: Mark congested->bdi as internal
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
congested->bdi pointer is used only to be able to remove congested
structure from bdi->cgwb_congested_tree on structure release. Moreover
the pointer can become NULL when we unregister the bdi. Rename the field
to __bdi and add a comment to make it more explicit this is internal
stuff of memcg writeback code and people should not use the field as
such use will be likely race prone.
We do not bother with converting congested->bdi to a proper refcounted
reference. It will be slightly ugly to special-case bdi->wb.congested to
avoid effectively a cyclic reference of bdi to itself and the reference
gets cleared from bdi_unregister() making it impossible to reference
a freed bdi.
Signed-off-by: Jan Kara <jack@suse.cz>
---
include/linux/backing-dev-defs.h | 4 +++-
mm/backing-dev.c | 10 +++++-----
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index ad955817916d..8fb3dcdebc80 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -54,7 +54,9 @@ struct bdi_writeback_congested {
atomic_t refcnt; /* nr of attached wb's and blkg */
#ifdef CONFIG_CGROUP_WRITEBACK
- struct backing_dev_info *bdi; /* the associated bdi */
+ struct backing_dev_info *__bdi; /* the associated bdi, set to NULL
+ * on bdi unregistration. For memcg-wb
+ * internal use only! */
int blkcg_id; /* ID of the associated blkcg */
struct rb_node rb_node; /* on bdi->cgwb_congestion_tree */
#endif
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 28ce6cf7b2ff..c324eae17f0d 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -438,7 +438,7 @@ wb_congested_get_create(struct backing_dev_info *bdi, int blkcg_id, gfp_t gfp)
return NULL;
atomic_set(&new_congested->refcnt, 0);
- new_congested->bdi = bdi;
+ new_congested->__bdi = bdi;
new_congested->blkcg_id = blkcg_id;
goto retry;
@@ -466,10 +466,10 @@ void wb_congested_put(struct bdi_writeback_congested *congested)
}
/* bdi might already have been destroyed leaving @congested unlinked */
- if (congested->bdi) {
+ if (congested->__bdi) {
rb_erase(&congested->rb_node,
- &congested->bdi->cgwb_congested_tree);
- congested->bdi = NULL;
+ &congested->__bdi->cgwb_congested_tree);
+ congested->__bdi = NULL;
}
spin_unlock_irqrestore(&cgwb_lock, flags);
@@ -698,7 +698,7 @@ static void cgwb_bdi_destroy(struct backing_dev_info *bdi)
rb_entry(rbn, struct bdi_writeback_congested, rb_node);
rb_erase(rbn, &bdi->cgwb_congested_tree);
- congested->bdi = NULL; /* mark @congested unlinked */
+ congested->__bdi = NULL; /* mark @congested unlinked */
}
spin_unlock_irq(&cgwb_lock);
--
2.10.2
^ permalink raw reply related
* [PATCH 04/13] block: Move bdi_unregister() to del_gendisk()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Commit 6cd18e711dd8 "block: destroy bdi before blockdev is
unregistered." moved bdi unregistration (at that time through
bdi_destroy()) from blk_release_queue() to blk_cleanup_queue() because
it needs to happen before blk_unregister_region() call in del_gendisk()
for MD. As much as it is fine for device registration / unregistration
purposes, it does not fit our needs wrt writeback code. For those we
will need bdi_unregister() to happen after bdev_unhash_inode() so that
we are sure bdev inode is destroyed or soon to be destroyed (as soon as
last inode reference is dropped and nobody should be holding bdev inode
reference for long at this point) because bdi_unregister() may block
waiting for bdev's inode i_wb reference to be dropped and that happens
only once bdev inode gets destroyed.
Also SCSI will free up the device number from sd_remove() called through
a maze of callbacks from device_del() in __scsi_remove_device() before
blk_cleanup_queue() and thus similar races as described in 6cd18e711dd8
can happen for SCSI as well as reported by Omar [1]. Moving
bdi_unregister() to del_gendisk() fixes these problems as well since
del_gendisk() gets called from sd_remove() before freeing the device
number.
This also makes device_add_disk() (calling bdi_register_owner()) more
symmetric with del_gendisk().
[1] http://marc.info/?l=linux-block&m=148554717109098&w=2
Tested-by: Lekshmi Pillai <lekshmicpillai@in.ibm.com>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
block/blk-core.c | 2 --
block/genhd.c | 7 +++++++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 47104f6a398b..9a901dcfdd5c 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -580,8 +580,6 @@ void blk_cleanup_queue(struct request_queue *q)
q->queue_lock = &q->__queue_lock;
spin_unlock_irq(lock);
- bdi_unregister(q->backing_dev_info);
-
/* @q is and will stay empty, shutdown and put */
blk_put_queue(q);
}
diff --git a/block/genhd.c b/block/genhd.c
index f6c4d4400759..68c613edb93a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -660,6 +660,13 @@ void del_gendisk(struct gendisk *disk)
disk->flags &= ~GENHD_FL_UP;
sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
+ /*
+ * Unregister bdi before releasing device numbers (as they can get
+ * reused and we'd get clashes in sysfs) but after bdev inodes are
+ * unhashed and thus will be soon destroyed as bdev inode's reference
+ * to wb_writeback can block bdi_unregister().
+ */
+ bdi_unregister(disk->queue->backing_dev_info);
blk_unregister_queue(disk);
blk_unregister_region(disk_devt(disk), disk->minors);
--
2.10.2
^ permalink raw reply related
* [PATCH 03/13] block: Revalidate i_bdev reference in bd_aquire()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
When a device gets removed, block device inode unhashed so that it is not
used anymore (bdget() will not find it anymore). Later when a new device
gets created with the same device number, we create new block device
inode. However there may be file system device inodes whose i_bdev still
points to the original block device inode and thus we get two active
block device inodes for the same device. They will share the same
gendisk so the only visible differences will be that page caches will
not be coherent and BDIs will be different (the old block device inode
still points to unregistered BDI).
Fix the problem by checking in bd_acquire() whether i_bdev still points
to active block device inode and re-lookup the block device if not. That
way any open of a block device happening after the old device has been
removed will get correct block device inode.
Tested-by: Lekshmi Pillai <lekshmicpillai@in.ibm.com>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/block_dev.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 601b71b76d7f..68e855fdce58 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1043,13 +1043,22 @@ static struct block_device *bd_acquire(struct inode *inode)
spin_lock(&bdev_lock);
bdev = inode->i_bdev;
- if (bdev) {
+ if (bdev && !inode_unhashed(bdev->bd_inode)) {
bdgrab(bdev);
spin_unlock(&bdev_lock);
return bdev;
}
spin_unlock(&bdev_lock);
+ /*
+ * i_bdev references block device inode that was already shut down
+ * (corresponding device got removed). Remove the reference and look
+ * up block device inode again just in case new device got
+ * reestablished under the same device number.
+ */
+ if (bdev)
+ bd_forget(inode);
+
bdev = bdget(inode->i_rdev);
if (bdev) {
spin_lock(&bdev_lock);
--
2.10.2
^ permalink raw reply related
* [PATCH 02/13] block: Unhash also block device inode for the whole device
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Iteration over partitions in del_gendisk() omits part0. Add
bdev_unhash_inode() call for the whole device. Otherwise if the device
number gets reused, bdev inode will be still associated with the old
(stale) bdi.
Tested-by: Lekshmi Pillai <lekshmicpillai@in.ibm.com>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
block/genhd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/genhd.c b/block/genhd.c
index 6cb9f3a34a92..f6c4d4400759 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -655,6 +655,7 @@ void del_gendisk(struct gendisk *disk)
disk_part_iter_exit(&piter);
invalidate_partition(disk, 0);
+ bdev_unhash_inode(disk_devt(disk));
set_capacity(disk, 0);
disk->flags &= ~GENHD_FL_UP;
--
2.10.2
^ permalink raw reply related
* [PATCH 01/13] block: Move bdev_unhash_inode() after invalidate_partition()
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
In-Reply-To: <20170221170958.21845-1-jack@suse.cz>
Move bdev_unhash_inode() after invalidate_partition() as
invalidate_partition() looks up bdev and it cannot find the right bdev
inode after bdev_unhash_inode() is called. Thus invalidate_partition()
would not invalidate page cache of the previously used bdev. Also use
part_devt() when calling bdev_unhash_inode() instead of manually
creating the device number.
Tested-by: Lekshmi Pillai <lekshmicpillai@in.ibm.com>
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
block/genhd.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index d9ccd42f3675..6cb9f3a34a92 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -648,9 +648,8 @@ void del_gendisk(struct gendisk *disk)
disk_part_iter_init(&piter, disk,
DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
while ((part = disk_part_iter_next(&piter))) {
- bdev_unhash_inode(MKDEV(disk->major,
- disk->first_minor + part->partno));
invalidate_partition(disk, part->partno);
+ bdev_unhash_inode(part_devt(part));
delete_partition(disk, part->partno);
}
disk_part_iter_exit(&piter);
--
2.10.2
^ permalink raw reply related
* [PATCH 0/13 v2] block: Fix block device shutdown related races
From: Jan Kara @ 2017-02-21 17:09 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-block, Christoph Hellwig, Dan Williams,
Thiago Jung Bauermann, Lekshmi Pillai, Tejun Heo, NeilBrown,
Omar Sandoval, Jan Kara
Hello,
this is a second revision of the patch set to fix several different races and
issues I've found when testing device shutdown and reuse. The first three
patches are fixes to problems in my previous series fixing BDI lifetime issues.
Patch 4 fixes issues with reuse of BDI name with scsi devices. With it I cannot
reproduce the BDI name reuse issues using Omar's stress test using scsi_debug
so it can be used as a replacement of Dan's patches. Patches 5-11 fix oops that
is triggered by __blkdev_put() calling inode_detach_wb() too early (the problem
reported by Thiago). Patches 12 and 13 fix oops due to a bug in gendisk code
where get_gendisk() can return already freed gendisk structure (again triggered
by Omar's stress test).
People, please have a look at patches. They are mostly simple however the
interactions are rather complex so I may have missed something. Also I'm
happy for any additional testing these patches can get - I've stressed them
with Omar's script, tested memcg writeback, tested static (not udev managed)
device inodes.
Jens, I think at least patches 1-3 should go in together with my fixes you
already have in your tree (or shortly after them). It is up to you whether
you decide to delay my first fixes or pick these up quickly. Patch 4 is
(IMHO a cleaner) replacement of Dan's patches so consider whether you want
to use it instead of those patches.
Changes since v1:
* Added Acks and Tested-by tags for patches in areas that did not change
* Reworked inode_detach_wb() related fixes based on Tejun's feedback
Honza
^ permalink raw reply
* Re: [PATCH v2] scsi: zero per-cmd driver data before each I/O
From: Jens Axboe @ 2017-02-21 16:19 UTC (permalink / raw)
To: Christoph Hellwig, martin.petersen; +Cc: decui, linux-block, linux-scsi
In-Reply-To: <20170221090455.4633-1-hch@lst.de>
On 02/21/2017 02:04 AM, Christoph Hellwig wrote:
> Without this drivers that don't clear the state themselves can see off
> effects. For example Hyper-V VMs using the storvsc driver will often
> hang during boot due to uncleared Test Unit Ready failures.
>
> Fixes: e9c787e6 ("scsi: allocate scsi_cmnd structures as part of struct request")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Dexuan Cui <decui@microsoft.com>
> Tested-by: Dexuan Cui <decui@microsoft.com>
> ---
>
> Changes since V1:
> - use a single memset as suggested by Bart
That's much better. I'll queue this up for the next pull in this merge
window.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH v2] scsi: zero per-cmd driver data before each I/O
From: Bart Van Assche @ 2017-02-21 15:33 UTC (permalink / raw)
To: Christoph Hellwig, axboe@kernel.dk, martin.petersen@oracle.com
Cc: decui@microsoft.com, linux-block@vger.kernel.org,
linux-scsi@vger.kernel.org
In-Reply-To: <20170221090455.4633-1-hch@lst.de>
On 02/21/2017 01:29 AM, Christoph Hellwig wrote:=0A=
> Without this drivers that don't clear the state themselves can see off=0A=
> effects. For example Hyper-V VMs using the storvsc driver will often=0A=
> hang during boot due to uncleared Test Unit Ready failures.=0A=
> =0A=
> Fixes: e9c787e6 ("scsi: allocate scsi_cmnd structures as part of struct r=
equest")=0A=
> Signed-off-by: Christoph Hellwig <hch@lst.de>=0A=
> Reported-by: Dexuan Cui <decui@microsoft.com>=0A=
> Tested-by: Dexuan Cui <decui@microsoft.com>=0A=
> ---=0A=
> =0A=
> Changes since V1:=0A=
> - use a single memset as suggested by Bart=0A=
> =0A=
> drivers/scsi/scsi_lib.c | 2 +-=0A=
> 1 file changed, 1 insertion(+), 1 deletion(-)=0A=
> =0A=
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c=0A=
> index 90f65c8f487a..1d87a809944d 100644=0A=
> --- a/drivers/scsi/scsi_lib.c=0A=
> +++ b/drivers/scsi/scsi_lib.c=0A=
> @@ -1166,7 +1166,7 @@ void scsi_init_command(struct scsi_device *dev, str=
uct scsi_cmnd *cmd)=0A=
> =0A=
> /* zero out the cmd, except for the embedded scsi_request */=0A=
> memset((char *)cmd + sizeof(cmd->req), 0,=0A=
> - sizeof(*cmd) - sizeof(cmd->req));=0A=
> + sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);=0A=
> =0A=
> cmd->device =3D dev;=0A=
> cmd->sense_buffer =3D buf;=0A=
=0A=
Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>=0A=
=0A=
=0A=
^ permalink raw reply
* [PATCH v2] scsi: zero per-cmd driver data before each I/O
From: Christoph Hellwig @ 2017-02-21 9:04 UTC (permalink / raw)
To: axboe, martin.petersen; +Cc: decui, linux-block, linux-scsi
Without this drivers that don't clear the state themselves can see off
effects. For example Hyper-V VMs using the storvsc driver will often
hang during boot due to uncleared Test Unit Ready failures.
Fixes: e9c787e6 ("scsi: allocate scsi_cmnd structures as part of struct request")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Dexuan Cui <decui@microsoft.com>
Tested-by: Dexuan Cui <decui@microsoft.com>
---
Changes since V1:
- use a single memset as suggested by Bart
drivers/scsi/scsi_lib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 90f65c8f487a..1d87a809944d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1166,7 +1166,7 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
/* zero out the cmd, except for the embedded scsi_request */
memset((char *)cmd + sizeof(cmd->req), 0,
- sizeof(*cmd) - sizeof(cmd->req));
+ sizeof(*cmd) - sizeof(cmd->req) + dev->host->hostt->cmd_size);
cmd->device = dev;
cmd->sense_buffer = buf;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] scsi: zero per-cmd driver data before each I/O
From: Hannes Reinecke @ 2017-02-21 6:48 UTC (permalink / raw)
To: Christoph Hellwig, axboe, martin.petersen; +Cc: decui, linux-block, linux-scsi
In-Reply-To: <20170220165239.25669-1-hch@lst.de>
On 02/20/2017 05:52 PM, Christoph Hellwig wrote:
> Without this drivers that don't clear the state themselves can see off
> effects. For example Hyper-V VMs using the storvsc driver will often
> hang during boot due to uncleared Test Unit Ready failures.
>
> Fixes: e9c787e6 ("scsi: allocate scsi_cmnd structures as part of struct request")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Dexuan Cui <decui@microsoft.com>
> Tested-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/scsi/scsi_lib.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 90f65c8f487a..daced9db8af8 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1167,6 +1167,7 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
> /* zero out the cmd, except for the embedded scsi_request */
> memset((char *)cmd + sizeof(cmd->req), 0,
> sizeof(*cmd) - sizeof(cmd->req));
> + memset((char *)(cmd + 1), 0, dev->host->hostt->cmd_size);
>
> cmd->device = dev;
> cmd->sense_buffer = buf;
>
Reviewed-by: Hannes Reinecke <hare@suse.com>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Bart Van Assche @ 2017-02-21 1:18 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig
Cc: Linus Torvalds, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, Mike Snitzer
In-Reply-To: <f9b3b2ca-b19b-7c4a-52bb-789c3e2b39e5@kernel.dk>
On 02/20/2017 08:32 AM, Jens Axboe wrote:=0A=
> Bart, since you are the only one that can reproduce this, can you just bi=
sect=0A=
> your way through that series?=0A=
=0A=
Hello Jens,=0A=
=0A=
I will do that as soon as I'm back in the office (later this week).=0A=
=0A=
Bart.=0A=
=0A=
=0A=
^ permalink raw reply
* blk_integrity_revalidate() clears BDI_CAP_STABLE_WRITES
From: Ilya Dryomov @ 2017-02-20 18:45 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Ceph Development, linux-block, Dan Williams, Christoph Hellwig
Hello,
rbd requires stable pages if ceph message data CRCs are enabled. To
that end we set BDI_CAP_STABLE_WRITES in rbd_init_disk(), but since
commit 25520d55cdb6 ("block: Inline blk_integrity in struct gendisk",
went into 4.4) this bit is almost immediately cleared:
add_disk
register_disk
blkdev_get
rescan_partitions
blk_integrity_revalidate
void blk_integrity_revalidate(struct gendisk *disk)
{
struct blk_integrity *bi = &disk->queue->integrity;
if (!(disk->flags & GENHD_FL_UP))
return;
if (bi->profile)
disk->queue->backing_dev_info.capabilities |=
BDI_CAP_STABLE_WRITES;
else
disk->queue->backing_dev_info.capabilities &=
~BDI_CAP_STABLE_WRITES;
}
ceph messenger is responsible for generating/verifying CRCs, so we
don't call blk_integrity_register() -- bi->profile is NULL.
Martin, could you please explain blk_integrity_revalidate() and its
GENHD_FL_UP check in particular? We have the queue, bi->profile can't
be NULL after blk_integrity_register(), and since the latter "must" be
used for registering the profile with the block layer, wouldn't the
following be sufficient for blk_integrity users?
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index d69c5c79f98e..319f2e4f4a8b 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -417,7 +417,7 @@ void blk_integrity_register(struct gendisk *disk,
struct blk_integrity *template
bi->tuple_size = template->tuple_size;
bi->tag_size = template->tag_size;
- blk_integrity_revalidate(disk);
+ disk->queue->backing_dev_info.capabilities |= BDI_CAP_STABLE_WRITES;
}
EXPORT_SYMBOL(blk_integrity_register);
@@ -430,26 +430,11 @@ EXPORT_SYMBOL(blk_integrity_register);
*/
void blk_integrity_unregister(struct gendisk *disk)
{
- blk_integrity_revalidate(disk);
+ disk->queue->backing_dev_info.capabilities &= ~BDI_CAP_STABLE_WRITES;
memset(&disk->queue->integrity, 0, sizeof(struct blk_integrity));
}
EXPORT_SYMBOL(blk_integrity_unregister);
blk_integrity_revalidate() can then go away -- I can send the full
patch later.
The alternative seems to be to set up a bogus blk_integrity_profile
(nop_profile won't do -- this one would have to be truly bogus w/ NULL
->*_fn) under BLK_DEV_INTEGRITY ifdefs and hope that nothing breaks.
That can't be the right thing to do here...
Thanks,
Ilya
^ permalink raw reply related
* Re: [PATCH 0/5] block subsystem refcounter conversions
From: James Bottomley @ 2017-02-20 17:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Jens Axboe, Elena Reshetova, linux-kernel, linux-block,
linux-scsi, linux-btrfs, gregkh, fujita.tomonori, mingo, clm,
jbacik, dsterba
In-Reply-To: <20170220165639.GE6515@twins.programming.kicks-ass.net>
On Mon, 2017-02-20 at 17:56 +0100, Peter Zijlstra wrote:
> On Mon, Feb 20, 2017 at 07:41:01AM -0800, James Bottomley wrote:
> > On Mon, 2017-02-20 at 08:15 -0700, Jens Axboe wrote:
> > > On 02/20/2017 04:16 AM, Elena Reshetova wrote:
> > > > Now when new refcount_t type and API are finally merged
> > > > (see include/linux/refcount.h), the following
> > > > patches convert various refcounters in the block susystem from
> > > > atomic_t to refcount_t. By doing this we prevent intentional or
> > > > accidental underflows or overflows that can led to use-after
> > > > -free vulnerabilities.
> >
> > This description isn't right ... nothing is prevented; we get
> > warnings on saturation and use after free with this.
>
> The thing that is prevented is overflow and then a use-after-free by
> making it a leak.
>
> Modular stuff, you put and free at: (n+1) mod n, by saturating at n-1
> we'll never get there.
>
> So you loose use-after-free, you gain a resource leak. The general
> idea being that use-after-free is a nice trampoline for exploits,
> leaks are 'only' a DoS.
OK, I see the intention: it's protection from outside influence. It
still doesn't prevent *us* from screwing up in the kernel and inducing
a use after free by doing too many puts (or too few gets) ... that's
what the message suggests to me (me coding wrongly is accidental
underflows or overflows as I read it).
James
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox