On 9/5/22 11:22, Mikulas Patocka wrote:
Previously, ti->error and ti_error strings were pointing to statically
allocated memory (the .rodata section) and it was not possible to add
parameters to the error strings.
This patch makes possible to allocate error strings dynamically using the
"kasprintf" function, so we can add arbitrary parameters to the strings.
I am so excited by this change.
We need to free the error string only if is allocated with kasprintf. So,
we introduce a function "dm_free_error" that tests if the string points to
the module area (and doesn't free the string if it does), then it calls
kfree_const. kfree_const detects if the string points to the kernel
.rodata section and frees the string if it is not in .rodata.
As I understand this change, if some new user used vmalloc to allocate a
error string, DM would leak the memory. So I think the struct dm_target
error member deserves documentation that it can be either constant or
allocated via kmalloc, but not vmalloc.
All of these dynamic messages seem to add the target type. Might it be
worth adding a helper macro to set a dynamic ti->error, like DMEMIT etc,
which added a prefix of "target type %s:": and enforced that
kasprintf(GFP_NOIO, ...) is used?
Thanks!
Sweet Tea
Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
---
drivers/md/dm-ioctl.c | 8 +++++++-
drivers/md/dm-table.c | 16 ++++++++++------
include/linux/device-mapper.h | 6 ++++++
3 files changed, 23 insertions(+), 7 deletions(-)
Index: linux-dm/drivers/md/dm-ioctl.c
===================================================================
--- linux-dm.orig/drivers/md/dm-ioctl.c 2022-09-05 14:31:51.000000000 +0200
+++ linux-dm/drivers/md/dm-ioctl.c 2022-09-05 14:31:51.000000000 +0200
@@ -1465,7 +1465,8 @@ static int table_load(struct file *filp,
DMERR("can't replace immutable target type %s",
immutable_target_type->name);
r = -EINVAL;
- ti_error = "can't replace immutable target";
+ ti_error = kasprintf(GFP_NOIO, "can't replace immutable target type %s",
+ immutable_target_type->name);
goto err_unlock_md_type;
}
@@ -1521,11 +1522,16 @@ err_destroy_table:
err:
dm_put(md);
err0:
+ if (!ti_error)
+ ti_error = "can't allocte error string";
if (param->flags & DM_RETURN_ERROR_FLAG) {
param->flags &= ~DM_RETURN_ERROR_FLAG;
strlcpy(param->name, ti_error, sizeof param->name);
param->error = r;
+ dm_free_error(ti_error);
return 0;
+ } else {
+ dm_free_error(ti_error);
}
return r;
}
Index: linux-dm/drivers/md/dm-table.c
===================================================================
--- linux-dm.orig/drivers/md/dm-table.c 2022-09-05 14:31:51.000000000 +0200
+++ linux-dm/drivers/md/dm-table.c 2022-09-05 14:31:51.000000000 +0200
@@ -656,32 +656,32 @@ int dm_table_add_target(struct dm_table
ti->type = dm_get_target_type(type);
if (!ti->type) {
- ti->error = "unknown target type";
+ ti->error = kasprintf(GFP_NOIO, "unknown target type %s", type);
r = -EINVAL;
goto bad0;
}
if (dm_target_needs_singleton(ti->type)) {
if (t->num_targets) {
- ti->error = "singleton target type must appear alone in table";
+ ti->error = kasprintf(GFP_NOIO, "singleton target type %s must appear alone in table", type);
goto bad1;
}
t->singleton = true;
}
if (dm_target_always_writeable(ti->type) && !(t->mode & FMODE_WRITE)) {
- ti->error = "target type may not be included in a read-only table";
+ ti->error = kasprintf(GFP_NOIO, "target type %s may not be included in a read-only table", type);
goto bad1;
}
if (t->immutable_target_type) {
if (t->immutable_target_type != ti->type) {
- ti->error = "immutable target type cannot be mixed with other target types";
+ ti->error = kasprintf(GFP_NOIO, "immutable target type %s cannot be mixed with other target types", type);
goto bad1;
}
} else if (dm_target_is_immutable(ti->type)) {
if (t->num_targets) {
- ti->error = "immutable target type cannot be mixed with other target types";
+ ti->error = kasprintf(GFP_NOIO, "immutable target type %s cannot be mixed with other target types", type);
goto bad1;
}
t->immutable_target_type = ti->type;
@@ -705,7 +705,7 @@ int dm_table_add_target(struct dm_table
r = dm_split_args(&argc, &argv, params);
if (r) {
- ti->error = "couldn't split parameters";
+ ti->error = kasprintf(GFP_NOIO, "couldn't split parameters for target %s", type);
goto bad1;
}
@@ -728,9 +728,13 @@ int dm_table_add_target(struct dm_table
bad1:
dm_put_target_type(ti->type);
bad0:
+ if (!ti->error)
+ ti->error = "can't allocte error string";
Typo: allocte->allocate.
DMERR("%s: %s: %s (%pe)", dm_device_name(t->md), type, ti->error, ERR_PTR(r));
if (ti_error)
*ti_error = ti->error;
+ else
+ dm_free_error(ti->error);
return r;
}
Index: linux-dm/include/linux/device-mapper.h
===================================================================
--- linux-dm.orig/include/linux/device-mapper.h 2022-09-05 14:31:51.000000000 +0200
+++ linux-dm/include/linux/device-mapper.h 2022-09-05 14:31:51.000000000 +0200
@@ -679,4 +679,10 @@ static inline unsigned long to_bytes(sec
return (n << SECTOR_SHIFT);
}
+static void dm_free_error(char *ti_error)
+{
+ if (!is_vmalloc_or_module_addr(ti_error))
+ kfree_const(ti_error);
+}
+
#endif /* _LINUX_DEVICE_MAPPER_H */
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
