Skip to contentSkip to conversation

Check region size during dirty log creation

PATCH In review Heinz Mauelshagen 2 participants 6 messages Oct 29, 2008 Device mapper

lore
Share & tools
lore

Heinz Mauelshagen

author
#0 lore raw

PATCH 1/1 Check region size during dirty log creation

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 <[email protected]>

---

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;
}

maintainer
#1 lore raw
Quoted 2 lines · 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.

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

Signature · 1 line
[email protected]
author
#2 lore raw

Am Mittwoch, den 29.10.2008, 13:56 +0000 schrieb Alasdair G Kergon:

Quoted 7 lines · Heinz Mauelshagen
Quoted 2 lines

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.

Quoted 7 lines

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

maintainer
#3 lore raw
Quoted 7 lines · Heinz Mauelshagen

Am Mittwoch, den 29.10.2008, 13:56 +0000 schrieb Alasdair G Kergon:

Quoted 2 lines · Heinz Mauelshagen

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

Signature · 1 line
[email protected]
author
#4 lore raw

Am Mittwoch, den 29.10.2008, 15:43 +0000 schrieb Alasdair G Kergon:

Quoted 10 lines · Heinz Mauelshagen
Quoted 7 lines

Am Mittwoch, den 29.10.2008, 13:56 +0000 schrieb Alasdair G Kergon:

Quoted 2 lines · Heinz Mauelshagen

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.

Quoted 1 line

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 ?

Quoted 2 lines

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

Quoted 2 lines

Alasdair

maintainer
#5 lore raw
Quoted 2 lines · Heinz Mauelshagen

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

Signature · 1 line
[email protected]