linux-kernel - Re: [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is initialized during call to blk_cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 22 Feb 2011 15:20:03 +1100
From:	NeilBrown <neilb@...e.de>
To:	Vivek Goyal <vgoyal@...hat.com>
Cc:	linux-kernel@...r.kernel.org, jaxboe@...ionio.com,
	sergey.senozhatsky@...il.com, tj@...nel.org, jmoyer@...hat.com,
	snitzer@...hat.com
Subject: Re: [PATCH 0/3] [RFC] block: Enforce that ->queue_lock is
 initialized during call to blk_cleanup_queue()

On Mon, 21 Feb 2011 22:53:34 -0500 Vivek Goyal <vgoyal@...hat.com> wrote:

> Hi All,
> 
> Currently it is not clear what's the status of ->queue_lock when
> blk_cleanup_queue() is called. Now block throttle code (blk_throtl_exit())
> requires queue lock to be initialized as it takes the spin lock, so we need
> some kind of clear convention that when is it safe to assume that queue lock
> is initiliazed and blk_throtle_exit() can be called safely.
> 
> We recently ran into issues with loop driver which was trying to free
> up queue almost immediately after block_alloc_queue() due to some internal
> errors. Also NeilBrown complained that in current form, md provides its
> own spinlock and zaps that before blk_cleanup_queue() call and that will
> have issues with throttling code. Neil has not fixed the issue with
> md driver and queued up a patch in this tree.
> 
> So there is a need to make sure blk_throtl_exit() can be called safely
> and to also catch any errors if some drivers are not doing so. This
                                                   ^^^
"now" !!  I make that typo all the time too :-(

> patch series puts a WARN_ON(!q->queue_lock) in blk_cleanup_queue() and
> moves the queue lock initialization in queue allocation code. Also it
> move blk_throtl_exit() from release_queue() to cleanup_queue().

I'm not sure there is a lot of point in the WARN_ON.
Modules that replace ->queue_lock are unlikely to set it to NULL when
done, they will just free the structure that it points into.
This will already be caught if you compile with DEBUG_PAGEALLOC and
DEBUG_SPINLOCK (I think) and there is not much else you can do to catch
these.

Thanks,
NeilBrown


> 
> These pathes are only boot tested on one machine. Before I do more
> extensive testing, I wanted to throw it out there and figure out
> if it makes sense or not.
> 
> Thanks
> Vivek
> 
> Vivek Goyal (3):
>   block: Initialize ->queue_lock to internal lock at queue allocation
>     time
>   loop: No need to initialize ->queue_lock explicitly before calling
>     blk_cleanup_queue()
>   block: Move blk_throtl_exit() call to blk_cleanup_queue()
> 
>  block/blk-core.c       |   26 ++++++++++++++++++++++++--
>  block/blk-settings.c   |    7 -------
>  block/blk-sysfs.c      |    2 --
>  block/blk-throttle.c   |    2 +-
>  drivers/block/loop.c   |    3 ---
>  include/linux/blkdev.h |    2 --
>  6 files changed, 25 insertions(+), 17 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/