* 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
* Re: [PATCH] scsi: zero per-cmd driver data before each I/O
From: Bart Van Assche @ 2017-02-20 17:00 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: <20170220165239.25669-1-hch@lst.de>
On 02/20/2017 08:52 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=
> drivers/scsi/scsi_lib.c | 1 +=0A=
> 1 file changed, 1 insertion(+)=0A=
> =0A=
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c=0A=
> index 90f65c8f487a..daced9db8af8 100644=0A=
> --- a/drivers/scsi/scsi_lib.c=0A=
> +++ b/drivers/scsi/scsi_lib.c=0A=
> @@ -1167,6 +1167,7 @@ void scsi_init_command(struct scsi_device *dev, str=
uct scsi_cmnd *cmd)=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=
> + memset((char *)(cmd + 1), 0, dev->host->hostt->cmd_size);=0A=
=0A=
Hello Christoph,=0A=
=0A=
Since this code is in the hot path: please combine the two memset()=0A=
calls into a single memset() call.=0A=
=0A=
Thanks,=0A=
=0A=
Bart.=0A=
^ permalink raw reply
* Re: [PATCH 08/10] block: Fix oops in locked_inode_to_wb_and_lock_list()
From: Jan Kara @ 2017-02-20 16:58 UTC (permalink / raw)
To: Tejun Heo
Cc: Jan Kara, Jens Axboe, linux-block, Christoph Hellwig,
Dan Williams, Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170212044027.GF29323@mtj.duckdns.org>
On Sun 12-02-17 13:40:27, Tejun Heo wrote:
> Hello, Jan.
>
> On Thu, Feb 09, 2017 at 01:44:31PM +0100, Jan Kara wrote:
> > 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. A complication is that i_wb can point to non-root wb_writeback
> > structure and in that case we do need to clean it up as bdi_unregister()
> > blocks waiting for all non-root wb_writeback references to get dropped.
> > Thus this i_wb reference could block device removal e.g. from
> > __scsi_remove_device() (which indirectly ends up calling
> > bdi_unregister()). We cannot rely on block device inode to go away soon
> > (and thus i_wb reference to get dropped) as the device may got
> > hot-removed e.g. under a mounted filesystem. We deal with these issues
> > by switching block device inode from non-root wb_writeback structure to
> > bdi->wb when needed. Since this is rather expensive (requires
> > synchronize_rcu()) we do the switching only in del_gendisk() when we
> > know the device is going away.
>
> So, the only reason cgwb_bdi_destroy() is synchronous is because bdi
> destruction was synchronous. Now that bdi is properly reference
> counted and can be decoupled from gendisk / q destruction, I can't
> think of a reason to keep cgwb destruction synchronous. Switching
> wb's on destruction is kinda clumsy and it almost always hurts to
> expose synchronize_rcu() in userland visible paths.
>
> Wouldn't something like the following work?
>
> * Remove bdi->usage_cnt and the synchronous waiting in
> cgwb_bdi_destroy().
>
> * Instead, make cgwb's hold bdi->refcnt and put it from
> cgwb_release_workfn().
>
> Then, we don't have to switch during shutdown and can just let things
> drain.
At first sight this looks workable and would mean less special code so I
like it. I'll experiment with it and see how it works out.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 0/5] block subsystem refcounter conversions
From: Peter Zijlstra @ 2017-02-20 16:56 UTC (permalink / raw)
To: James Bottomley
Cc: Jens Axboe, Elena Reshetova, linux-kernel, linux-block,
linux-scsi, linux-btrfs, gregkh, fujita.tomonori, mingo, clm,
jbacik, dsterba
In-Reply-To: <1487605261.2337.4.camel@HansenPartnership.com>
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.
^ permalink raw reply
* [PATCH] scsi: zero per-cmd driver data before each I/O
From: Christoph Hellwig @ 2017-02-20 16:52 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>
---
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;
--
2.11.0
^ permalink raw reply related
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-20 16:32 UTC (permalink / raw)
To: Bart Van Assche, Christoph Hellwig
Cc: Linus Torvalds, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, Mike Snitzer
In-Reply-To: <1D08B61A9CF0974AA09887BE32D889DA0A8CBD@ULS-OP-MBXIP03.sdcorp.global.sandisk.com>
On 02/20/2017 09:16 AM, Bart Van Assche wrote:
> On 02/19/2017 11:35 PM, Christoph Hellwig wrote:
>> On Sun, Feb 19, 2017 at 06:15:41PM -0700, Jens Axboe wrote:
>>> That said, we will look into this again, of course. Christoph, any idea?
>>
>> No idea really - this seems so far away from the code touched, and there
>> are no obvious signs for a memory scamble from another object touched
>> that I think if it really bisects down to that issue it must be a timing
>> issue.
>>
>> But reading Bart's message again: Did you actually bisect it down
>> to the is commit? Or just test the whole tree? Between the 4.10-rc5
>> merge and all the block tree there might a few more likely suspects
>> like the scsi bdi lifetime fixes that James mentioned.
>
> Hello Christoph,
>
> As far as I know Jens does not rebase his trees so we can use the commit
> date to check which patch went in when. From the first of Jan's bdi patches:
>
> CommitDate: Thu Feb 2 08:18:41 2017 -0700
>
> So the bdi patches went in several days after I reported the general protection
> fault issue.
>
> In an e-mail of January 30th I wrote the following: "Running the srp-test
> software against kernel 4.9.6 and kernel 4.10-rc5 went fine. With your
> for-4.11/block branch (commit 400f73b23f457a) however I just ran into
> the following warning: [ ... ]" That means that I did not hit the crash with
> Jens' for-4.11/block branch but only with the for-next branch. The patches
> on Jens' for-next branch after that commit that were applied before I ran
> my test are:
>
> $ PAGER= git log --format=oneline 400f73b23f457a..fb045ca25cc7 block drivers/md/dm{,-mpath,-table}.[ch]
> fb045ca25cc7b6d46368ab8221774489c2a81648 block: don't assign cmd_flags in __blk_rq_prep_clone
> 82ed4db499b8598f16f8871261bff088d6b0597f block: split scsi_request out of struct request
> 8ae94eb65be9425af4d57a4f4cfebfdf03081e93 block/bsg: move queue creation into bsg_setup_queue
> eb8db831be80692bf4bda3dfc55001daf64ec299 dm: always defer request allocation to the owner of the request_queue
> 6d247d7f71d1fa4b66a5f4da7b1daa21510d529b block: allow specifying size for extra command data
> 5ea708d15a928f7a479987704203616d3274c03b block: simplify blk_init_allocated_queue
> e6f7f93d58de74700f83dd0547dd4306248a093d block: fix elevator init check
> f924ba70c1b12706c6679d793202e8f4c125f7ae Merge branch 'for-4.11/block' into for-4.11/rq-refactor
> 88a7503376f4f3bf303c809d1a389739e1205614 blk-mq: Remove unused variable
> bef13315e990fd3d3fb4c39013aefd53f06c3657 block: don't try to discard from __blkdev_issue_zeroout
> f99e86485cc32cd16e5cc97f9bb0474f28608d84 block: Rename blk_queue_zone_size and bdev_zone_size
>
> Do you see any patch in the above list that does not belong to the "split
> scsi passthrough fields out of struct request" series and that could have
> caused the reported behavior change?
Bart, since you are the only one that can reproduce this, can you just bisect
your way through that series?
--
Jens Axboe
^ permalink raw reply
* RE: [GIT PULL] Block pull request for- 4.11-rc1
From: Bart Van Assche @ 2017-02-20 16:16 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Linus Torvalds, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, Mike Snitzer
In-Reply-To: <20170220073539.GA17687@lst.de>
On 02/19/2017 11:35 PM, Christoph Hellwig wrote:=0A=
> On Sun, Feb 19, 2017 at 06:15:41PM -0700, Jens Axboe wrote:=0A=
>> That said, we will look into this again, of course. Christoph, any idea?=
=0A=
> =0A=
> No idea really - this seems so far away from the code touched, and there=
=0A=
> are no obvious signs for a memory scamble from another object touched=0A=
> that I think if it really bisects down to that issue it must be a timing=
=0A=
> issue.=0A=
> =0A=
> But reading Bart's message again: Did you actually bisect it down=0A=
> to the is commit? Or just test the whole tree? Between the 4.10-rc5=0A=
> merge and all the block tree there might a few more likely suspects=0A=
> like the scsi bdi lifetime fixes that James mentioned.=0A=
=0A=
Hello Christoph,=0A=
=0A=
As far as I know Jens does not rebase his trees so we can use the commit=0A=
date to check which patch went in when. From the first of Jan's bdi patches=
:=0A=
=0A=
CommitDate: Thu Feb 2 08:18:41 2017 -0700=0A=
=0A=
So the bdi patches went in several days after I reported the general protec=
tion=0A=
fault issue.=0A=
=0A=
In an e-mail of January 30th I wrote the following: "Running the srp-test=
=0A=
software against kernel 4.9.6 and kernel 4.10-rc5 went fine. With your=0A=
for-4.11/block branch (commit 400f73b23f457a) however I just ran into=0A=
the following warning: [ ... ]" That means that I did not hit the crash wit=
h=0A=
Jens' for-4.11/block branch but only with the for-next branch. The patches=
=0A=
on Jens' for-next branch after that commit that were applied before I ran=
=0A=
my test are:=0A=
=0A=
$ PAGER=3D git log --format=3Doneline 400f73b23f457a..fb045ca25cc7 block dr=
ivers/md/dm{,-mpath,-table}.[ch]=0A=
fb045ca25cc7b6d46368ab8221774489c2a81648 block: don't assign cmd_flags in _=
_blk_rq_prep_clone=0A=
82ed4db499b8598f16f8871261bff088d6b0597f block: split scsi_request out of s=
truct request=0A=
8ae94eb65be9425af4d57a4f4cfebfdf03081e93 block/bsg: move queue creation int=
o bsg_setup_queue=0A=
eb8db831be80692bf4bda3dfc55001daf64ec299 dm: always defer request allocatio=
n to the owner of the request_queue=0A=
6d247d7f71d1fa4b66a5f4da7b1daa21510d529b block: allow specifying size for e=
xtra command data=0A=
5ea708d15a928f7a479987704203616d3274c03b block: simplify blk_init_allocated=
_queue=0A=
e6f7f93d58de74700f83dd0547dd4306248a093d block: fix elevator init check=0A=
f924ba70c1b12706c6679d793202e8f4c125f7ae Merge branch 'for-4.11/block' into=
for-4.11/rq-refactor=0A=
88a7503376f4f3bf303c809d1a389739e1205614 blk-mq: Remove unused variable=0A=
bef13315e990fd3d3fb4c39013aefd53f06c3657 block: don't try to discard from _=
_blkdev_issue_zeroout=0A=
f99e86485cc32cd16e5cc97f9bb0474f28608d84 block: Rename blk_queue_zone_size =
and bdev_zone_size=0A=
=0A=
Do you see any patch in the above list that does not belong to the "split=
=0A=
scsi passthrough fields out of struct request" series and that could have=
=0A=
caused the reported behavior change?=0A=
=0A=
Thanks,=0A=
=0A=
Bart.=
^ permalink raw reply
* Re: [PATCH 0/5] block subsystem refcounter conversions
From: Jens Axboe @ 2017-02-20 15:44 UTC (permalink / raw)
To: James Bottomley, Elena Reshetova, linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh,
fujita.tomonori, mingo, clm, jbacik, dsterba
In-Reply-To: <1487605261.2337.4.camel@HansenPartnership.com>
On 02/20/2017 08:41 AM, 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 below patches are fully independent and can be cherry-picked
>>> separately. Since we convert all kernel subsystems in the same
>>> fashion, resulting in about 300 patches, we have to group them for
>>> sending at least in some fashion to be manageable. Please excuse
>>> the long cc list.
>>>
>>> Elena Reshetova (5):
>>> block: convert bio.__bi_cnt from atomic_t to refcount_t
>>> block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
>>> block: convert blkcg_gq.refcnt from atomic_t to refcount_t
>>> block: convert io_context.active_ref from atomic_t to refcount_t
>>> block: convert bsg_device.ref_count from atomic_t to refcount_t
>>
>> I went to look at the implementation, and the size of a refcount_t.
>> But the code is not there?! You say it's finally merged, where is
>> it merged? Don't send code like this without the necessary
>> infrastructure being in mainline.
>
> It's one of those no discussion except notice by tipbot things because
> Ingo liked it.
>
> The size is atomic_t, but the primitives check for overflow and check
> inc from zero and things, so in a true refcounting situation we get
> automatic warnings of problems inside the primitives.
>
> That said, if we were going to convert the block layer to this
> semantic, surely the benefit of the conversion would be getting rid of
> the now unnecessary BUG_ON and WARN_ON in the code, which do exactly
> the same thing the refcount primitives now do?
Well, I have no idea what it does, which is why I went to look at the
code. So any talk of converting the block layer is premature. But it's
not there. I'll defer judgment until we have something in mainline,
until then I've archived this thread.
And I agree, keeping warn/bug for cases that should be handled
with this framework would be counter productive and pointless.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 0/5] block subsystem refcounter conversions
From: James Bottomley @ 2017-02-20 15:41 UTC (permalink / raw)
To: Jens Axboe, Elena Reshetova, linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh,
fujita.tomonori, mingo, clm, jbacik, dsterba
In-Reply-To: <5fd906c2-61ba-319c-1bc2-cb48b2b378b4@kernel.dk>
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 below patches are fully independent and can be cherry-picked
> > separately. Since we convert all kernel subsystems in the same
> > fashion, resulting in about 300 patches, we have to group them for
> > sending at least in some fashion to be manageable. Please excuse
> > the long cc list.
> >
> > Elena Reshetova (5):
> > block: convert bio.__bi_cnt from atomic_t to refcount_t
> > block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
> > block: convert blkcg_gq.refcnt from atomic_t to refcount_t
> > block: convert io_context.active_ref from atomic_t to refcount_t
> > block: convert bsg_device.ref_count from atomic_t to refcount_t
>
> I went to look at the implementation, and the size of a refcount_t.
> But the code is not there?! You say it's finally merged, where is
> it merged? Don't send code like this without the necessary
> infrastructure being in mainline.
It's one of those no discussion except notice by tipbot things because
Ingo liked it.
The size is atomic_t, but the primitives check for overflow and check
inc from zero and things, so in a true refcounting situation we get
automatic warnings of problems inside the primitives.
That said, if we were going to convert the block layer to this
semantic, surely the benefit of the conversion would be getting rid of
the now unnecessary BUG_ON and WARN_ON in the code, which do exactly
the same thing the refcount primitives now do?
James
^ permalink raw reply
* Re: Some throughput tests with MQ and BFQ on MMC/SD
From: Adrian Hunter @ 2017-02-20 15:32 UTC (permalink / raw)
To: Linus Walleij
Cc: Ziji Hu, linux-mmc@vger.kernel.org, linux-block, Ulf Hansson,
Ritesh Harjani, Avri Altman, Arnd Bergmann, Christoph Hellwig,
Jens Axboe, Paolo Valente, Per Förlin
In-Reply-To: <CACRpkdZADpiD8KNUqyhqdpf4HoVJQx3Z2_5dkw6BG2MG-5r8Jg@mail.gmail.com>
On 20/02/17 15:46, Linus Walleij wrote:
> On Mon, Feb 20, 2017 at 9:03 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
>
>> MQ is not better - it is just different.
>
> Well it is better in the sense that it has active maintainers and is
> not scheduled
> for depreciation.
>
>> Because mmc devices do not have
>> multiple hardware queues, blk-mq essentially offers nothing but a different
>> way of doing the same thing.
>
> I think what Ziji is pointing out is the hourglass-shaped structure of MQ.
> It has multiple *issue* queues as well, not just multiple *hardware*
> queues. That means that processes can have issue queues on different
> CPUs and not all requests end up in a single nexus like with the old blk
> layer.
blk-mq has a lighter touch, but if you use BLK_MQ_F_BLOCKING and only have
one hardware context, then you are still putting everything through a single
work item.
>
> Whether it benefits MMC/SD in the end is a good question. It might,
> testing on multicores with multiple issue threads is needed.
>
>> It would be better if blk-mq support was experimental until we can see how
>> well it works in practice.
>
> Do you mean experimental in the MMC/SD stack, such that we should
> merge it as an additional scheduler instead of as the only scheduler
> replacement?
Not sure what you mean by "scheduler".
>
> I think SCSI did/still does things like that.
A quick look at SCSI shows a module parameter use_blk_mq that defaults based
on a config option CONFIG_SCSI_MQ_DEFAULT i.e. defaults to false unless
selected.
Yes, something like that.
> On the other hand, UBI
> just replaced the old block layer with MQ in commit
> ff1f48ee3bb3, and it is also very widely
> used, so there are example of both approaches. (How typical.)
UBI block is read-only and does not benefit from an I/O scheduler, and has
nothing like a command queue. It doesn't seem unreasonable that very
different kinds of devices would take different approaches.
Unfortunately, ff1f48ee3bb3 mixes together switching the underlying I/O to
using a scatter gather list and removing a serializing mutex, so it is hard
to tell if improvements came from that or from blk-mq.
^ permalink raw reply
* Re: [PATCH 0/5] block subsystem refcounter conversions
From: Jens Axboe @ 2017-02-20 15:15 UTC (permalink / raw)
To: Elena Reshetova, linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh,
fujita.tomonori, mingo, clm, jbacik, dsterba
In-Reply-To: <1487589368-17666-1-git-send-email-elena.reshetova@intel.com>
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.
>
> The below patches are fully independent and can be cherry-picked separately.
> Since we convert all kernel subsystems in the same fashion, resulting
> in about 300 patches, we have to group them for sending at least in some
> fashion to be manageable. Please excuse the long cc list.
>
> Elena Reshetova (5):
> block: convert bio.__bi_cnt from atomic_t to refcount_t
> block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
> block: convert blkcg_gq.refcnt from atomic_t to refcount_t
> block: convert io_context.active_ref from atomic_t to refcount_t
> block: convert bsg_device.ref_count from atomic_t to refcount_t
I went to look at the implementation, and the size of a refcount_t.
But the code is not there?! You say it's finally merged, where is
it merged? Don't send code like this without the necessary
infrastructure being in mainline.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 01/10] block: Move bdev_unhash_inode() after invalidate_partition()
From: Jan Kara @ 2017-02-20 14:53 UTC (permalink / raw)
To: Tejun Heo
Cc: Jan Kara, Jens Axboe, linux-block, Christoph Hellwig,
Dan Williams, Thiago Jung Bauermann, NeilBrown
In-Reply-To: <20170212035833.GA29323@mtj.duckdns.org>
On Sun 12-02-17 12:58:33, Tejun Heo wrote:
> On Thu, Feb 09, 2017 at 01:44:24PM +0100, Jan Kara wrote:
> > Move bdev_unhash_inode() after invalidate_partition() as
> > invalidate_partition() looks up bdev and will unnecessarily recreate it
> > if bdev_unhash_inode() destroyed it. Also use part_devt() when calling
> > bdev_unhash_inode() instead of manually creating the device number.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
>
> Acked-by: Tejun Heo <tj@kernel.org>
>
> > @@ -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);
>
> So, before this patch, invalidate_partition() would have operated on a
> newly created inode and thus wouldn't have actually invalidated the
> existing bdev / mapping, right?
Yes, that is another effect. I'll add a note about this to the changelog.
Thanks for noting this.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: Some throughput tests with MQ and BFQ on MMC/SD
From: Linus Walleij @ 2017-02-20 13:46 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ziji Hu, linux-mmc@vger.kernel.org, linux-block, Ulf Hansson,
Ritesh Harjani, Avri Altman, Arnd Bergmann, Christoph Hellwig,
Jens Axboe, Paolo Valente, Per Förlin
In-Reply-To: <2995d066-ebee-c05c-a4cc-63d5b86df0be@intel.com>
On Mon, Feb 20, 2017 at 9:03 AM, Adrian Hunter <adrian.hunter@intel.com> wrote:
> MQ is not better - it is just different.
Well it is better in the sense that it has active maintainers and is
not scheduled
for depreciation.
> Because mmc devices do not have
> multiple hardware queues, blk-mq essentially offers nothing but a different
> way of doing the same thing.
I think what Ziji is pointing out is the hourglass-shaped structure of MQ.
It has multiple *issue* queues as well, not just multiple *hardware*
queues. That means that processes can have issue queues on different
CPUs and not all requests end up in a single nexus like with the old blk
layer.
Whether it benefits MMC/SD in the end is a good question. It might,
testing on multicores with multiple issue threads is needed.
> It would be better if blk-mq support was experimental until we can see how
> well it works in practice.
Do you mean experimental in the MMC/SD stack, such that we should
merge it as an additional scheduler instead of as the only scheduler
replacement?
I think SCSI did/still does things like that. On the other hand, UBI
just replaced the old block layer with MQ in commit
ff1f48ee3bb3, and it is also very widely
used, so there are example of both approaches. (How typical.)
Yours,
Linus Walleij
^ permalink raw reply
* Re: Some throughput tests with MQ and BFQ on MMC/SD
From: Adrian Hunter @ 2017-02-20 11:19 UTC (permalink / raw)
To: Ziji Hu, Linus Walleij
Cc: linux-mmc@vger.kernel.org, linux-block, Ulf Hansson,
Ritesh Harjani, Avri Altman, Arnd Bergmann, Christoph Hellwig,
Jens Axboe, Paolo Valente, Per Förlin
In-Reply-To: <02cc3bc1-3ca6-2561-c914-a7993403c424@marvell.com>
On 20/02/17 13:04, Ziji Hu wrote:
> Hi Adrian,
>
> On 2017/2/20 16:03, Adrian Hunter wrote:
>> On 17/02/17 15:22, Linus Walleij wrote:
>>> On Fri, Feb 17, 2017 at 12:53 PM, Ziji Hu <huziji@marvell.com> wrote:
> <snip>
>>> Ulf describes it: we want to switch MMC/SD to MQ.
>>>
>>> To me, there are two reasons for that (no secret agendas...)
>>>
>>> 1. To get away from the legacy codebase in the old block layer.
>>> Christoph and Jens have been very clear stating that the old block
>>> layer is in maintenance mode and should be phased out, and they
>>> asked explicitly for out help to do so. Currently
>>> MMC/SD is a big fat roadblock to these plans so it is win-win for
>>> MMC/SD and the block layer if we can just switch over to MQ.
>>>
>>> 2. My colleague Paolo Valente is working on the next generation
>>> block scheduler BFQ which has very promising potential for
>>> interactive loads. (Like taking a backup of your harddrive while
>>> playing 1080p video let's say.) Since the
>>> old block layer is no longer maintained, this scheduler will only
>>> be merged and made available for systems deploying MQ. He's
>>> already working full steam on that.
>>>
>>> I would like to make 1+2 happen in the next merge window
>>> ultimately, but yeah, maybe I'm overly optimistic. But I will sure
>>> try.
>>>
>>> Maybe I should add:
>>>
>>> 3. MQ is a better and future-proof fit for command queueing.
>>
>> MQ is not better - it is just different. Because mmc devices do not have
>> multiple hardware queues, blk-mq essentially offers nothing but a different
>> way of doing the same thing. And there are problems, such as blk-mq assumes
>> that the primary arbiter of whether a request can be issued is the queue
>> depth. As I wrote here:
>> https://marc.info/?l=linux-mmc&m=148336571720463&w=2
>> that is not the case for mmc, even with command queuing.
>>
> I guess it might benefit CMDQ since multi-thread can more easily let
> CMDQ achieve full performance. It is just my guess. We shall wait for
> the test result.
>
> Based on my own experience on developing CMDQ driver, some of the issues, like different
> partition and ioctl, can be solved by getting and putting mmc_host (mmc_claim_host/mmc_release_host).
> The key issue is Direct-CMD, It will be more complex to handle Direct-CMD with blk-mq,
> than doing it with current block layer. With current block layer, I just let CMDQ driver
> borrow existing non-CMDQ transfer routine, and turn back to CMDQ transfer routine
> after DCMD completes. But I'm not the effort we need with blk-mq.
>
> My key point is, please correct me if I'm wrong, shall we avoid binding CMDQ and
> non-CMDQ card layer together?
> To be honest, I don't think CMDQ is a good design. It will bring a lot of troubles.
> In my very own opinion, if we try to develop a single routine to support both CMDQ
> and non-CMDQ, it will be painful to both sides.
Not sure what you mean, but the patches linked below support DCMD and
non-DCMD for flush, and the regular approach for discards:
https://marc.info/?l=linux-mmc&m=148673270920906
CQE can be used for reads/writes only, or reads/writes/flushes. Everything
else is done the regular way.
>
> Thank you.
>
> Best regards,
> Hu Ziji
>
>> Also I wouldn't be surprise if BFQ needs some changes to work well with
>> command queuing.
>>
>> It would be better if blk-mq support was experimental until we can see how
>> well it works in practice.
>>
>
^ permalink raw reply
* [PATCH 5/5] block: convert bsg_device.ref_count from atomic_t to refcount_t
From: Elena Reshetova @ 2017-02-20 11:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh, axboe,
fujita.tomonori, mingo, clm, jbacik, dsterba, Elena Reshetova,
Hans Liljestrand, Kees Cook, David Windsor
In-Reply-To: <1487589368-17666-1-git-send-email-elena.reshetova@intel.com>
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
block/bsg.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/bsg.c b/block/bsg.c
index 74835db..6d0ceb9 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -21,6 +21,7 @@
#include <linux/idr.h>
#include <linux/bsg.h>
#include <linux/slab.h>
+#include <linux/refcount.h>
#include <scsi/scsi.h>
#include <scsi/scsi_ioctl.h>
@@ -38,7 +39,7 @@ struct bsg_device {
struct list_head busy_list;
struct list_head done_list;
struct hlist_node dev_list;
- atomic_t ref_count;
+ refcount_t ref_count;
int queued_cmds;
int done_cmds;
wait_queue_head_t wq_done;
@@ -711,7 +712,7 @@ static int bsg_put_device(struct bsg_device *bd)
mutex_lock(&bsg_mutex);
- do_free = atomic_dec_and_test(&bd->ref_count);
+ do_free = refcount_dec_and_test(&bd->ref_count);
if (!do_free) {
mutex_unlock(&bsg_mutex);
goto out;
@@ -763,7 +764,7 @@ static struct bsg_device *bsg_add_device(struct inode *inode,
bsg_set_block(bd, file);
- atomic_set(&bd->ref_count, 1);
+ refcount_set(&bd->ref_count, 1);
mutex_lock(&bsg_mutex);
hlist_add_head(&bd->dev_list, bsg_dev_idx_hash(iminor(inode)));
@@ -783,7 +784,7 @@ static struct bsg_device *__bsg_get_device(int minor, struct request_queue *q)
hlist_for_each_entry(bd, bsg_dev_idx_hash(minor), dev_list) {
if (bd->queue == q) {
- atomic_inc(&bd->ref_count);
+ refcount_inc(&bd->ref_count);
goto found;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH 4/5] block: convert io_context.active_ref from atomic_t to refcount_t
From: Elena Reshetova @ 2017-02-20 11:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh, axboe,
fujita.tomonori, mingo, clm, jbacik, dsterba, Elena Reshetova,
Hans Liljestrand, Kees Cook, David Windsor
In-Reply-To: <1487589368-17666-1-git-send-email-elena.reshetova@intel.com>
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
block/blk-ioc.c | 4 ++--
block/cfq-iosched.c | 4 ++--
include/linux/iocontext.h | 7 ++++---
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index b12f9c8..130dc23 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -173,7 +173,7 @@ void put_io_context_active(struct io_context *ioc)
unsigned long flags;
struct io_cq *icq;
- if (!atomic_dec_and_test(&ioc->active_ref)) {
+ if (!refcount_dec_and_test(&ioc->active_ref)) {
put_io_context(ioc);
return;
}
@@ -256,7 +256,7 @@ int create_task_io_context(struct task_struct *task, gfp_t gfp_flags, int node)
/* initialize */
atomic_long_set(&ioc->refcount, 1);
atomic_set(&ioc->nr_tasks, 1);
- atomic_set(&ioc->active_ref, 1);
+ refcount_set(&ioc->active_ref, 1);
spin_lock_init(&ioc->lock);
INIT_RADIX_TREE(&ioc->icq_tree, GFP_ATOMIC | __GFP_HIGH);
INIT_HLIST_HEAD(&ioc->icq_list);
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 9212627..2871bb9 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -2959,7 +2959,7 @@ static void cfq_arm_slice_timer(struct cfq_data *cfqd)
* task has exited, don't wait
*/
cic = cfqd->active_cic;
- if (!cic || !atomic_read(&cic->icq.ioc->active_ref))
+ if (!cic || !refcount_read(&cic->icq.ioc->active_ref))
return;
/*
@@ -3959,7 +3959,7 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq,
if (cfqq->next_rq && req_noidle(cfqq->next_rq))
enable_idle = 0;
- else if (!atomic_read(&cic->icq.ioc->active_ref) ||
+ else if (!refcount_read(&cic->icq.ioc->active_ref) ||
!cfqd->cfq_slice_idle ||
(!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq)))
enable_idle = 0;
diff --git a/include/linux/iocontext.h b/include/linux/iocontext.h
index df38db2..a1e28c3 100644
--- a/include/linux/iocontext.h
+++ b/include/linux/iocontext.h
@@ -3,6 +3,7 @@
#include <linux/radix-tree.h>
#include <linux/rcupdate.h>
+#include <linux/refcount.h>
#include <linux/workqueue.h>
enum {
@@ -96,7 +97,7 @@ struct io_cq {
*/
struct io_context {
atomic_long_t refcount;
- atomic_t active_ref;
+ refcount_t active_ref;
atomic_t nr_tasks;
/* all the fields below are protected by this lock */
@@ -128,9 +129,9 @@ struct io_context {
static inline void get_io_context_active(struct io_context *ioc)
{
WARN_ON_ONCE(atomic_long_read(&ioc->refcount) <= 0);
- WARN_ON_ONCE(atomic_read(&ioc->active_ref) <= 0);
+ WARN_ON_ONCE(refcount_read(&ioc->active_ref) == 0);
atomic_long_inc(&ioc->refcount);
- atomic_inc(&ioc->active_ref);
+ refcount_inc(&ioc->active_ref);
}
static inline void ioc_task_link(struct io_context *ioc)
--
2.7.4
^ permalink raw reply related
* [PATCH 3/5] block: convert blkcg_gq.refcnt from atomic_t to refcount_t
From: Elena Reshetova @ 2017-02-20 11:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh, axboe,
fujita.tomonori, mingo, clm, jbacik, dsterba, Elena Reshetova,
Hans Liljestrand, Kees Cook, David Windsor
In-Reply-To: <1487589368-17666-1-git-send-email-elena.reshetova@intel.com>
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
block/blk-cgroup.c | 2 +-
include/linux/blk-cgroup.h | 11 ++++++-----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 295e98c2..75de844 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -106,7 +106,7 @@ static struct blkcg_gq *blkg_alloc(struct blkcg *blkcg, struct request_queue *q,
blkg->q = q;
INIT_LIST_HEAD(&blkg->q_node);
blkg->blkcg = blkcg;
- atomic_set(&blkg->refcnt, 1);
+ refcount_set(&blkg->refcnt, 1);
/* root blkg uses @q->root_rl, init rl only for !root blkgs */
if (blkcg != &blkcg_root) {
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 01b62e7..0d3efa9 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -19,6 +19,7 @@
#include <linux/radix-tree.h>
#include <linux/blkdev.h>
#include <linux/atomic.h>
+#include <linux/refcount.h>
/* percpu_counter batch for blkg_[rw]stats, per-cpu drift doesn't matter */
#define BLKG_STAT_CPU_BATCH (INT_MAX / 2)
@@ -122,7 +123,7 @@ struct blkcg_gq {
struct request_list rl;
/* reference count */
- atomic_t refcnt;
+ refcount_t refcnt;
/* is this blkg online? protected by both blkcg and q locks */
bool online;
@@ -354,8 +355,8 @@ static inline int blkg_path(struct blkcg_gq *blkg, char *buf, int buflen)
*/
static inline void blkg_get(struct blkcg_gq *blkg)
{
- WARN_ON_ONCE(atomic_read(&blkg->refcnt) <= 0);
- atomic_inc(&blkg->refcnt);
+ WARN_ON_ONCE(refcount_read(&blkg->refcnt) == 0);
+ refcount_inc(&blkg->refcnt);
}
void __blkg_release_rcu(struct rcu_head *rcu);
@@ -366,8 +367,8 @@ void __blkg_release_rcu(struct rcu_head *rcu);
*/
static inline void blkg_put(struct blkcg_gq *blkg)
{
- WARN_ON_ONCE(atomic_read(&blkg->refcnt) <= 0);
- if (atomic_dec_and_test(&blkg->refcnt))
+ WARN_ON_ONCE(refcount_read(&blkg->refcnt) == 0);
+ if (refcount_dec_and_test(&blkg->refcnt))
call_rcu(&blkg->rcu_head, __blkg_release_rcu);
}
--
2.7.4
^ permalink raw reply related
* [PATCH 2/5] block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
From: Elena Reshetova @ 2017-02-20 11:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh, axboe,
fujita.tomonori, mingo, clm, jbacik, dsterba, Elena Reshetova,
Hans Liljestrand, Kees Cook, David Windsor
In-Reply-To: <1487589368-17666-1-git-send-email-elena.reshetova@intel.com>
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
block/blk-tag.c | 8 ++++----
include/linux/blkdev.h | 3 ++-
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/block/blk-tag.c b/block/blk-tag.c
index 07cc329..d83555e 100644
--- a/block/blk-tag.c
+++ b/block/blk-tag.c
@@ -35,7 +35,7 @@ EXPORT_SYMBOL(blk_queue_find_tag);
*/
void blk_free_tags(struct blk_queue_tag *bqt)
{
- if (atomic_dec_and_test(&bqt->refcnt)) {
+ if (refcount_dec_and_test(&bqt->refcnt)) {
BUG_ON(find_first_bit(bqt->tag_map, bqt->max_depth) <
bqt->max_depth);
@@ -130,7 +130,7 @@ static struct blk_queue_tag *__blk_queue_init_tags(struct request_queue *q,
if (init_tag_map(q, tags, depth))
goto fail;
- atomic_set(&tags->refcnt, 1);
+ refcount_set(&tags->refcnt, 1);
tags->alloc_policy = alloc_policy;
tags->next_tag = 0;
return tags;
@@ -180,7 +180,7 @@ int blk_queue_init_tags(struct request_queue *q, int depth,
queue_flag_set(QUEUE_FLAG_QUEUED, q);
return 0;
} else
- atomic_inc(&tags->refcnt);
+ refcount_inc(&tags->refcnt);
/*
* assign it, all done
@@ -225,7 +225,7 @@ int blk_queue_resize_tags(struct request_queue *q, int new_depth)
* Currently cannot replace a shared tag map with a new
* one, so error out if this is the case
*/
- if (atomic_read(&bqt->refcnt) != 1)
+ if (refcount_read(&bqt->refcnt) != 1)
return -EBUSY;
/*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aecca0e..95ef11c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -25,6 +25,7 @@
#include <linux/percpu-refcount.h>
#include <linux/scatterlist.h>
#include <linux/blkzoned.h>
+#include <linux/refcount.h>
struct module;
struct scsi_ioctl_command;
@@ -288,7 +289,7 @@ struct blk_queue_tag {
unsigned long *tag_map; /* bit map of free/busy tags */
int max_depth; /* what we will send to device */
int real_max_depth; /* what the array can hold */
- atomic_t refcnt; /* map can be shared */
+ refcount_t refcnt; /* map can be shared */
int alloc_policy; /* tag allocation policy */
int next_tag; /* next tag */
};
--
2.7.4
^ permalink raw reply related
* [PATCH 1/5] block: convert bio.__bi_cnt from atomic_t to refcount_t
From: Elena Reshetova @ 2017-02-20 11:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh, axboe,
fujita.tomonori, mingo, clm, jbacik, dsterba, Elena Reshetova,
Hans Liljestrand, Kees Cook, David Windsor
In-Reply-To: <1487589368-17666-1-git-send-email-elena.reshetova@intel.com>
refcount_t type and corresponding API should be
used instead of atomic_t when the variable is used as
a reference counter. This allows to avoid accidental
refcounter overflows that might lead to use-after-free
situations.
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
Signed-off-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: David Windsor <dwindsor@gmail.com>
---
block/bio.c | 6 +++---
fs/btrfs/volumes.c | 2 +-
include/linux/bio.h | 4 ++--
include/linux/blk_types.h | 3 ++-
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 5eec5e0..3dffc17 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -275,7 +275,7 @@ void bio_init(struct bio *bio, struct bio_vec *table,
{
memset(bio, 0, sizeof(*bio));
atomic_set(&bio->__bi_remaining, 1);
- atomic_set(&bio->__bi_cnt, 1);
+ refcount_set(&bio->__bi_cnt, 1);
bio->bi_io_vec = table;
bio->bi_max_vecs = max_vecs;
@@ -543,12 +543,12 @@ void bio_put(struct bio *bio)
if (!bio_flagged(bio, BIO_REFFED))
bio_free(bio);
else {
- BIO_BUG_ON(!atomic_read(&bio->__bi_cnt));
+ BIO_BUG_ON(!refcount_read(&bio->__bi_cnt));
/*
* last put frees it
*/
- if (atomic_dec_and_test(&bio->__bi_cnt))
+ if (refcount_dec_and_test(&bio->__bi_cnt))
bio_free(bio);
}
}
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 13e55d1..ff1fe9d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -441,7 +441,7 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
waitqueue_active(&fs_info->async_submit_wait))
wake_up(&fs_info->async_submit_wait);
- BUG_ON(atomic_read(&cur->__bi_cnt) == 0);
+ BUG_ON(refcount_read(&cur->__bi_cnt) == 0);
/*
* if we're doing the sync list, record that our
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 8e52119..44ac678 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -234,7 +234,7 @@ static inline void bio_get(struct bio *bio)
{
bio->bi_flags |= (1 << BIO_REFFED);
smp_mb__before_atomic();
- atomic_inc(&bio->__bi_cnt);
+ refcount_inc(&bio->__bi_cnt);
}
static inline void bio_cnt_set(struct bio *bio, unsigned int count)
@@ -243,7 +243,7 @@ static inline void bio_cnt_set(struct bio *bio, unsigned int count)
bio->bi_flags |= (1 << BIO_REFFED);
smp_mb__before_atomic();
}
- atomic_set(&bio->__bi_cnt, count);
+ refcount_set(&bio->__bi_cnt, count);
}
static inline bool bio_flagged(struct bio *bio, unsigned int bit)
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index d703acb..c41407f 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -7,6 +7,7 @@
#include <linux/types.h>
#include <linux/bvec.h>
+#include <linux/refcount.h>
struct bio_set;
struct bio;
@@ -73,7 +74,7 @@ struct bio {
unsigned short bi_max_vecs; /* max bvl_vecs we can hold */
- atomic_t __bi_cnt; /* pin count */
+ refcount_t __bi_cnt; /* pin count */
struct bio_vec *bi_io_vec; /* the actual vec list */
--
2.7.4
^ permalink raw reply related
* [PATCH 0/5] block subsystem refcounter conversions
From: Elena Reshetova @ 2017-02-20 11:16 UTC (permalink / raw)
To: linux-kernel
Cc: linux-block, linux-scsi, linux-btrfs, peterz, gregkh, axboe,
fujita.tomonori, mingo, clm, jbacik, dsterba, Elena Reshetova
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.
The below patches are fully independent and can be cherry-picked separately.
Since we convert all kernel subsystems in the same fashion, resulting
in about 300 patches, we have to group them for sending at least in some
fashion to be manageable. Please excuse the long cc list.
Elena Reshetova (5):
block: convert bio.__bi_cnt from atomic_t to refcount_t
block: convert blk_queue_tag.refcnt from atomic_t to refcount_t
block: convert blkcg_gq.refcnt from atomic_t to refcount_t
block: convert io_context.active_ref from atomic_t to refcount_t
block: convert bsg_device.ref_count from atomic_t to refcount_t
block/bio.c | 6 +++---
block/blk-cgroup.c | 2 +-
block/blk-ioc.c | 4 ++--
block/blk-tag.c | 8 ++++----
block/bsg.c | 9 +++++----
block/cfq-iosched.c | 4 ++--
fs/btrfs/volumes.c | 2 +-
include/linux/bio.h | 4 ++--
include/linux/blk-cgroup.h | 11 ++++++-----
include/linux/blk_types.h | 3 ++-
include/linux/blkdev.h | 3 ++-
include/linux/iocontext.h | 7 ++++---
12 files changed, 34 insertions(+), 29 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH] lightnvm: Fix error handling
From: Matias Bjørling @ 2017-02-20 11:13 UTC (permalink / raw)
To: Christophe JAILLET; +Cc: linux-block, linux-kernel, kernel-janitors
In-Reply-To: <20170219140928.12968-1-christophe.jaillet@wanadoo.fr>
On 02/19/2017 03:09 PM, Christophe JAILLET wrote:
> According to error handling in this function, it is likely that going to
> 'out' was expected here.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> drivers/lightnvm/rrpc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/lightnvm/rrpc.c b/drivers/lightnvm/rrpc.c
> index e00b1d7b976f..e68efbcf1188 100644
> --- a/drivers/lightnvm/rrpc.c
> +++ b/drivers/lightnvm/rrpc.c
> @@ -1275,8 +1275,10 @@ static int rrpc_bb_discovery(struct nvm_tgt_dev *dev, struct rrpc_lun *rlun)
> }
>
> nr_blks = nvm_bb_tbl_fold(dev->parent, blks, nr_blks);
> - if (nr_blks < 0)
> - return nr_blks;
> + if (nr_blks < 0) {
> + ret = nr_blks;
> + goto out;
> + }
>
> for (i = 0; i < nr_blks; i++) {
> if (blks[i] == NVM_BLK_T_FREE)
>
Thanks Christophe. Applied for 4.12.
^ permalink raw reply
* Re: Some throughput tests with MQ and BFQ on MMC/SD
From: Ziji Hu @ 2017-02-20 11:04 UTC (permalink / raw)
To: Adrian Hunter, Linus Walleij
Cc: linux-mmc@vger.kernel.org, linux-block, Ulf Hansson,
Ritesh Harjani, Avri Altman, Arnd Bergmann, Christoph Hellwig,
Jens Axboe, Paolo Valente, Per Förlin
In-Reply-To: <2995d066-ebee-c05c-a4cc-63d5b86df0be@intel.com>
Hi Adrian,
On 2017/2/20 16:03, Adrian Hunter wrote:
> On 17/02/17 15:22, Linus Walleij wrote:
>> On Fri, Feb 17, 2017 at 12:53 PM, Ziji Hu <huziji@marvell.com> wrote:
<snip>
>> Ulf describes it: we want to switch MMC/SD to MQ.
>>
>> To me, there are two reasons for that (no secret agendas...)
>>
>> 1. To get away from the legacy codebase in the old block layer.
>> Christoph and Jens have been very clear stating that the old block
>> layer is in maintenance mode and should be phased out, and they
>> asked explicitly for out help to do so. Currently
>> MMC/SD is a big fat roadblock to these plans so it is win-win for
>> MMC/SD and the block layer if we can just switch over to MQ.
>>
>> 2. My colleague Paolo Valente is working on the next generation
>> block scheduler BFQ which has very promising potential for
>> interactive loads. (Like taking a backup of your harddrive while
>> playing 1080p video let's say.) Since the
>> old block layer is no longer maintained, this scheduler will only
>> be merged and made available for systems deploying MQ. He's
>> already working full steam on that.
>>
>> I would like to make 1+2 happen in the next merge window
>> ultimately, but yeah, maybe I'm overly optimistic. But I will sure
>> try.
>>
>> Maybe I should add:
>>
>> 3. MQ is a better and future-proof fit for command queueing.
>
> MQ is not better - it is just different. Because mmc devices do not have
> multiple hardware queues, blk-mq essentially offers nothing but a different
> way of doing the same thing. And there are problems, such as blk-mq assumes
> that the primary arbiter of whether a request can be issued is the queue
> depth. As I wrote here:
> https://marc.info/?l=linux-mmc&m=148336571720463&w=2
> that is not the case for mmc, even with command queuing.
>
I guess it might benefit CMDQ since multi-thread can more easily let
CMDQ achieve full performance. It is just my guess. We shall wait for
the test result.
Based on my own experience on developing CMDQ driver, some of the issues, like different
partition and ioctl, can be solved by getting and putting mmc_host (mmc_claim_host/mmc_release_host).
The key issue is Direct-CMD, It will be more complex to handle Direct-CMD with blk-mq,
than doing it with current block layer. With current block layer, I just let CMDQ driver
borrow existing non-CMDQ transfer routine, and turn back to CMDQ transfer routine
after DCMD completes. But I'm not the effort we need with blk-mq.
My key point is, please correct me if I'm wrong, shall we avoid binding CMDQ and
non-CMDQ card layer together?
To be honest, I don't think CMDQ is a good design. It will bring a lot of troubles.
In my very own opinion, if we try to develop a single routine to support both CMDQ
and non-CMDQ, it will be painful to both sides.
Thank you.
Best regards,
Hu Ziji
> Also I wouldn't be surprise if BFQ needs some changes to work well with
> command queuing.
>
> It would be better if blk-mq support was experimental until we can see how
> well it works in practice.
>
^ permalink raw reply
* Re: Some throughput tests with MQ and BFQ on MMC/SD
From: Adrian Hunter @ 2017-02-20 8:03 UTC (permalink / raw)
To: Linus Walleij, Ziji Hu
Cc: linux-mmc@vger.kernel.org, linux-block, Ulf Hansson,
Ritesh Harjani, Avri Altman, Arnd Bergmann, Christoph Hellwig,
Jens Axboe, Paolo Valente, Per Förlin
In-Reply-To: <CACRpkda+p4erE9CdMUWi55-Ji8rj8TNOeJadHeOkHT7a1wbLmA@mail.gmail.com>
On 17/02/17 15:22, Linus Walleij wrote:
> On Fri, Feb 17, 2017 at 12:53 PM, Ziji Hu <huziji@marvell.com> wrote:
>
>> I would like to suggest that you should try the multiple thread
>> test mode of iozone, since you are testing *Multi* Queue.
>
> Good point. This target has only 2 CPUs but still, maybe it performs!
>
>> Besides, it seems that your eMMC transfer speed is quite low.
>> It is normal that read speed can reach more than 100MB/s in HS400.
>> Could you try a higher speed mode? The test result might be
>> limited by the bus clock frequency.
>
> The iozone tests are done on an SDcard. And I only did read tests on
> the eMMC I have.
>
> It's because I'm afriad of wearing out my eMMC :(
>
> But OK I'll just take the risk and run iozone on the eMMC.
>
>> Actually I have been following your thread for some time.
>> But currently I'm a little confused.
>> May I know the purpose of your patch?
>
> Ulf describes it: we want to switch MMC/SD to MQ.
>
> To me, there are two reasons for that (no secret agendas...)
>
> 1. To get away from the legacy codebase in the old block layer.
> Christoph and Jens have been very clear stating that the old block
> layer is in maintenance mode and should be phased out, and they
> asked explicitly for out help to do so. Currently
> MMC/SD is a big fat roadblock to these plans so it is win-win for
> MMC/SD and the block layer if we can just switch over to MQ.
>
> 2. My colleague Paolo Valente is working on the next generation
> block scheduler BFQ which has very promising potential for
> interactive loads. (Like taking a backup of your harddrive while
> playing 1080p video let's say.) Since the
> old block layer is no longer maintained, this scheduler will only
> be merged and made available for systems deploying MQ. He's
> already working full steam on that.
>
> I would like to make 1+2 happen in the next merge window
> ultimately, but yeah, maybe I'm overly optimistic. But I will sure
> try.
>
> Maybe I should add:
>
> 3. MQ is a better and future-proof fit for command queueing.
MQ is not better - it is just different. Because mmc devices do not have
multiple hardware queues, blk-mq essentially offers nothing but a different
way of doing the same thing. And there are problems, such as blk-mq assumes
that the primary arbiter of whether a request can be issued is the queue
depth. As I wrote here:
https://marc.info/?l=linux-mmc&m=148336571720463&w=2
that is not the case for mmc, even with command queuing.
Also I wouldn't be surprise if BFQ needs some changes to work well with
command queuing.
It would be better if blk-mq support was experimental until we can see how
well it works in practice.
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Christoph Hellwig @ 2017-02-20 7:35 UTC (permalink / raw)
To: Jens Axboe
Cc: Bart Van Assche, Linus Torvalds, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, Christoph Hellwig, Mike Snitzer
In-Reply-To: <633d226d-cec3-a01f-a069-ffff307e9715@kernel.dk>
On Sun, Feb 19, 2017 at 06:15:41PM -0700, Jens Axboe wrote:
> I don't think that's a regression in this series, it just triggers more easily
> with this series. The BLOCK_PC removal fixes aren't touching device life times
> at all.
Yes.
> That said, we will look into this again, of course. Christoph, any idea?
No idea really - this seems so far away from the code touched, and there
are no obvious signs for a memory scamble from another object touched
that I think if it really bisects down to that issue it must be a timing
issue.
But reading Bart's message again: Did you actually bisect it down
to the is commit? Or just test the whole tree? Between the 4.10-rc5
merge and all the block tree there might a few more likely suspects
like the scsi bdi lifetime fixes that James mentioned.
^ permalink raw reply
* Re: [GIT PULL] Block pull request for- 4.11-rc1
From: Jens Axboe @ 2017-02-20 3:02 UTC (permalink / raw)
To: James Bottomley, Bart Van Assche, Linus Torvalds
Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
Christoph Hellwig, Mike Snitzer
In-Reply-To: <75b9d9d8-9395-c7af-b29d-f016e9921498@kernel.dk>
On 02/19/2017 07:59 PM, Jens Axboe wrote:
> On 02/19/2017 07:12 PM, James Bottomley wrote:
>> On Sun, 2017-02-19 at 18:15 -0700, Jens Axboe wrote:
>>> On 02/19/2017 06:09 PM, Bart Van Assche wrote:
>>>> On 02/19/2017 04:11 PM, Jens Axboe wrote:
>>>>> - Removal of the BLOCK_PC support in struct request, and
>>>>> refactoring of
>>>>> carrying SCSI payloads in the block layer. This cleans up the
>>>>> code
>>>>> nicely, and enables us to kill the SCSI specific parts of
>>>>> struct
>>>>> request, shrinking it down nicely. From Christoph mainly, with
>>>>> help
>>>>> from Hannes.
>>>>
>>>> Hello Jens, Christoph and Mike,
>>>>
>>>> Is anyone working on a fix for the regression introduced by the
>>>> BLOCK_PC removal changes (general protection fault) that I had
>>>> reported three weeks ago? See also
>>>> https://www.spinics.net/lists/raid/msg55494.html
>>>
>>> I don't think that's a regression in this series, it just triggers
>>> more easily with this series. The BLOCK_PC removal fixes aren't
>>> touching device life times at all.
>>>
>>> That said, we will look into this again, of course. Christoph, any
>>> idea?
>>
>> We could do with tracing the bdi removal failure issue fingered both by
>> the block xfstests and Omar. It's something to do with this set of
>> commits
>>
>>> - Fixes for duplicate bdi registrations and bdi/queue life time
>>> problems from Jan and Dan.
>>
>> But no-one has actually root caused it yet.
>
> The above set from Jan and Dan fixed one set of issues around this, and
> the reproducer test case was happy as well. But we've recently found
> that there are still corner cases that aren't happy, Omar reported that
> separately on Friday. So there will be a followup set for that,
> hopefully intersecting with the issue that Bart reported.
Forgot to mention, that these cases exist in 4.0 and 4.6 as well, so
neither are a new problem with this series. The fixes from Jan and
Dan didn't close all of them.
--
Jens Axboe
^ 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