* [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code
@ 2015-10-11 20:27 ` Christophe JAILLET
0 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-11 20:27 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: linuxppc-dev, linux-kernel, kernel-janitors, Christophe JAILLET
of_get_next_parent can be used to simplify the while() loop and
avoid the need of a temp variable.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index f4f0301..5732926 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -13,7 +13,6 @@
unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
{
- struct device_node *np;
const unsigned int *p_bus_freq = NULL;
of_node_get(node);
@@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
if (p_bus_freq)
break;
- np = of_get_parent(node);
- of_node_put(node);
- node = np;
+ node = of_get_next_parent(node);
}
of_node_put(node);
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code
@ 2015-10-11 20:27 ` Christophe JAILLET
0 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-11 20:27 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: linuxppc-dev, linux-kernel, kernel-janitors, Christophe JAILLET
of_get_next_parent can be used to simplify the while() loop and
avoid the need of a temp variable.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index f4f0301..5732926 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -13,7 +13,6 @@
unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
{
- struct device_node *np;
const unsigned int *p_bus_freq = NULL;
of_node_get(node);
@@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
if (p_bus_freq)
break;
- np = of_get_parent(node);
- of_node_put(node);
- node = np;
+ node = of_get_next_parent(node);
}
of_node_put(node);
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code
2015-10-11 20:27 ` Christophe JAILLET
@ 2015-10-11 20:44 ` Julia Lawall
-1 siblings, 0 replies; 27+ messages in thread
From: Julia Lawall @ 2015-10-11 20:44 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, mpe, linuxppc-dev, linux-kernel, kernel-janitors
On Sun, 11 Oct 2015, Christophe JAILLET wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
Can you do something with the loop in __of_translate_address, in
drivers/of/address.c? Is there not an iterator for this?
julia
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> index f4f0301..5732926 100644
> --- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
> +++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> @@ -13,7 +13,6 @@
>
> unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> {
> - struct device_node *np;
> const unsigned int *p_bus_freq = NULL;
>
> of_node_get(node);
> @@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> if (p_bus_freq)
> break;
>
> - np = of_get_parent(node);
> - of_node_put(node);
> - node = np;
> + node = of_get_next_parent(node);
> }
> of_node_put(node);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code
@ 2015-10-11 20:44 ` Julia Lawall
0 siblings, 0 replies; 27+ messages in thread
From: Julia Lawall @ 2015-10-11 20:44 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, mpe, linuxppc-dev, linux-kernel, kernel-janitors
On Sun, 11 Oct 2015, Christophe JAILLET wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
Can you do something with the loop in __of_translate_address, in
drivers/of/address.c? Is there not an iterator for this?
julia
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> index f4f0301..5732926 100644
> --- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
> +++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> @@ -13,7 +13,6 @@
>
> unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> {
> - struct device_node *np;
> const unsigned int *p_bus_freq = NULL;
>
> of_node_get(node);
> @@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> if (p_bus_freq)
> break;
>
> - np = of_get_parent(node);
> - of_node_put(node);
> - node = np;
> + node = of_get_next_parent(node);
> }
> of_node_put(node);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code
2015-10-11 20:44 ` Julia Lawall
@ 2015-10-12 5:05 ` Christophe JAILLET
-1 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-12 5:05 UTC (permalink / raw)
To: linux-kernel; +Cc: linuxppc-dev, kernel-janitors
Le 11/10/2015 22:44, Julia Lawall a écrit :
>
>> of_get_next_parent can be used to simplify the while() loop and
>> avoid the need of a temp variable.
> Can you do something with the loop in __of_translate_address, in
> drivers/of/address.c? Is there not an iterator for this?
>
> julia
>
Hi Julia,
There does not seem to be any 'for_each_parent_of_node' or equivalent.
Best regards,
CJ
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code
@ 2015-10-12 5:05 ` Christophe JAILLET
0 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-12 5:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: linux-kernel, kernel-janitors
Le 11/10/2015 22:44, Julia Lawall a écrit :
>
>> of_get_next_parent can be used to simplify the while() loop and
>> avoid the need of a temp variable.
> Can you do something with the loop in __of_translate_address, in
> drivers/of/address.c? Is there not an iterator for this?
>
> julia
>
Hi Julia,
There does not seem to be any 'for_each_parent_of_node' or equivalent.
Best regards,
CJ
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code
2015-10-11 20:27 ` Christophe JAILLET
@ 2015-10-14 4:00 ` Michael Ellerman
-1 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-14 4:00 UTC (permalink / raw)
To: Christophe Jaillet, benh, paulus
Cc: kernel-janitors, Christophe JAILLET, linuxppc-dev, linux-kernel
On Sun, 2015-11-10 at 20:27:40 UTC, Christophe Jaillet wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> index f4f0301..5732926 100644
> --- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
> +++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> @@ -13,7 +13,6 @@
>
> unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> {
> - struct device_node *np;
> const unsigned int *p_bus_freq = NULL;
>
> of_node_get(node);
> @@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> if (p_bus_freq)
> break;
>
> - np = of_get_parent(node);
> - of_node_put(node);
> - node = np;
> + node = of_get_next_parent(node);
> }
> of_node_put(node);
This conversion is OK, but the logic in the function is still wrong.
It uses of_get_property() inside the loop, but then drops the reference to the
node before dereferencing the p_bus_freq pointer, which could by then point to
junk if the node has been freed.
Instead it should use of_property_read_u32() to actually read the property
value before dropping the reference.
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code
@ 2015-10-14 4:00 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-14 4:00 UTC (permalink / raw)
To: Christophe Jaillet, benh, paulus
Cc: kernel-janitors, linuxppc-dev, linux-kernel
On Sun, 2015-11-10 at 20:27:40 UTC, Christophe Jaillet wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> arch/powerpc/sysdev/mpc5xxx_clocks.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> index f4f0301..5732926 100644
> --- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
> +++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> @@ -13,7 +13,6 @@
>
> unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> {
> - struct device_node *np;
> const unsigned int *p_bus_freq = NULL;
>
> of_node_get(node);
> @@ -22,9 +21,7 @@ unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
> if (p_bus_freq)
> break;
>
> - np = of_get_parent(node);
> - of_node_put(node);
> - node = np;
> + node = of_get_next_parent(node);
> }
> of_node_put(node);
This conversion is OK, but the logic in the function is still wrong.
It uses of_get_property() inside the loop, but then drops the reference to the
node before dereferencing the p_bus_freq pointer, which could by then point to
junk if the node has been freed.
Instead it should use of_property_read_u32() to actually read the property
value before dropping the reference.
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
2015-10-14 4:00 ` Michael Ellerman
@ 2015-10-15 5:56 ` Christophe JAILLET
-1 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-15 5:56 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: linuxppc-dev, linux-kernel, kernel-janitors, Christophe JAILLET
Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
dereference in order to avoid access to potentially freed memory.
Use 'of_get_next_parent()' to simplify the while() loop and avoid the
need of a temp variable.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
*** Untested ***
---
arch/powerpc/sysdev/mpc5xxx_clocks.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index f4f0301..92fbcf8 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -13,21 +13,17 @@
unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
{
- struct device_node *np;
- const unsigned int *p_bus_freq = NULL;
+ u32 bus_freq = 0;
of_node_get(node);
while (node) {
- p_bus_freq = of_get_property(node, "bus-frequency", NULL);
- if (p_bus_freq)
+ if (!of_property_read_u32(node, "bus-frequency", &bus_freq))
break;
- np = of_get_parent(node);
- of_node_put(node);
- node = np;
+ node = of_get_next_parent(node);
}
of_node_put(node);
- return p_bus_freq ? *p_bus_freq : 0;
+ return bus_freq;
}
EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-15 5:56 ` Christophe JAILLET
0 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-15 5:56 UTC (permalink / raw)
To: benh, paulus, mpe
Cc: linuxppc-dev, linux-kernel, kernel-janitors, Christophe JAILLET
Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
dereference in order to avoid access to potentially freed memory.
Use 'of_get_next_parent()' to simplify the while() loop and avoid the
need of a temp variable.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
*** Untested ***
---
arch/powerpc/sysdev/mpc5xxx_clocks.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index f4f0301..92fbcf8 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -13,21 +13,17 @@
unsigned long mpc5xxx_get_bus_frequency(struct device_node *node)
{
- struct device_node *np;
- const unsigned int *p_bus_freq = NULL;
+ u32 bus_freq = 0;
of_node_get(node);
while (node) {
- p_bus_freq = of_get_property(node, "bus-frequency", NULL);
- if (p_bus_freq)
+ if (!of_property_read_u32(node, "bus-frequency", &bus_freq))
break;
- np = of_get_parent(node);
- of_node_put(node);
- node = np;
+ node = of_get_next_parent(node);
}
of_node_put(node);
- return p_bus_freq ? *p_bus_freq : 0;
+ return bus_freq;
}
EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
--
2.1.4
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
2015-10-15 5:56 ` Christophe JAILLET
(?)
@ 2015-10-15 6:36 ` Michael Ellerman
-1 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-15 6:36 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> dereference in order to avoid access to potentially freed memory.
>
> Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> need of a temp variable.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> *** Untested ***
Thanks.
Can someone with an mpc5xxx test this?
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-15 6:36 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-15 6:36 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> dereference in order to avoid access to potentially freed memory.
>
> Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> need of a temp variable.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> *** Untested ***
Thanks.
Can someone with an mpc5xxx test this?
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-15 6:36 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-15 6:36 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> dereference in order to avoid access to potentially freed memory.
>
> Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> need of a temp variable.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> *** Untested ***
Thanks.
Can someone with an mpc5xxx test this?
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code
2015-10-11 20:27 ` Christophe JAILLET
@ 2015-10-15 11:11 ` Michael Ellerman
-1 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-15 11:11 UTC (permalink / raw)
To: Christophe Jaillet, benh, paulus
Cc: kernel-janitors, Christophe JAILLET, linuxppc-dev, linux-kernel
On Sun, 2015-11-10 at 20:27:40 UTC, Christophe Jaillet wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/b340587e68b479e52039f800
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: powerpc/mpc5xxx: Use of_get_next_parent to simplify code
@ 2015-10-15 11:11 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-15 11:11 UTC (permalink / raw)
To: Christophe Jaillet, benh, paulus
Cc: kernel-janitors, linuxppc-dev, linux-kernel
On Sun, 2015-11-10 at 20:27:40 UTC, Christophe Jaillet wrote:
> of_get_next_parent can be used to simplify the while() loop and
> avoid the need of a temp variable.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/b340587e68b479e52039f800
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
2015-10-15 6:36 ` Michael Ellerman
@ 2015-10-16 6:20 ` Christophe JAILLET
-1 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-16 6:20 UTC (permalink / raw)
To: Michael Ellerman
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
Le 15/10/2015 08:36, Michael Ellerman a écrit :
> On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
>> Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
>> dereference in order to avoid access to potentially freed memory.
>>
>> Use 'of_get_next_parent()' to simplify the while() loop and avoid the
>> need of a temp variable.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
>> *** Untested ***
> Thanks.
>
> Can someone with an mpc5xxx test this?
>
> cheers
>
Hi,
I don't think it is an issue, but while looking at another similar
patch, I noticed that the proposed patch adds a call to be32_to_cpup()
(within of_property_read_u32).
Apparently, powerPC is a BE architecture, so this call should be a no-op.
Just wanted to point it out, in case of.
Best regards,
CJ
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-16 6:20 ` Christophe JAILLET
0 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-16 6:20 UTC (permalink / raw)
To: Michael Ellerman
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
Le 15/10/2015 08:36, Michael Ellerman a écrit :
> On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
>> Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
>> dereference in order to avoid access to potentially freed memory.
>>
>> Use 'of_get_next_parent()' to simplify the while() loop and avoid the
>> need of a temp variable.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
>> *** Untested ***
> Thanks.
>
> Can someone with an mpc5xxx test this?
>
> cheers
>
Hi,
I don't think it is an issue, but while looking at another similar
patch, I noticed that the proposed patch adds a call to be32_to_cpup()
(within of_property_read_u32).
Apparently, powerPC is a BE architecture, so this call should be a no-op.
Just wanted to point it out, in case of.
Best regards,
CJ
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
2015-10-16 6:20 ` Christophe JAILLET
@ 2015-10-16 7:15 ` Gabriel Paubert
-1 siblings, 0 replies; 27+ messages in thread
From: Gabriel Paubert @ 2015-10-16 7:15 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Michael Ellerman, kernel-janitors, paulus, linuxppc-dev, linux-kernel
On Fri, Oct 16, 2015 at 08:20:13AM +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> >On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> >>Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> >>dereference in order to avoid access to potentially freed memory.
> >>
> >>Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> >>need of a temp variable.
> >>
> >>Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >>---
> >>v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> >>*** Untested ***
> >Thanks.
> >
> >Can someone with an mpc5xxx test this?
> >
> >cheers
> >
>
> Hi,
> I don't think it is an issue, but while looking at another similar
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup() (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no-op.
Sadly no more. 32 bit is BE only, but 64 bit can be either BEtter or
LEsser.
Gabriel
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-16 7:15 ` Gabriel Paubert
0 siblings, 0 replies; 27+ messages in thread
From: Gabriel Paubert @ 2015-10-16 7:15 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Michael Ellerman, kernel-janitors, paulus, linuxppc-dev, linux-kernel
On Fri, Oct 16, 2015 at 08:20:13AM +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> >On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> >>Use 'of_property_read_u32()' instead of 'of_get_property()'+pointer
> >>dereference in order to avoid access to potentially freed memory.
> >>
> >>Use 'of_get_next_parent()' to simplify the while() loop and avoid the
> >>need of a temp variable.
> >>
> >>Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >>---
> >>v2: Use of_property_read_u32 instead of of_get_property+pointer dereference
> >>*** Untested ***
> >Thanks.
> >
> >Can someone with an mpc5xxx test this?
> >
> >cheers
> >
>
> Hi,
> I don't think it is an issue, but while looking at another similar
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup() (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no-op.
Sadly no more. 32 bit is BE only, but 64 bit can be either BEtter or
LEsser.
Gabriel
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
2015-10-16 6:20 ` Christophe JAILLET
(?)
@ 2015-10-16 9:49 ` Michael Ellerman
-1 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-16 9:49 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> > On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> > > Use 'of_property_read_u32()' instead of
> > > 'of_get_property()'+pointer
> > > dereference in order to avoid access to potentially freed memory.
> > >
> > > Use 'of_get_next_parent()' to simplify the while() loop and avoid
> > > the
> > > need of a temp variable.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > v2: Use of_property_read_u32 instead of of_get_property+pointer
> > > dereference
> > > *** Untested ***
> > Thanks.
> >
> > Can someone with an mpc5xxx test this?
>
> Hi,
> I don't think it is an issue, but while looking at another similar
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup()
> (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no
> -op.
>
> Just wanted to point it out, in case of.
Hi Christoph,
I'm not sure I follow.
The device tree is always big endian, but of_property_read_u32() does
the
conversion to CPU endian for you already. That is one of the advantages
of
using it.
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-16 9:49 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-16 9:49 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> > On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> > > Use 'of_property_read_u32()' instead of
> > > 'of_get_property()'+pointer
> > > dereference in order to avoid access to potentially freed memory.
> > >
> > > Use 'of_get_next_parent()' to simplify the while() loop and avoid
> > > the
> > > need of a temp variable.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > v2: Use of_property_read_u32 instead of of_get_property+pointer
> > > dereference
> > > *** Untested ***
> > Thanks.
> >
> > Can someone with an mpc5xxx test this?
>
> Hi,
> I don't think it is an issue, but while looking at another similar
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup()
> (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no
> -op.
>
> Just wanted to point it out, in case of.
Hi Christoph,
I'm not sure I follow.
The device tree is always big endian, but of_property_read_u32() does
the
conversion to CPU endian for you already. That is one of the advantages
of
using it.
cheers
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-16 9:49 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-16 9:49 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
> Le 15/10/2015 08:36, Michael Ellerman a écrit :
> > On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
> > > Use 'of_property_read_u32()' instead of
> > > 'of_get_property()'+pointer
> > > dereference in order to avoid access to potentially freed memory.
> > >
> > > Use 'of_get_next_parent()' to simplify the while() loop and avoid
> > > the
> > > need of a temp variable.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > > v2: Use of_property_read_u32 instead of of_get_property+pointer
> > > dereference
> > > *** Untested ***
> > Thanks.
> >
> > Can someone with an mpc5xxx test this?
>
> Hi,
> I don't think it is an issue, but while looking at another similar
> patch, I noticed that the proposed patch adds a call to
> be32_to_cpup()
> (within of_property_read_u32).
> Apparently, powerPC is a BE architecture, so this call should be a no
> -op.
>
> Just wanted to point it out, in case of.
Hi Christoph,
I'm not sure I follow.
The device tree is always big endian, but of_property_read_u32() does
the
conversion to CPU endian for you already. That is one of the advantages
of
using it.
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
2015-10-16 9:49 ` Michael Ellerman
@ 2015-10-16 20:05 ` Christophe JAILLET
-1 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-16 20:05 UTC (permalink / raw)
To: Michael Ellerman
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
Le 16/10/2015 11:49, Michael Ellerman a écrit :
> On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
>> Le 15/10/2015 08:36, Michael Ellerman a écrit :
>>> On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
>>>> Use 'of_property_read_u32()' instead of
>>>> 'of_get_property()'+pointer
>>>> dereference in order to avoid access to potentially freed memory.
>>>>
>>>> Use 'of_get_next_parent()' to simplify the while() loop and avoid
>>>> the
>>>> need of a temp variable.
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> ---
>>>> v2: Use of_property_read_u32 instead of of_get_property+pointer
>>>> dereference
>>>> *** Untested ***
>>> Thanks.
>>>
>>> Can someone with an mpc5xxx test this?
>> Hi,
>> I don't think it is an issue, but while looking at another similar
>> patch, I noticed that the proposed patch adds a call to
>> be32_to_cpup()
>> (within of_property_read_u32).
>> Apparently, powerPC is a BE architecture, so this call should be a no
>> -op.
>>
>> Just wanted to point it out, in case of.
> Hi Christoph,
>
> I'm not sure I follow.
>
> The device tree is always big endian, but of_property_read_u32() does
> the
> conversion to CPU endian for you already. That is one of the advantages
> of
> using it.
>
> cheers
>
Hi,
sorry if un-clear.
What I mean is that in the patch related
'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
So in the proposed patch, 'of_property_read_u32' adds it.
While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was
called explicitly.
So using 'of_property_read_u32' keep the same logic.
Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were
written the same way. I found spurious that a call to 'be32_to_cpup' was
done in only one case.
Maybe, it was a missing in 'mpc5xxx_clocks.c'.
I don't know if it can be an issue or not. I just find it 'strange'.
CJ
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-16 20:05 ` Christophe JAILLET
0 siblings, 0 replies; 27+ messages in thread
From: Christophe JAILLET @ 2015-10-16 20:05 UTC (permalink / raw)
To: Michael Ellerman
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
Le 16/10/2015 11:49, Michael Ellerman a écrit :
> On Fri, 2015-10-16 at 08:20 +0200, Christophe JAILLET wrote:
>> Le 15/10/2015 08:36, Michael Ellerman a écrit :
>>> On Thu, 2015-10-15 at 07:56 +0200, Christophe JAILLET wrote:
>>>> Use 'of_property_read_u32()' instead of
>>>> 'of_get_property()'+pointer
>>>> dereference in order to avoid access to potentially freed memory.
>>>>
>>>> Use 'of_get_next_parent()' to simplify the while() loop and avoid
>>>> the
>>>> need of a temp variable.
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> ---
>>>> v2: Use of_property_read_u32 instead of of_get_property+pointer
>>>> dereference
>>>> *** Untested ***
>>> Thanks.
>>>
>>> Can someone with an mpc5xxx test this?
>> Hi,
>> I don't think it is an issue, but while looking at another similar
>> patch, I noticed that the proposed patch adds a call to
>> be32_to_cpup()
>> (within of_property_read_u32).
>> Apparently, powerPC is a BE architecture, so this call should be a no
>> -op.
>>
>> Just wanted to point it out, in case of.
> Hi Christoph,
>
> I'm not sure I follow.
>
> The device tree is always big endian, but of_property_read_u32() does
> the
> conversion to CPU endian for you already. That is one of the advantages
> of
> using it.
>
> cheers
>
Hi,
sorry if un-clear.
What I mean is that in the patch related
'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
So in the proposed patch, 'of_property_read_u32' adds it.
While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was
called explicitly.
So using 'of_property_read_u32' keep the same logic.
Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were
written the same way. I found spurious that a call to 'be32_to_cpup' was
done in only one case.
Maybe, it was a missing in 'mpc5xxx_clocks.c'.
I don't know if it can be an issue or not. I just find it 'strange'.
CJ
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
2015-10-16 20:05 ` Christophe JAILLET
(?)
@ 2015-10-19 4:38 ` Michael Ellerman
-1 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-19 4:38 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Fri, 2015-10-16 at 22:05 +0200, Christophe JAILLET wrote:
> Hi,
> sorry if un-clear.
>
> What I mean is that in the patch related
> 'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
> So in the proposed patch, 'of_property_read_u32' adds it.
>
> While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was
> called explicitly.
> So using 'of_property_read_u32' keep the same logic.
Ah right, I understand now.
> Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were
> written the same way. I found spurious that a call to 'be32_to_cpup' was
> done in only one case.
> Maybe, it was a missing in 'mpc5xxx_clocks.c'.
Yes it was missing in that code.
But that's not a real bug because that code only ever runs on BE systems.
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-19 4:38 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-19 4:38 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Fri, 2015-10-16 at 22:05 +0200, Christophe JAILLET wrote:
> Hi,
> sorry if un-clear.
>
> What I mean is that in the patch related
> 'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
> So in the proposed patch, 'of_property_read_u32' adds it.
>
> While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was
> called explicitly.
> So using 'of_property_read_u32' keep the same logic.
Ah right, I understand now.
> Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were
> written the same way. I found spurious that a call to 'be32_to_cpup' was
> done in only one case.
> Maybe, it was a missing in 'mpc5xxx_clocks.c'.
Yes it was missing in that code.
But that's not a real bug because that code only ever runs on BE systems.
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory
@ 2015-10-19 4:38 ` Michael Ellerman
0 siblings, 0 replies; 27+ messages in thread
From: Michael Ellerman @ 2015-10-19 4:38 UTC (permalink / raw)
To: Christophe JAILLET
Cc: benh, paulus, linuxppc-dev, linux-kernel, kernel-janitors
On Fri, 2015-10-16 at 22:05 +0200, Christophe JAILLET wrote:
> Hi,
> sorry if un-clear.
>
> What I mean is that in the patch related
> 'powerpc/sysdev/mpc5xxx_clocks.c', there was no call to 'be32_to_cpup'.
> So in the proposed patch, 'of_property_read_u32' adds it.
>
> While in the patch against 'powerpc/kernel/prom.c', 'be32_to_cpup' was
> called explicitly.
> So using 'of_property_read_u32' keep the same logic.
Ah right, I understand now.
> Basically the code from 'mpc5xxx_clocks.c' and from 'prom.c' were
> written the same way. I found spurious that a call to 'be32_to_cpup' was
> done in only one case.
> Maybe, it was a missing in 'mpc5xxx_clocks.c'.
Yes it was missing in that code.
But that's not a real bug because that code only ever runs on BE systems.
cheers
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2015-10-19 4:38 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-11 20:27 [PATCH] powerpc/mpc5xxx: Use of_get_next_parent to simplify code Christophe JAILLET
2015-10-11 20:27 ` Christophe JAILLET
2015-10-11 20:44 ` Julia Lawall
2015-10-11 20:44 ` Julia Lawall
2015-10-12 5:05 ` Christophe JAILLET
2015-10-12 5:05 ` Christophe JAILLET
2015-10-14 4:00 ` Michael Ellerman
2015-10-14 4:00 ` Michael Ellerman
2015-10-15 5:56 ` [PATCH v2] powerpc/mpc5xxx: Avoid dereferencing potentially freed memory Christophe JAILLET
2015-10-15 5:56 ` Christophe JAILLET
2015-10-15 6:36 ` Michael Ellerman
2015-10-15 6:36 ` Michael Ellerman
2015-10-15 6:36 ` Michael Ellerman
2015-10-16 6:20 ` Christophe JAILLET
2015-10-16 6:20 ` Christophe JAILLET
2015-10-16 7:15 ` Gabriel Paubert
2015-10-16 7:15 ` Gabriel Paubert
2015-10-16 9:49 ` Michael Ellerman
2015-10-16 9:49 ` Michael Ellerman
2015-10-16 9:49 ` Michael Ellerman
2015-10-16 20:05 ` Christophe JAILLET
2015-10-16 20:05 ` Christophe JAILLET
2015-10-19 4:38 ` Michael Ellerman
2015-10-19 4:38 ` Michael Ellerman
2015-10-19 4:38 ` Michael Ellerman
2015-10-15 11:11 ` powerpc/mpc5xxx: Use of_get_next_parent to simplify code Michael Ellerman
2015-10-15 11:11 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.