CHROMIUM: verity: use sg_set_buf instead of sg_set_page

CHROMIUM: verity: use sg_set_buf instead of sg_set_page

This is in preparation for removing the assumption that the block
size = PAGE_SIZE.

BUG=9752
TEST=Ran tests in verity.git. Ran platform_DMVerityCorruption on H/W.

Change-Id: I4ef68f914f58f181c131df6cccab0d27e5e00ee2
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>;

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=5ee4dc2

[Will Drewry, 2011-03-11 19:01:57]

This is fine, but the reason it wasn't done before is because I didn't have good
answers around alignment.  Do you know?

http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c
File drivers/md/dm-bht.c (right):

http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode105
drivers/md/dm-bht.c:105: sg_set_buf(&sg, block, PAGE_SIZE);
I considered using this before but don't we need to make sure that the block of
PAGE_SIZE doesn't exceed the page?

sg_set_buf is just  sg_set_page(sg, virt_to_page(buf), buflen,
offset_in_page(buf)) but then set_page just assigns the page with length.  If
our data spills over onto another page, I'm not sure what happens.

Do you know?

http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode639
drivers/md/dm-bht.c:639: sg_set_buf(&sg, entry->nodes, PAGE_SIZE);
This one is known safe since we use a page pool :)

[Mandeep Singh Baines, 2011-03-11 19:28:03]

Thanks much for the prompt reviews!

wad@chromium.org (wad@chromium.org) wrote:
> This is fine, but the reason it wasn't done before is because I
> didn't have good
> answers around alignment.  Do you know?
> 

We only pass hash data that is within a page. We could add BUG_ONs.

> 
> http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c
> File drivers/md/dm-bht.c (right):
> 
> http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode105
> drivers/md/dm-bht.c:105: sg_set_buf(&sg, block, PAGE_SIZE);
> I considered using this before but don't we need to make sure that the
> block of PAGE_SIZE doesn't exceed the page?
> 
> sg_set_buf is just  sg_set_page(sg, virt_to_page(buf), buflen,
> offset_in_page(buf)) but then set_page just assigns the page with
> length.  If our data spills over onto another page, I'm not sure what
> happens.
> 
> Do you know?
> 

We check alignment is dm-verity.c:

static int verity_verify(struct verity_config *vc,
			 struct bio *bio)
{
	int r;
	unsigned int idx, block;

	VERITY_BUG_ON(bio == NULL);

	block = to_bytes(bio->bi_sector) >> VERITY_BLOCK_SHIFT;

	for (idx = bio->bi_idx; idx < bio->bi_vcnt; idx++) {
		struct bio_vec *bv = bio_iovec_idx(bio, idx);

		VERITY_BUG_ON(bv->bv_offset % VERITY_BLOCK_SIZE);
		VERITY_BUG_ON(bv->bv_len % VERITY_BLOCK_SIZE);

I could add a similar BUG_ON in dm-bht.c

VERITY_BLOCK_SIZE is a power of 2 and strictly <= PAGE_SIZE.

> http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode639
> drivers/md/dm-bht.c:639: sg_set_buf(&sg, entry->nodes, PAGE_SIZE);
> This one is known safe since we use a page pool :)
> 
> http://codereview.chromium.org/6684008/

[Will Drewry, 2011-03-11 19:33:55]

On 2011/03/11 19:28:03, Mandeep Singh Baines wrote:
> Thanks much for the prompt reviews!
> 
> mailto:wad@chromium.org (mailto:wad@chromium.org) wrote:
> > This is fine, but the reason it wasn't done before is because I
> > didn't have good
> > answers around alignment.  Do you know?
> > 
> 
> We only pass hash data that is within a page. We could add BUG_ONs.
> 
> > 
> > http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c
> > File drivers/md/dm-bht.c (right):
> > 
> > http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode105
> > drivers/md/dm-bht.c:105: sg_set_buf(&sg, block, PAGE_SIZE);
> > I considered using this before but don't we need to make sure that the
> > block of PAGE_SIZE doesn't exceed the page?
> > 
> > sg_set_buf is just  sg_set_page(sg, virt_to_page(buf), buflen,
> > offset_in_page(buf)) but then set_page just assigns the page with
> > length.  If our data spills over onto another page, I'm not sure what
> > happens.
> > 
> > Do you know?
> > 
> 
> We check alignment is dm-verity.c:
> 
> static int verity_verify(struct verity_config *vc,
> 			 struct bio *bio)
> {
> 	int r;
> 	unsigned int idx, block;
> 
> 	VERITY_BUG_ON(bio == NULL);
> 
> 	block = to_bytes(bio->bi_sector) >> VERITY_BLOCK_SHIFT;
> 
> 	for (idx = bio->bi_idx; idx < bio->bi_vcnt; idx++) {
> 		struct bio_vec *bv = bio_iovec_idx(bio, idx);
> 
> 		VERITY_BUG_ON(bv->bv_offset % VERITY_BLOCK_SIZE);
> 		VERITY_BUG_ON(bv->bv_len % VERITY_BLOCK_SIZE);
> 
> I could add a similar BUG_ON in dm-bht.c
> 
> VERITY_BLOCK_SIZE is a power of 2 and strictly <= PAGE_SIZE.

So we'll need to update the comments around any place this bubbles up in the
interfaces because to avoid future pain if anyone else starts to use dm-bht (or
we use it for other purposes).

A BUG_ON() would at least be a safe cross-check.  We need to know:
(a) length < page_size
(b) offset_into_page + length < page_size

to ensure we don't artificially truncate.

Does that make sense?

> 
> > http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode639
> > drivers/md/dm-bht.c:639: sg_set_buf(&sg, entry->nodes, PAGE_SIZE);
> > This one is known safe since we use a page pool :)
> > 
> > http://codereview.chromium.org/6684008/

[Mandeep Singh Baines, 2011-03-11 21:18:07]

Fixed. PTAL.

wad@chromium.org (wad@chromium.org) wrote:
> On 2011/03/11 19:28:03, Mandeep Singh Baines wrote:
> >Thanks much for the prompt reviews!
> 
> >mailto:wad@chromium.org (mailto:wad@chromium.org) wrote:
> >> This is fine, but the reason it wasn't done before is because I
> >> didn't have good
> >> answers around alignment.  Do you know?
> >>
> 
> >We only pass hash data that is within a page. We could add BUG_ONs.
> 
> >>
> >> http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c
> >> File drivers/md/dm-bht.c (right):
> >>
> >>
http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode105
> >> drivers/md/dm-bht.c:105: sg_set_buf(&sg, block, PAGE_SIZE);
> >> I considered using this before but don't we need to make sure that the
> >> block of PAGE_SIZE doesn't exceed the page?
> >>
> >> sg_set_buf is just  sg_set_page(sg, virt_to_page(buf), buflen,
> >> offset_in_page(buf)) but then set_page just assigns the page with
> >> length.  If our data spills over onto another page, I'm not sure what
> >> happens.
> >>
> >> Do you know?
> >>
> 
> >We check alignment is dm-verity.c:
> 
> >static int verity_verify(struct verity_config *vc,
> >			 struct bio *bio)
> >{
> >	int r;
> >	unsigned int idx, block;
> 
> >	VERITY_BUG_ON(bio == NULL);
> 
> >	block = to_bytes(bio->bi_sector) >> VERITY_BLOCK_SHIFT;
> 
> >	for (idx = bio->bi_idx; idx < bio->bi_vcnt; idx++) {
> >		struct bio_vec *bv = bio_iovec_idx(bio, idx);
> 
> >		VERITY_BUG_ON(bv->bv_offset % VERITY_BLOCK_SIZE);
> >		VERITY_BUG_ON(bv->bv_len % VERITY_BLOCK_SIZE);
> 
> >I could add a similar BUG_ON in dm-bht.c
> 
> >VERITY_BLOCK_SIZE is a power of 2 and strictly <= PAGE_SIZE.
> 
> So we'll need to update the comments around any place this bubbles up in the
> interfaces because to avoid future pain if anyone else starts to use
> dm-bht (or
> we use it for other purposes).
> 
> A BUG_ON() would at least be a safe cross-check.  We need to know:
> (a) length < page_size
> (b) offset_into_page + length < page_size
> 
> to ensure we don't artificially truncate.
> 
> Does that make sense?
> 
> 
> >>
http://codereview.chromium.org/6684008/diff/1/drivers/md/dm-bht.c#newcode639
> >> drivers/md/dm-bht.c:639: sg_set_buf(&sg, entry->nodes, PAGE_SIZE);
> >> This one is known safe since we use a page pool :)
> >>
> >> http://codereview.chromium.org/6684008/
> 
> 
> 
> http://codereview.chromium.org/6684008/

[Will Drewry, 2011-03-11 21:32:29]

LGTM

I look forward to seeing the unittest kernel stubbing stuff in userspace :)

http://codereview.chromium.org/6684008/diff/6001/drivers/md/dm-bht.c
File drivers/md/dm-bht.c (right):

http://codereview.chromium.org/6684008/diff/6001/drivers/md/dm-bht.c#newcode108
drivers/md/dm-bht.c:108: BUG_ON((uintptr_t)block % PAGE_SIZE);
IS_ALIGNED(block, PAGE_SIZE)?

[Paul T, 2011-03-14 16:42:59]

LGTM