[PATCH] Check region size during dirty log creation

[PATCH] Check region size during dirty log creation

[Heinz Mauelshagen]

This patch adds checks to the dirty log creation for region size to be
larger than 2 sectors and to be a power of 2.

Signed-off-by: Heinz Mauelshagen <heinzm@redhat.com>
---

diff -up linux-2.6.27.4/drivers/md/dm-log.c.orig
linux-2.6.27.4/drivers/md/dm-log.c
--- linux-2.6.27.4/drivers/md/dm-log.c.log	2008-10-29 13:51:03.000000000
+0100
+++ linux-2.6.27.4/drivers/md/dm-log.c	2008-10-29 13:55:57.000000000
+0100
@@ -403,8 +403,10 @@ static int create_log_context(struct dm_
 		}
 	}
 
-	if (sscanf(argv[0], "%u", &region_size) != 1) {
-		DMWARN("invalid region size string");
+	if (sscanf(argv[0], "%u", &region_size) != 1 ||
+	    region_size < 2 ||
+	    (region_size & (region_size - 1))) {
+		DMWARN("invalid region size");
 		return -EINVAL;
 	}
 

Re: [PATCH] Check region size during dirty log creation

[Alasdair G Kergon]

On Wed, Oct 29, 2008 at 02:02:08PM +0100, Heinz Mauelshagen wrote:
> This patch adds checks to the dirty log creation for region size to be
> larger than 2 sectors and to be a power of 2.
 
Interesting we didn't validate that, but the userspace validation is
tighter.

        if (lp->region_size % (pagesize >> SECTOR_SHIFT)) {
                log_error("Region size (%" PRIu32 ") must be a multiple of "
                          "machine memory page size (%d)",
                          lp->region_size, pagesize >> SECTOR_SHIFT);

The validation should be consistent between userspace and kernel.

Alasdair
-- 
agk@redhat.com

Re: [PATCH] Check region size during dirty log creation

[Heinz Mauelshagen]

Am Mittwoch, den 29.10.2008, 13:56 +0000 schrieb Alasdair G Kergon:
> On Wed, Oct 29, 2008 at 02:02:08PM +0100, Heinz Mauelshagen wrote:
> > This patch adds checks to the dirty log creation for region size to be
> > larger than 2 sectors and to be a power of 2.
>  
> Interesting we didn't validate that, but the userspace validation is
> tighter.
> 
>         if (lp->region_size % (pagesize >> SECTOR_SHIFT)) {
>                 log_error("Region size (%" PRIu32 ") must be a multiple of "
>                           "machine memory page size (%d)",
>                           lp->region_size, pagesize >> SECTOR_SHIFT);
> 
> The validation should be consistent between userspace and kernel.
> Alasdair

It should be sufficiently restrictive in the kernel to prevent
programming errors causing oopses. It can be called via the kernel
interface by any application anyway, so doesn't need to enforce our
uspace restrictions (think direct IOCTL calls).

Heinz

Re: [PATCH] Check region size during dirty log creation

[Alasdair G Kergon]

On Wed, Oct 29, 2008 at 04:07:30PM +0100, Heinz Mauelshagen wrote:
> Am Mittwoch, den 29.10.2008, 13:56 +0000 schrieb Alasdair G Kergon:
> > On Wed, Oct 29, 2008 at 02:02:08PM +0100, Heinz Mauelshagen wrote:
> > The validation should be consistent between userspace and kernel.
> It should be sufficiently restrictive in the kernel to prevent
> programming errors causing oopses. It can be called via the kernel
> interface by any application anyway, so doesn't need to enforce our
> uspace restrictions (think direct IOCTL calls).
 
Surely the validation in the two places should match?
Why would userspace need a tighter restriction than the kernel?
One or the other is incorrect.
- E.g. Does kcopyd handle sub-page regions OK now?

Alasdair
-- 
agk@redhat.com

Re: [PATCH] Check region size during dirty log creation

[Heinz Mauelshagen]

Am Mittwoch, den 29.10.2008, 15:43 +0000 schrieb Alasdair G Kergon:
> On Wed, Oct 29, 2008 at 04:07:30PM +0100, Heinz Mauelshagen wrote:
> > Am Mittwoch, den 29.10.2008, 13:56 +0000 schrieb Alasdair G Kergon:
> > > On Wed, Oct 29, 2008 at 02:02:08PM +0100, Heinz Mauelshagen wrote:
> > > The validation should be consistent between userspace and kernel.
> > It should be sufficiently restrictive in the kernel to prevent
> > programming errors causing oopses. It can be called via the kernel
> > interface by any application anyway, so doesn't need to enforce our
> > uspace restrictions (think direct IOCTL calls).
>  
> Surely the validation in the two places should match?

I'm arguing, that the kernel should only restrict to prevent against
programming errors.

> Why would userspace need a tighter restriction than the kernel?

Because of specific application requirements.
Why shouldn't we support 2 sector dirty log region sizes for any
application who wants them ?

> One or the other is incorrect.
> - E.g. Does kcopyd handle sub-page regions OK now?

kcopyd allows copying io regions of single sectors.

Heinz

> 
> Alasdair

Re: [PATCH] Check region size during dirty log creation

[Alasdair G Kergon]

On Wed, Oct 29, 2008 at 04:53:42PM +0100, Heinz Mauelshagen wrote:
> I'm arguing, that the kernel should only restrict to prevent against
> programming errors.
 
Indeed - but I'm asking where the existing constraint is coming
from and if it is still/was ever correct?

Also, this patch duplicates existing checks in _check_region_size()
that should be removed if the validation moves.

static inline int _check_region_size(struct dm_target *ti, uint32_t size)
{
        return !(size % (PAGE_SIZE >> 9) || !is_power_of_2(size) ||
                 size > ti->len);
}

Alasdair
-- 
agk@redhat.com