linux-kernel - Re: [PATCH] block: fix accounting bug on cross partition merges

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]

Message-ID: <4D0BB4A1.8080305@fusionio.com>
Date:	Fri, 17 Dec 2010 20:06:09 +0100
From:	Jens Axboe <jaxboe@...ionio.com>
To:	Jerome Marchand <jmarchan@...hat.com>
CC:	Vivek Goyal <vgoyal@...hat.com>,
	Satoru Takeuchi <takeuchi_satoru@...fujitsu.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Yasuaki Ishimatsu <isimatu.yasuaki@...fujitsu.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] block: fix accounting bug on cross partition merges

On 2010-12-17 14:42, Jerome Marchand wrote:
> 
> /proc/diskstats would display a strange output as follows.

[snip]

This looks a lot better! One comment:

> diff --git a/block/blk-core.c b/block/blk-core.c
> index 4ce953f..064921d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -64,13 +64,16 @@ static void drive_stat_acct(struct request *rq, int new_io)
>  		return;
>  
>  	cpu = part_stat_lock();
> -	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
>  
> -	if (!new_io)
> +	if (!new_io) {
> +		part = rq->part;
>  		part_stat_inc(cpu, part, merges[rw]);
> -	else {
> +	} else {
> +		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
>  		part_round_stats(cpu, part);
>  		part_inc_in_flight(part, rw);
> +		kref_get(&part->ref);
> +		rq->part = part;
>  	}
>  
>  	part_stat_unlock();

I don't think this is completely safe. The rcu lock is held due to the
part_stat_lock(), but that only prevents the __delete_partition()
callback from happening. Lets say you have this:

CPU0                                         CPU1
part = disk_map_sector_rcu()
                                             kref_put(part); <- now 0
part_stat_unlock()
                                             __delete_partition();
                                             ...
                                             delete_partition_rcu_cb();
merge, or endio, boom

Now rq has ->part pointing to freed memory, later merges or end
accounting will touch freed memory.

I think we can fix this by just having delete_partition_rcu_rb() check
the reference count and return if non-zero. Since someone holds a
reference to the table, they will drop it and we'll re-schedule the rcu
callback.


-- 
Jens Axboe

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