Skip to contentSkip to conversation

rtc: sun6i: Use struct_size() in kzalloc()

PATCH Applied Gustavo A. R. Silva 5 participants 15 messages Aug 23, 2018 – Aug 27, 2018 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo { 
	int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +1 -2
index 2cd5a7b..fe07310 100644
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
 	if (!rtc)
 		return;
 
-	clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-			   GFP_KERNEL);
+	clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
 	if (!clk_data) {
 		kfree(rtc);
 		return;
Signature · 1 line
2.7.4
#1 lore raw
Quoted 26 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo {
        int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +0 -0
index 2cd5a7b..fe07310 100644

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 9 lines
drivers/rtc/rtc-sun6i.c +1 -2
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
        if (!rtc)
                return;
 
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;

This looks like entirely correct to me, but I'm surprised the Coccinelle script didn't discover this. I guess the isomorphisms don't cover the parenthesis?

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#3 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

In reply to a message not in the archive
Quoted 40 lines · Joe Perches

Forwarding a question about coccinelle and isomorphisms from Kees Cook

---------- Forwarded message ----------
From: Kees Cook <[email protected]>
To: "Gustavo A. R. Silva" <[email protected]>
Cc: Alessandro Zummo <[email protected]>, Alexandre Belloni <[email protected]>, Maxime Ripard <[email protected]>, Chen-Yu Tsai <[email protected]>, linux-rtc at vger.kernel.org, linux-arm-kernel <[email protected]>, LKML <[email protected]> Bcc:
Date: Thu, 23 Aug 2018 13:56:35 -0700
Subject: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()

Quoted 29 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo {
        int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +0 -0
index 2cd5a7b..fe07310 100644
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
        if (!rtc)
                return;
 
Quoted 10 lines

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 6 lines
drivers/rtc/rtc-sun6i.c +1 -2
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;

This looks like entirely correct to me, but I'm surprised the Coccinelle script didn't discover this. I guess the isomorphisms don't cover the parenthesis?

I had this:

@@
identifier alloc =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; identifier VAR, ELEMENT;
expression COUNT;
@@

- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)

But I needed to explicitly change the rule to:

(
- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
|
- alloc(sizeof(*VAR) + (COUNT * sizeof(*VAR->ELEMENT))
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
)

to add the ()s. I would expect arithmetic commutative expressions to match? But I had to add parens?

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
Reviewed-by on v1 1/1
#14 lore raw
In reply to Gustavo A. R. Silva
Quoted 26 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo {
        int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +0 -0
index 2cd5a7b..fe07310 100644

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 9 lines
drivers/rtc/rtc-sun6i.c +1 -2
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
        if (!rtc)
                return;
 
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#4 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

In reply to Kees Cook
Quoted 77 lines · Kees Cook
Quoted 40 lines · Joe Perches

Forwarding a question about coccinelle and isomorphisms from Kees Cook

---------- Forwarded message ----------
From: Kees Cook <[email protected]>
To: "Gustavo A. R. Silva" <[email protected]>
Cc: Alessandro Zummo <[email protected]>, Alexandre Belloni <[email protected]>, Maxime Ripard <[email protected]>, Chen-Yu Tsai <[email protected]>, linux-rtc at vger.kernel.org, linux-arm-kernel <[email protected]>, LKML <[email protected]> Bcc:
Date: Thu, 23 Aug 2018 13:56:35 -0700
Subject: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()

Quoted 29 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo {
        int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +0 -0
index 2cd5a7b..fe07310 100644
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
        if (!rtc)
                return;
 
Quoted 10 lines

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 6 lines
drivers/rtc/rtc-sun6i.c +1 -2
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;

This looks like entirely correct to me, but I'm surprised the Coccinelle script didn't discover this. I guess the isomorphisms don't cover the parenthesis?

I had this:

@@
identifier alloc =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; identifier VAR, ELEMENT;
expression COUNT;
@@

- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)

But I needed to explicitly change the rule to:

(
- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
|
- alloc(sizeof(*VAR) + (COUNT * sizeof(*VAR->ELEMENT))
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
)

Quoted 2 lines

to add the ()s. I would expect arithmetic commutative expressions to match? But I had to add parens?

Isomorphisms don't add parens. They only remove them. If they added them, you would end up with the possibility of having them everywhere, in all permutations, which would be slow and useless.

julia

#13 lore raw
In reply to Kees Cook
Quoted 8 lines · Kees Cook

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 7 lines
drivers/rtc/rtc-sun6i.c +1 -2
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;
Quoted 4 lines

This looks like entirely correct to me, but I'm surprised the Coccinelle script didn't discover this. I guess the isomorphisms don't cover the parenthesis?

Apparently.

If I manually remove the ()s, the cocci script successfully generates a patch.

--
Gustavo

#5 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

In reply to Julia Lawall
Quoted 81 lines · Julia Lawall
Quoted 77 lines · Kees Cook
Quoted 40 lines · Joe Perches

Forwarding a question about coccinelle and isomorphisms from Kees Cook

---------- Forwarded message ----------
From: Kees Cook <[email protected]>
To: "Gustavo A. R. Silva" <[email protected]>
Cc: Alessandro Zummo <[email protected]>, Alexandre Belloni <[email protected]>, Maxime Ripard <[email protected]>, Chen-Yu Tsai <[email protected]>, linux-rtc at vger.kernel.org, linux-arm-kernel <[email protected]>, LKML <[email protected]> Bcc:
Date: Thu, 23 Aug 2018 13:56:35 -0700
Subject: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()

Quoted 29 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo {
        int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +0 -0
index 2cd5a7b..fe07310 100644
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
        if (!rtc)
                return;
 
Quoted 10 lines

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 6 lines
drivers/rtc/rtc-sun6i.c +1 -2
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;

This looks like entirely correct to me, but I'm surprised the Coccinelle script didn't discover this. I guess the isomorphisms don't cover the parenthesis?

I had this:

@@
identifier alloc =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; identifier VAR, ELEMENT;
expression COUNT;
@@

- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)

But I needed to explicitly change the rule to:

(
- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
|
- alloc(sizeof(*VAR) + (COUNT * sizeof(*VAR->ELEMENT))
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
)

Quoted 6 lines
Quoted 2 lines

to add the ()s. I would expect arithmetic commutative expressions to match? But I had to add parens?

Isomorphisms don't add parens. They only remove them. If they added them, you would end up with the possibility of having them everywhere, in all permutations, which would be slow and useless.

Would a rule for:

a + (b * c)

match:

a + b * c

?

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#6 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

Quoted 89 lines · Kees Cook
Quoted 81 lines · Julia Lawall
Quoted 77 lines · Kees Cook
Quoted 40 lines · Joe Perches

Forwarding a question about coccinelle and isomorphisms from Kees Cook

---------- Forwarded message ----------
From: Kees Cook <[email protected]>
To: "Gustavo A. R. Silva" <[email protected]>
Cc: Alessandro Zummo <[email protected]>, Alexandre Belloni <[email protected]>, Maxime Ripard <[email protected]>, Chen-Yu Tsai <[email protected]>, linux-rtc at vger.kernel.org, linux-arm-kernel <[email protected]>, LKML <[email protected]> Bcc:
Date: Thu, 23 Aug 2018 13:56:35 -0700
Subject: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()

Quoted 29 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo {
        int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +0 -0
index 2cd5a7b..fe07310 100644
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
        if (!rtc)
                return;
 
Quoted 10 lines

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 6 lines
drivers/rtc/rtc-sun6i.c +1 -2
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;

This looks like entirely correct to me, but I'm surprised the Coccinelle script didn't discover this. I guess the isomorphisms don't cover the parenthesis?

I had this:

@@
identifier alloc =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; identifier VAR, ELEMENT;
expression COUNT;
@@

- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)

But I needed to explicitly change the rule to:

(
- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
|
- alloc(sizeof(*VAR) + (COUNT * sizeof(*VAR->ELEMENT))
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
)

Quoted 6 lines
Quoted 2 lines

to add the ()s. I would expect arithmetic commutative expressions to match? But I had to add parens?

Isomorphisms don't add parens. They only remove them. If they added them, you would end up with the possibility of having them everywhere, in all permutations, which would be slow and useless.

Quoted 7 lines

Would a rule for:

a + (b * c)

match:

a + b * c

I would say yes. Basically it removes the parentheses but doesn't reparse the code, so it doesn't redo the associativity. Since * has higher precedence than +, then both will be matched. On the other hand, if you put:

(a + b) * c

It will consider a pattern with the parentheses removed, but that pattern won't match anything, because real trees that consist of a + b * c are parsed in a different way.

julia

#7 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

Quoted 106 lines · Julia Lawall
Quoted 90 lines · Kees Cook
Quoted 82 lines · Julia Lawall
Quoted 78 lines · Kees Cook
Quoted 40 lines · Joe Perches

Forwarding a question about coccinelle and isomorphisms from Kees Cook

---------- Forwarded message ----------
From: Kees Cook <[email protected]>
To: "Gustavo A. R. Silva" <[email protected]>
Cc: Alessandro Zummo <[email protected]>, Alexandre Belloni <[email protected]>, Maxime Ripard <[email protected]>, Chen-Yu Tsai <[email protected]>, linux-rtc at vger.kernel.org, linux-arm-kernel <[email protected]>, LKML <[email protected]> Bcc:
Date: Thu, 23 Aug 2018 13:56:35 -0700
Subject: Re: [PATCH] rtc: sun6i: Use struct_size() in kzalloc()

Quoted 29 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo {
        int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/rtc/rtc-sun6i.c +0 -0
index 2cd5a7b..fe07310 100644
@@ -199,8 +199,7 @@ static void __init sun6i_rtc_clk_init(struct device_node *node)
        if (!rtc)
                return;
 
Quoted 10 lines

drivers/rtc/rtc-sun6i.c static void __init sun6i_rtc_clk_init(struct device_node *node) · line 205

Quoted 6 lines
drivers/rtc/rtc-sun6i.c +1 -2
-       clk_data = kzalloc(sizeof(*clk_data) + (sizeof(*clk_data->hws) * 2),
-                          GFP_KERNEL);
+       clk_data = kzalloc(struct_size(clk_data, hws, 2), GFP_KERNEL);
        if (!clk_data) {
                kfree(rtc);
                return;

This looks like entirely correct to me, but I'm surprised the Coccinelle script didn't discover this. I guess the isomorphisms don't cover the parenthesis?

I had this:

@@
identifier alloc =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; identifier VAR, ELEMENT;
expression COUNT;
@@

- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)

But I needed to explicitly change the rule to:

(
- alloc(sizeof(*VAR) + COUNT * sizeof(*VAR->ELEMENT)
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)

Quoted 1 line

- alloc(sizeof(*VAR) + (COUNT * sizeof(*VAR->ELEMENT))
+ alloc(struct_size(VAR, ELEMENT, COUNT)
, ...)
)

Quoted 6 lines
Quoted 2 lines

to add the ()s. I would expect arithmetic commutative expressions to match? But I had to add parens?

Isomorphisms don't add parens. They only remove them. If they added them, you would end up with the possibility of having them everywhere, in all permutations, which would be slow and useless.

Quoted 7 lines

Would a rule for:

a + (b * c)

match:

a + b * c

I would say yes. Basically it removes the parentheses but doesn't reparse the code, so it doesn't redo the associativity. Since * has higher precedence than +, then both will be matched. On the other hand, if you put:

Quoted 5 lines

(a + b) * c

It will consider a pattern with the parentheses removed, but that pattern won't match anything, because real trees that consist of a + b * c are parsed in a different way.

spatch 1.0.4 doesn't seem to:

$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
$ cat match_mul.cocci 
@@
expression a, b, c;
int d;
@@

* d = a * b + c;

$ cat test_mul.c 
int a, b, c, d;

void foo(void)

{
	d = (a * b) + c;
	d = a * b + c;
}
$ spatch -sp-file match_mul.cocci test_mul.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: test_mul.c
diff = 
--- test_mul.c
+++ /tmp/cocci-output-22607-ba6f76-test_mul.c
@@ -3,5 +3,4 @@ int a, b, c, d;
 void foo(void)
 {
 	d = (a * b) + c;
-	d = a * b + c;
 }
#8 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

Quoted 20 lines · Joe Perches
Quoted 8 lines · Julia Lawall

On Thu, 23 Aug 2018, Kees Cook wrote:

(a + b) * c

It will consider a pattern with the parentheses removed, but that pattern won't match anything, because real trees that consist of a + b * c are parsed in a different way.

spatch 1.0.4 doesn't seem to:

$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@
Quoted 1 line

* d = a * b + c;

But "(a * b) + c" for the rule does:

$ cat isomorphisms.c
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])

{
        int a, b, c;

        a = atoi(argv[1]);
        b = atoi(argv[2]);
        c = atoi(argv[3]);

        printf("%d\n", a + b * c);
        printf("%d\n", (a + b) * c);
        printf("%d\n", a + (b * c));
        printf("%d\n", b * c + a);
        printf("%d\n", b * (c + a));
        printf("%d\n", (b * c) + a);

        return 0;
}
$ cat match.cocci
@@
identifier A, B, C;
@@

* (A * B) + C

$ spatch -sp-file match.cocci isomorphisms.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: isomorphisms.c
diff =
--- isomorphisms.c
+++ /tmp/cocci-output-8402-870fd6-isomorphisms.c
@@ -9,12 +9,8 @@ int main(int argc, char *argv[])
        b = atoi(argv[2]);
        c = atoi(argv[3]);
-       printf("%d\n", a + b * c);
        printf("%d\n", (a + b) * c);
-       printf("%d\n", a + (b * c));
-       printf("%d\n", b * c + a);
        printf("%d\n", b * (c + a));

- printf("%d\n", (b * c) + a);

        return 0;

}

So it sounds like I should put parens around all kinds of things in my rules...

-Kees

Signature · 2 lines
Kees Cook
Pixel Security
#9 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

Quoted 75 lines · Kees Cook
Quoted 20 lines · Joe Perches
Quoted 8 lines · Julia Lawall

On Thu, 23 Aug 2018, Kees Cook wrote:

(a + b) * c

It will consider a pattern with the parentheses removed, but that pattern won't match anything, because real trees that consist of a + b * c are parsed in a different way.

spatch 1.0.4 doesn't seem to:

$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@
Quoted 1 line

* d = a * b + c;

But "(a * b) + c" for the rule does:

$ cat isomorphisms.c
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])

{
        int a, b, c;

        a = atoi(argv[1]);
        b = atoi(argv[2]);
        c = atoi(argv[3]);

        printf("%d\n", a + b * c);
        printf("%d\n", (a + b) * c);
        printf("%d\n", a + (b * c));
        printf("%d\n", b * c + a);
        printf("%d\n", b * (c + a));
        printf("%d\n", (b * c) + a);

        return 0;
}
$ cat match.cocci
@@
identifier A, B, C;
@@

* (A * B) + C

$ spatch -sp-file match.cocci isomorphisms.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: isomorphisms.c
diff =
tmp/cocci-output-8402-870fd6-isomorphisms.c renamed from isomorphisms.c +0 -4
@@ -9,12 +9,8 @@ int main(int argc, char *argv[])
        b = atoi(argv[2]);
        c = atoi(argv[3]);
 
-       printf("%d\n", a + b * c);
        printf("%d\n", (a + b) * c);
-       printf("%d\n", a + (b * c));
-       printf("%d\n", b * c + a);
        printf("%d\n", b * (c + a));
-       printf("%d\n", (b * c) + a);
 
        return 0;
 }
Quoted 1 line

So it sounds like I should put parens around all kinds of things in my rules...

If you think someone might reasonably use parentheses somewhere, you should put them.

julia

#10 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

In reply to Kees Cook
Quoted 21 lines · Kees Cook
Quoted 20 lines · Joe Perches
Quoted 8 lines · Julia Lawall

On Thu, 23 Aug 2018, Kees Cook wrote:

(a + b) * c

It will consider a pattern with the parentheses removed, but that pattern won't match anything, because real trees that consist of a + b * c are parsed in a different way.

spatch 1.0.4 doesn't seem to:

$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@
Quoted 3 lines
Quoted 1 line

* d = a * b + c;

But "(a * b) + c" for the rule does:

Yes, but not for other valid uses like below:

$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@

* d = (a * b) + c;

$ cat test_mul.c 
int a, b, c, d;

void foo(void)

{
	d = ((a * b) + c);
	d = ((a * b)) + c;
	d = (a * b) + c;
	d = a * b + c;
}
$ spatch -sp-file match_mul.cocci test_mul.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: test_mul.c
diff = 
--- test_mul.c
+++ /tmp/cocci-output-23856-7e83f7-test_mul.c
@@ -4,6 +4,4 @@ void foo(void)
 {
 	d = ((a * b) + c);
 	d = ((a * b)) + c;
-	d = (a * b) + c;
-	d = a * b + c;
 }
#11 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

Quoted 52 lines · Joe Perches
Quoted 21 lines · Kees Cook
Quoted 20 lines · Joe Perches
Quoted 8 lines · Julia Lawall

On Thu, 23 Aug 2018, Kees Cook wrote:

(a + b) * c

It will consider a pattern with the parentheses removed, but that pattern won't match anything, because real trees that consist of a + b * c are parsed in a different way.

spatch 1.0.4 doesn't seem to:

$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@
Quoted 3 lines
Quoted 1 line

* d = a * b + c;

But "(a * b) + c" for the rule does:

Yes, but not for other valid uses like below:

$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@

* d = (a * b) + c;

$ cat test_mul.c
int a, b, c, d;

void foo(void)

{
	d = ((a * b) + c);
	d = ((a * b)) + c;
	d = (a * b) + c;
	d = a * b + c;
}
$ spatch -sp-file match_mul.cocci test_mul.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: test_mul.c
diff =

tmp/cocci-output-23856-7e83f7-test_mul.c void foo(void) · line 7

Quoted 6 lines
tmp/cocci-output-23856-7e83f7-test_mul.c renamed from test_mul.c +0 -2
@@ -4,6 +4,4 @@ void foo(void)
 {
 	d = ((a * b) + c);
 	d = ((a * b)) + c;
-	d = (a * b) + c;
-	d = a * b + c;
 }

I'm not sure to get the point.

If you think that people may fully parenthesize the code, then ((a * b) +
c) would be a reasonable option for the pattern. I'm not sure why double parentheses, ((a * b)) + c, should exist in the first place.

julia

#12 lore raw

PATCH 1/1 rtc: sun6i: Use struct_size() in kzalloc()]

Quoted 64 lines · Julia Lawall
Quoted 52 lines · Joe Perches
Quoted 21 lines · Kees Cook
Quoted 20 lines · Joe Perches
Quoted 8 lines · Julia Lawall

On Thu, 23 Aug 2018, Kees Cook wrote:

(a + b) * c

It will consider a pattern with the parentheses removed, but that pattern won't match anything, because real trees that consist of a + b * c are parsed in a different way.

spatch 1.0.4 doesn't seem to:

$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@
Quoted 3 lines
Quoted 1 line

* d = a * b + c;

But "(a * b) + c" for the rule does:

Yes, but not for other valid uses like below:

$ cat match_mul.cocci
@@
expression a, b, c;
int d;
@@

* d = (a * b) + c;

$ cat test_mul.c
int a, b, c, d;

void foo(void)

{
	d = ((a * b) + c);
	d = ((a * b)) + c;
	d = (a * b) + c;
	d = a * b + c;
}
$ spatch -sp-file match_mul.cocci test_mul.c
init_defs_builtins: /usr/lib/coccinelle/standard.h
HANDLING: test_mul.c
diff =

tmp/cocci-output-23856-7e83f7-test_mul.c void foo(void) · line 7

Quoted 6 lines
tmp/cocci-output-23856-7e83f7-test_mul.c renamed from test_mul.c +0 -2
@@ -4,6 +4,4 @@ void foo(void)
 {
 	d = ((a * b) + c);
 	d = ((a * b)) + c;
-	d = (a * b) + c;
-	d = a * b + c;
 }

I'm not sure to get the point.

Quoted 3 lines

If you think that people may fully parenthesize the code, then ((a * b) +
c) would be a reasonable option for the pattern. I'm not sure why double parentheses, ((a * b)) + c, should exist in the first place.

People who write code do a lot of unnecessary things. It's code I've seen in the past.
It occurs more with expressions than identifiers.

It'd be nice if coccinelle could parse arbitrarily parenthesized code and script down to its minimal logical requirements and match as maximally as possible.

cheers, Joe

maintainer Applied
#15 lore raw
In reply to Gustavo A. R. Silva
Quoted 14 lines · Gustavo A. R. Silva

One of the more common cases of allocation size calculations is finding the size of a structure that has a zero-sized array at the end, along with memory for some number of elements for that array. For example:

struct foo { 
	int stuff;
        void *entry[];
};

instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);

Instead of leaving these open-coded and prone to type mistakes, we can now use the new struct_size() helper:

Quoted 7 lines

instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);

---
 drivers/rtc/rtc-sun6i.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Applied, thanks.

Signature · 3 lines
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com