linux-kernel - Re: [PATCH 7/8] lguest: trivial guest block driver

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: <20070212044339.GJ3685@kernel.dk>
Date:	Mon, 12 Feb 2007 05:43:39 +0100
From:	Jens Axboe <jens.axboe@...cle.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	lkml - Kernel Mailing List <linux-kernel@...r.kernel.org>,
	virtualization <virtualization@...ts.osdl.org>
Subject: Re: [PATCH 7/8] lguest: trivial guest block driver

On Mon, Feb 12 2007, Rusty Russell wrote:
> +static irqreturn_t lgb_irq(int irq, void *_bd)
> +{
> +	struct blockdev *bd = _bd;
> +	unsigned long flags;
> +
> +	if (!bd->req) {
> +		pr_debug("No work!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	if (!bd->lb_page->result) {
> +		pr_debug("No result!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	spin_lock_irqsave(&bd->lock, flags);
> +	end_request(bd->req, bd->lb_page->result == 1);
> +	bd->req = NULL;
> +	bd->dma.used_len = 0;
> +	blk_start_queue(bd->disk->queue);
> +	spin_unlock_irqrestore(&bd->lock, flags);
> +	return IRQ_HANDLED;
> +}

You are using the old-style end request handling. So while I generally
discourage use of end_request(), you seem to have a bigger problem here:

> +static unsigned int req_to_dma(struct request *req, struct lguest_dma *dma)
> +{
> +	unsigned int i = 0, idx, len = 0;
> +	struct bio *bio;
> +
> +	rq_for_each_bio(bio, req) {
> +		struct bio_vec *bvec;
> +		bio_for_each_segment(bvec, bio, idx) {
> +			BUG_ON(i == LGUEST_MAX_DMA_SECTIONS);
> +			BUG_ON(!bvec->bv_len);
> +			dma->addr[i] = page_to_phys(bvec->bv_page)
> +				+ bvec->bv_offset;
> +			dma->len[i] = bvec->bv_len;
> +			len += bvec->bv_len;
> +			i++;
> +		}
> +	}
> +	if (i < LGUEST_MAX_DMA_SECTIONS)
> +		dma->len[i] = 0;
> +	return len;
> +}

Here you map the entire request (lets call that segment A..Z), but
end_request() only completes the first chunk of the request. So
elv_next_request() will retrieve the same request again, and you'll then
map B..Z and repeat that transfer. So unless I'm missing some other part
here (just read it over quickly), you are re-doing large parts of a
merged request several times.

So: don't use end_request(). Add some driver helper that does:

static void lgb_end_request(struct blockdev *bd)
{
        int uptodate = bd->lb_page->result == 1;
        struct request *rq = bd->req;

        end_that_request_first(rq, uptodate, req->hard_nr_sectors);
        add_disk_randomness(rq->rq_disk);
        blkdev_dequeue_request(rq);
        end_that_request_last(rq, uptodate);
}

We could probably even make that a block layer helper, I'm sure others
could be cleaned up with that as well. You want to use that helper in
do_lgb_request() as well.

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