linux-kernel - Re: WARNING in 2.6.25-07422-gb66e1f1

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:	Fri, 9 May 2008 12:15:27 +1000
From:	Neil Brown <neilb@...e.de>
To:	Dan Williams <dan.j.williams@...el.com>
Cc:	Jens Axboe <jens.axboe@...cle.com>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Jacek Luczak <difrost.kernel@...il.com>,
	Prakash Punnoor <prakash@...noor.de>,
	Linux Kernel list <linux-kernel@...r.kernel.org>,
	linux-raid@...r.kernel.org
Subject: Re: WARNING in 2.6.25-07422-gb66e1f1

On Thursday May 8, dan.j.williams@...el.com wrote:
> On Thu, 2008-05-08 at 11:46 -0700, Dan Williams wrote:
> Subject: md: tell blk-core about device_lock for protecting the queue flags
> From: Dan Williams <dan.j.williams@...el.com>
> 
> Now that queue flags are no longer atomic (commit:
> 75ad23bc0fcb4f992a5d06982bf0857ab1738e9e) blk-core checks the queue is locked
> via ->queue_lock.  As noticed by Neil conf->device_lock already satisfies this
> requirement.
> 
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
> 
>  drivers/md/linear.c         |    6 ++++++
>  drivers/md/multipath.c      |    6 ++++++
>  drivers/md/raid0.c          |    6 ++++++
>  drivers/md/raid1.c          |    7 ++++++-
>  drivers/md/raid10.c         |    7 ++++++-
>  drivers/md/raid5.c          |    2 ++
>  include/linux/raid/linear.h |    3 ++-
>  include/linux/raid/raid0.h  |    1 +
>  8 files changed, 35 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/drivers/md/linear.c b/drivers/md/linear.c
> index 0b85117..d026f08 100644
> --- a/drivers/md/linear.c
> +++ b/drivers/md/linear.c
> @@ -122,6 +122,10 @@ static linear_conf_t *linear_conf(mddev_t *mddev, int raid_disks)
>  	cnt = 0;
>  	conf->array_size = 0;
>  
> +	spin_lock_init(&conf->device_lock);
> +	/* blk-core uses queue_lock to verify protection of the queue flags */
> +	mddev->queue->queue_lock = &conf->device_lock;
> +
>  	rdev_for_each(rdev, tmp, mddev) {
>  		int j = rdev->raid_disk;
>  		dev_info_t *disk = conf->disks + j;
> @@ -133,8 +137,10 @@ static linear_conf_t *linear_conf(mddev_t *mddev, int raid_disks)
>  
>  		disk->rdev = rdev;
>  
> +		spin_lock(&conf->device_lock);
>  		blk_queue_stack_limits(mddev->queue,
>  				       rdev->bdev->bd_disk->queue);
> +		spin_unlock(&conf->device_lock);
>  		/* as we don't honour merge_bvec_fn, we must never risk
>  		 * violating it, so limit ->max_sector to one PAGE, as
>  		 * a one page request is never in violation.

This shouldn't be necessary.
There is no actual race here -- mddev->queue->queue_flags is not going to be
accessed by anyone else until do_md_run does
	mddev->queue->make_request_fn = mddev->pers->make_request;
which is much later.
So we only need to be sure that "queue_is_locked" doesn't complain.
And as q->queue_lock is still NULL at this point, it won't complain.

I think that the *only* change that is needs is to put

> +	/* blk-core uses queue_lock to verify protection of the queue flags */
> +	mddev->queue->queue_lock = &conf->device_lock;

after each
> +	spin_lock_init(&conf->device_lock);

i.e. in raid1.c, raid10.c and raid5.c

??

NeilBrown
--
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/