On Tue, Jul 09, 2024 at 09:06:58AM +0200, Przemek Kitszel wrote:
> On 7/8/24 21:18, Kees Cook wrote:
> > diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> > index de8cf5d75f34..7bb9cacb380f 100644
> > --- a/fs/pstore/blk.c
> > +++ b/fs/pstore/blk.c
> > @@ -297,7 +297,7 @@ static int __init __best_effort_init(void)
> > return -EINVAL;
> > }
> > - best_effort_dev = kzalloc(sizeof(*best_effort_dev), GFP_KERNEL);
> > + best_effort_dev = kzalloc(best_effort_dev, GFP_KERNEL);
> > if (!best_effort_dev)
> > return -ENOMEM;
> I expect raised eyebrows and typical vocalizations of amusement :D -
> IOW: we should consider changing the name of the macro, although I like
> it as is :)
Yeah, I prefer keeping the name too. I feel like adding yet more macros
around the allocators does not make anyone too happy. :)
> other:
> you repeat the pointer name twice, and code is magic anyway, so perhaps:
> kzalloc_me(best_effort_dev, GFP_KERNEL);
> and another variant to cover declaration-with-init.
Switch the calling style is, however, where I think it'd be good
to change the name. I've had push-back in the past on changing away
from the "assignment style" to the "pass by reference" style, but I
would honestly prefer dropping the assignment: it is almost always
redundant. (I understood the push-back to be around the case of not
being able to easily use the "return kmalloc(...)" code pattern.)
It also makes it easier to deal with fixed array and flexible array
variants, as the argument count can be examined to determine if this is
a fixed-size struct or a flexible object:
kzalloc_struct(ptr, GFP_KERNEL);
kzalloc_struct(ptr, flex_member, count, GFP_KERNEL);
-> uses struct_size() to get allocation size
I wonder if we can find a way to also handle the array case at compile
time:
kzalloc_struct(array, count, GFP_KERNEL);
And if so, maybe the naming should be "kzalloc_me" like you suggest, or
maybe "kzalloc_obj"?
The resulting Coccinelle script gets a little more complex since we have
to rewrite the matched function, but it's not bad:
@find@
type TYPE;
TYPE *P;
expression GFP;
identifier ALLOC =~ "k[mz]alloc";
@@
P = ALLOC(\(sizeof(*P)\|sizeof(TYPE)\), GFP)
@script:python rename@
alloc_name << find.ALLOC;
ALLOC_OBJ;
@@
coccinelle.ALLOC_OBJ = cocci.make_ident(alloc_name + "_obj")
@convert@
type find.TYPE;
TYPE *find.P;
expression find.GFP;
identifier find.ALLOC;
identifier rename.ALLOC_OBJ;
@@
- P = ALLOC(\(sizeof(*P)\|sizeof(TYPE)\), GFP)
+ ALLOC_OBJ(P, GFP)
And the results in fs/pstore/ look like this:
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index de8cf5d75f34..bc0e0a170604 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -297,7 +297,7 @@ static int __init __best_effort_init(void)
return -EINVAL;
}
- best_effort_dev = kzalloc(sizeof(*best_effort_dev), GFP_KERNEL);
+ kzalloc_obj(best_effort_dev, GFP_KERNEL);
if (!best_effort_dev)
return -ENOMEM;
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..e0ba70543121 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -682,7 +682,7 @@ void pstore_get_backend_records(struct pstore_info *psi,
struct pstore_record *record;
int rc;
- record = kzalloc(sizeof(*record), GFP_KERNEL);
+ kzalloc_obj(record, GFP_KERNEL);
if (!record) {
pr_err("out of memory creating record\n");
break;
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b1a455f42e93..93064ba2c480 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -228,8 +228,7 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
*/
struct persistent_ram_zone *tmp_prz, *prz_next;
- tmp_prz = kzalloc(sizeof(struct persistent_ram_zone),
- GFP_KERNEL);
+ kzalloc_obj(tmp_prz, GFP_KERNEL);
if (!tmp_prz)
return -ENOMEM;
prz = tmp_prz;
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index f1848cdd6d34..9561f4dfa853 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -588,7 +588,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
struct persistent_ram_zone *prz;
int ret = -ENOMEM;
- prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
+ kzalloc_obj(prz, GFP_KERNEL);
if (!prz) {
pr_err("failed to allocate persistent ram zone\n");
goto err;
diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
index 694db616663f..312796dc93f0 100644
--- a/fs/pstore/zone.c
+++ b/fs/pstore/zone.c
@@ -1165,7 +1165,7 @@ static struct pstore_zone *psz_init_zone(enum pstore_type_id type,
return ERR_PTR(-ENOMEM);
}
- zone = kzalloc(sizeof(struct pstore_zone), GFP_KERNEL);
+ kzalloc_obj(zone, GFP_KERNEL);
if (!zone)
return ERR_PTR(-ENOMEM);
--
Kees Cook
