Check region size during dirty log creation
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.
---
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", ®ion_size) != 1) {
- DMWARN("invalid region size string");
+ if (sscanf(argv[0], "%u", ®ion_size) != 1 ||
+ region_size < 2 ||
+ (region_size & (region_size - 1))) {
+ DMWARN("invalid region size");
return -EINVAL;
}
Alasdair G Kergon
maintainerQuoted 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]
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
Alasdair G Kergon
maintainerQuoted 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]
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
Alasdair G Kergon
maintainerQuoted 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]