diff mbox series

[RFC,4/4] pstore: Replace classic kmalloc code pattern with typed argument

Message ID 20240708191840.335463-4-kees@kernel.org (mailing list archive)
State Handled Elsewhere
Headers show
Series slab: Allow for type introspection during allocation | expand

Commit Message

Kees Cook July 8, 2024, 7:18 p.m. UTC 
Using a short Coccinelle script, it is possible to replace the classic
kmalloc code patterns with the typed information:

@alloc@
type TYPE;
TYPE *P;
expression GFP;
identifier ALLOC =~ "k[mz]alloc";
@@

	P = ALLOC(
-		\(sizeof(*P)\|sizeof(TYPE)\), GFP)
+		P, GFP)

Show this just for kmalloc/kzalloc usage in fs/pstore as an example.

Doing this for all allocator calls in the kernel touches much more:

 11941 files changed, 22459 insertions(+), 22345 deletions(-)

And obviously requires some more wrappers for kv*alloc, devm_*alloc,
etc.

Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Tony Luck <tony.luck@intel.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
Cc: linux-hardening@vger.kernel.org
---
 fs/pstore/blk.c      | 2 +-
 fs/pstore/platform.c | 2 +-
 fs/pstore/ram.c      | 3 +--
 fs/pstore/ram_core.c | 2 +-
 fs/pstore/zone.c     | 2 +-
 5 files changed, 5 insertions(+), 6 deletions(-)

Comments

Przemek Kitszel July 9, 2024, 7:06 a.m. UTC | #1 
On 7/8/24 21:18, Kees Cook wrote:
> Using a short Coccinelle script, it is possible to replace the classic
> kmalloc code patterns with the typed information:
> 
> @alloc@
> type TYPE;
> TYPE *P;
> expression GFP;
> identifier ALLOC =~ "k[mz]alloc";
> @@
> 
> 	P = ALLOC(
> -		\(sizeof(*P)\|sizeof(TYPE)\), GFP)
> +		P, GFP)
> 
> Show this just for kmalloc/kzalloc usage in fs/pstore as an example.
> 
> Doing this for all allocator calls in the kernel touches much more:
> 
>   11941 files changed, 22459 insertions(+), 22345 deletions(-)
> 
> And obviously requires some more wrappers for kv*alloc, devm_*alloc,
> etc.
> 
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
> Cc: linux-hardening@vger.kernel.org
> ---
>   fs/pstore/blk.c      | 2 +-
>   fs/pstore/platform.c | 2 +-
>   fs/pstore/ram.c      | 3 +--
>   fs/pstore/ram_core.c | 2 +-
>   fs/pstore/zone.c     | 2 +-
>   5 files changed, 5 insertions(+), 6 deletions(-)
> 
> 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 :)

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.
Kees Cook July 9, 2024, 4:32 p.m. UTC | #2 
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);
diff mbox series

Patch

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;
 
diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index 03425928d2fb..4e527c3ea530 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);
+		record = kzalloc(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..a0665a98b135 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);
+			tmp_prz = kzalloc(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..01ddf1be6c3a 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);
+	prz = kzalloc(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..8df890bb4db9 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);
+	zone = kzalloc(zone, GFP_KERNEL);
 	if (!zone)
 		return ERR_PTR(-ENOMEM);