[PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[Qi Hou]

When more than one GP timers are used as kernel system timers and the
corresponding nodes in device-tree are marked with the same "disabled"
property, then the "attr" field of the property will be initialized
more than once as the property being added to sys file system via
__of_add_property_sysfs().

In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
directly to the return value of safe_name(), without taking care of
whether it's already a valid pointer to a memory block. If it is, its
old value will always be overwritten by the new one and the memory block
allocated before will a "ghost", then a kmemleak happened.

That the same "disabled" property being added to different nodes of device
tree would cause that kind of kmemleak overhead, at leat once.

To fix it, allocate the property dynamically, and delete static one.

Signed-off-by: Qi Hou <qi.hou@windriver.com>
---
 arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index ece09c9..206ae8d 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
 	.tick_resume		= omap2_gp_timer_shutdown,
 };
 
-static struct property device_disabled = {
-	.name = "status",
-	.length = sizeof("disabled"),
-	.value = "disabled",
-};
-
 static const struct of_device_id omap_timer_match[] __initconst = {
 	{ .compatible = "ti,omap2420-timer", },
 	{ .compatible = "ti,omap3430-timer", },
@@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
 				  of_get_property(np, "ti,timer-secure", NULL)))
 			continue;
 
-		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
-			of_add_property(np, &device_disabled);
+		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
+			struct property *prop;
+
+			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+			if (!prop)
+				return NULL;
+			prop->name = "status";
+			prop->length = sizeof("disabled");
+			prop->value = "disabled";
+			of_add_property(np, prop);
+		}
 		return np;
 	}
 
-- 
2.7.4

[PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[Qi Hou]

When more than one GP timers are used as kernel system timers and the
corresponding nodes in device-tree are marked with the same "disabled"
property, then the "attr" field of the property will be initialized
more than once as the property being added to sys file system via
__of_add_property_sysfs().

In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
directly to the return value of safe_name(), without taking care of
whether it's already a valid pointer to a memory block. If it is, its
old value will always be overwritten by the new one and the memory block
allocated before will a "ghost", then a kmemleak happened.

That the same "disabled" property being added to different nodes of device
tree would cause that kind of kmemleak overhead, at leat once.

To fix it, allocate the property dynamically, and delete static one.

Signed-off-by: Qi Hou <qi.hou@windriver.com>
---
 arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index ece09c9..206ae8d 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
 	.tick_resume		= omap2_gp_timer_shutdown,
 };
 
-static struct property device_disabled = {
-	.name = "status",
-	.length = sizeof("disabled"),
-	.value = "disabled",
-};
-
 static const struct of_device_id omap_timer_match[] __initconst = {
 	{ .compatible = "ti,omap2420-timer", },
 	{ .compatible = "ti,omap3430-timer", },
@@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
 				  of_get_property(np, "ti,timer-secure", NULL)))
 			continue;
 
-		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
-			of_add_property(np, &device_disabled);
+		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
+			struct property *prop;
+
+			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+			if (!prop)
+				return NULL;
+			prop->name = "status";
+			prop->length = sizeof("disabled");
+			prop->value = "disabled";
+			of_add_property(np, prop);
+		}
 		return np;
 	}
 
-- 
2.7.4

[PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[Qi Hou]

When more than one GP timers are used as kernel system timers and the
corresponding nodes in device-tree are marked with the same "disabled"
property, then the "attr" field of the property will be initialized
more than once as the property being added to sys file system via
__of_add_property_sysfs().

In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
directly to the return value of safe_name(), without taking care of
whether it's already a valid pointer to a memory block. If it is, its
old value will always be overwritten by the new one and the memory block
allocated before will a "ghost", then a kmemleak happened.

That the same "disabled" property being added to different nodes of device
tree would cause that kind of kmemleak overhead, at leat once.

To fix it, allocate the property dynamically, and delete static one.

Signed-off-by: Qi Hou <qi.hou@windriver.com>
---
 arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index ece09c9..206ae8d 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
 	.tick_resume		= omap2_gp_timer_shutdown,
 };
 
-static struct property device_disabled = {
-	.name = "status",
-	.length = sizeof("disabled"),
-	.value = "disabled",
-};
-
 static const struct of_device_id omap_timer_match[] __initconst = {
 	{ .compatible = "ti,omap2420-timer", },
 	{ .compatible = "ti,omap3430-timer", },
@@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
 				  of_get_property(np, "ti,timer-secure", NULL)))
 			continue;
 
-		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
-			of_add_property(np, &device_disabled);
+		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
+			struct property *prop;
+
+			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+			if (!prop)
+				return NULL;
+			prop->name = "status";
+			prop->length = sizeof("disabled");
+			prop->value = "disabled";
+			of_add_property(np, prop);
+		}
 		return np;
 	}
 
-- 
2.7.4

Re: [PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[Ladislav Michl]

On Tue, Jan 09, 2018 at 05:24:21PM +0800, Qi Hou wrote:
> When more than one GP timers are used as kernel system timers and the
> corresponding nodes in device-tree are marked with the same "disabled"
> property, then the "attr" field of the property will be initialized
> more than once as the property being added to sys file system via
> __of_add_property_sysfs().
> 
> In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
> directly to the return value of safe_name(), without taking care of
> whether it's already a valid pointer to a memory block. If it is, its
> old value will always be overwritten by the new one and the memory block
> allocated before will a "ghost", then a kmemleak happened.

As timers does not seem to be deallocated, this does not matter in practice.
Fix eats a bit more from heap.

> That the same "disabled" property being added to different nodes of device
> tree would cause that kind of kmemleak overhead, at leat once.
> 
> To fix it, allocate the property dynamically, and delete static one.
>
> Signed-off-by: Qi Hou <qi.hou@windriver.com>
> ---
>  arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index ece09c9..206ae8d 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
>  	.tick_resume		= omap2_gp_timer_shutdown,
>  };
>  
> -static struct property device_disabled = {
> -	.name = "status",
> -	.length = sizeof("disabled"),
> -	.value = "disabled",
> -};
> -
>  static const struct of_device_id omap_timer_match[] __initconst = {
>  	{ .compatible = "ti,omap2420-timer", },
>  	{ .compatible = "ti,omap3430-timer", },
> @@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
>  				  of_get_property(np, "ti,timer-secure", NULL)))
>  			continue;
>  
> -		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
> -			of_add_property(np, &device_disabled);
> +		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
> +			struct property *prop;
> +
> +			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +			if (!prop)
> +				return NULL;
> +			prop->name = "status";
> +			prop->length = sizeof("disabled");
> +			prop->value = "disabled";

How about (see drivers/of/unittest.c)?
prop->value = "disabled";
prop->length = strlen(prop->value);

> +			of_add_property(np, prop);
> +		}
>  		return np;
>  	}
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[Ladislav Michl]

On Tue, Jan 09, 2018 at 05:24:21PM +0800, Qi Hou wrote:
> When more than one GP timers are used as kernel system timers and the
> corresponding nodes in device-tree are marked with the same "disabled"
> property, then the "attr" field of the property will be initialized
> more than once as the property being added to sys file system via
> __of_add_property_sysfs().
> 
> In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
> directly to the return value of safe_name(), without taking care of
> whether it's already a valid pointer to a memory block. If it is, its
> old value will always be overwritten by the new one and the memory block
> allocated before will a "ghost", then a kmemleak happened.

As timers does not seem to be deallocated, this does not matter in practice.
Fix eats a bit more from heap.

> That the same "disabled" property being added to different nodes of device
> tree would cause that kind of kmemleak overhead, at leat once.
> 
> To fix it, allocate the property dynamically, and delete static one.
>
> Signed-off-by: Qi Hou <qi.hou@windriver.com>
> ---
>  arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index ece09c9..206ae8d 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
>  	.tick_resume		= omap2_gp_timer_shutdown,
>  };
>  
> -static struct property device_disabled = {
> -	.name = "status",
> -	.length = sizeof("disabled"),
> -	.value = "disabled",
> -};
> -
>  static const struct of_device_id omap_timer_match[] __initconst = {
>  	{ .compatible = "ti,omap2420-timer", },
>  	{ .compatible = "ti,omap3430-timer", },
> @@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
>  				  of_get_property(np, "ti,timer-secure", NULL)))
>  			continue;
>  
> -		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
> -			of_add_property(np, &device_disabled);
> +		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
> +			struct property *prop;
> +
> +			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +			if (!prop)
> +				return NULL;
> +			prop->name = "status";
> +			prop->length = sizeof("disabled");
> +			prop->value = "disabled";

How about (see drivers/of/unittest.c)?
prop->value = "disabled";
prop->length = strlen(prop->value);

> +			of_add_property(np, prop);
> +		}
>  		return np;
>  	}
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[Ladislav Michl]

On Tue, Jan 09, 2018 at 05:24:21PM +0800, Qi Hou wrote:
> When more than one GP timers are used as kernel system timers and the
> corresponding nodes in device-tree are marked with the same "disabled"
> property, then the "attr" field of the property will be initialized
> more than once as the property being added to sys file system via
> __of_add_property_sysfs().
> 
> In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
> directly to the return value of safe_name(), without taking care of
> whether it's already a valid pointer to a memory block. If it is, its
> old value will always be overwritten by the new one and the memory block
> allocated before will a "ghost", then a kmemleak happened.

As timers does not seem to be deallocated, this does not matter in practice.
Fix eats a bit more from heap.

> That the same "disabled" property being added to different nodes of device
> tree would cause that kind of kmemleak overhead, at leat once.
> 
> To fix it, allocate the property dynamically, and delete static one.
>
> Signed-off-by: Qi Hou <qi.hou@windriver.com>
> ---
>  arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index ece09c9..206ae8d 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
>  	.tick_resume		= omap2_gp_timer_shutdown,
>  };
>  
> -static struct property device_disabled = {
> -	.name = "status",
> -	.length = sizeof("disabled"),
> -	.value = "disabled",
> -};
> -
>  static const struct of_device_id omap_timer_match[] __initconst = {
>  	{ .compatible = "ti,omap2420-timer", },
>  	{ .compatible = "ti,omap3430-timer", },
> @@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
>  				  of_get_property(np, "ti,timer-secure", NULL)))
>  			continue;
>  
> -		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
> -			of_add_property(np, &device_disabled);
> +		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
> +			struct property *prop;
> +
> +			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +			if (!prop)
> +				return NULL;
> +			prop->name = "status";
> +			prop->length = sizeof("disabled");
> +			prop->value = "disabled";

How about (see drivers/of/unittest.c)?
prop->value = "disabled";
prop->length = strlen(prop->value);

> +			of_add_property(np, prop);
> +		}
>  		return np;
>  	}
>  
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[qhou]

On 2018年01月09日 19:52, Ladislav Michl wrote:
> On Tue, Jan 09, 2018 at 05:24:21PM +0800, Qi Hou wrote:
> > When more than one GP timers are used as kernel system timers and the
> > corresponding nodes in device-tree are marked with the same "disabled"
> > property, then the "attr" field of the property will be initialized
> > more than once as the property being added to sys file system via
> > __of_add_property_sysfs().
> > 
> > In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
> > directly to the return value of safe_name(), without taking care of
> > whether it's already a valid pointer to a memory block. If it is, its
> > old value will always be overwritten by the new one and the memory block
> > allocated before will a "ghost", then a kmemleak happened.
>
> As timers does not seem to be deallocated, this does not matter in practice.
> Fix eats a bit more from heap.
Yes, there is no timers being deallocated, as I see.

But it is not eligible that sharing the same property among different 
device nodes.
And itexposes kernel info to users. That is not safe.

The fix causes a tiny consumption of heap, maybe it's worth.

--
best regards,
Qi Hou
>
> > That the same "disabled" property being added to different nodes of device
> > tree would cause that kind of kmemleak overhead, at leat once.
> > 
> > To fix it, allocate the property dynamically, and delete static one.
> >
> > Signed-off-by: Qi Hou <qi.hou@windriver.com>
> > ---
> >  arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> > index ece09c9..206ae8d 100644
> > --- a/arch/arm/mach-omap2/timer.c
> > +++ b/arch/arm/mach-omap2/timer.c
> > @@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
> >  	.tick_resume		= omap2_gp_timer_shutdown,
> >  };
> >  
> > -static struct property device_disabled = {
> > -	.name = "status",
> > -	.length = sizeof("disabled"),
> > -	.value = "disabled",
> > -};
> > -
> >  static const struct of_device_id omap_timer_match[] __initconst = {
> >  	{ .compatible = "ti,omap2420-timer", },
> >  	{ .compatible = "ti,omap3430-timer", },
> > @@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
> >  				  of_get_property(np, "ti,timer-secure", NULL)))
> >  			continue;
> >  
> > -		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
> > -			of_add_property(np, &device_disabled);
> > +		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
> > +			struct property *prop;
> > +
> > +			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +			if (!prop)
> > +				return NULL;
> > +			prop->name = "status";
> > +			prop->length = sizeof("disabled");
> > +			prop->value = "disabled";
>
> How about (see drivers/of/unittest.c)?
> prop->value = "disabled";
> prop->length = strlen(prop->value);

It's better. I will do that.

thanks,
Qi Hou
>
> > +			of_add_property(np, prop);
> > +		}
> >  		return np;
> >  	}
> >  
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best regards,
Qi Hou
Phone number: +86-10-8477-8608
Address: Floor 15, Building B, Wangjing Plaza, No.9 Zhong-Huan Nanlu, Chaoyang District

Re: [PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[qhou]

On 2018年01月09日 19:52, Ladislav Michl wrote:
> On Tue, Jan 09, 2018 at 05:24:21PM +0800, Qi Hou wrote:
> > When more than one GP timers are used as kernel system timers and the
> > corresponding nodes in device-tree are marked with the same "disabled"
> > property, then the "attr" field of the property will be initialized
> > more than once as the property being added to sys file system via
> > __of_add_property_sysfs().
> > 
> > In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
> > directly to the return value of safe_name(), without taking care of
> > whether it's already a valid pointer to a memory block. If it is, its
> > old value will always be overwritten by the new one and the memory block
> > allocated before will a "ghost", then a kmemleak happened.
>
> As timers does not seem to be deallocated, this does not matter in practice.
> Fix eats a bit more from heap.
Yes, there is no timers being deallocated, as I see.

But it is not eligible that sharing the same property among different 
device nodes.
And itexposes kernel info to users. That is not safe.

The fix causes a tiny consumption of heap, maybe it's worth.

--
best regards,
Qi Hou
>
> > That the same "disabled" property being added to different nodes of device
> > tree would cause that kind of kmemleak overhead, at leat once.
> > 
> > To fix it, allocate the property dynamically, and delete static one.
> >
> > Signed-off-by: Qi Hou <qi.hou@windriver.com>
> > ---
> >  arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> > index ece09c9..206ae8d 100644
> > --- a/arch/arm/mach-omap2/timer.c
> > +++ b/arch/arm/mach-omap2/timer.c
> > @@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
> >  	.tick_resume		= omap2_gp_timer_shutdown,
> >  };
> >  
> > -static struct property device_disabled = {
> > -	.name = "status",
> > -	.length = sizeof("disabled"),
> > -	.value = "disabled",
> > -};
> > -
> >  static const struct of_device_id omap_timer_match[] __initconst = {
> >  	{ .compatible = "ti,omap2420-timer", },
> >  	{ .compatible = "ti,omap3430-timer", },
> > @@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
> >  				  of_get_property(np, "ti,timer-secure", NULL)))
> >  			continue;
> >  
> > -		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
> > -			of_add_property(np, &device_disabled);
> > +		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
> > +			struct property *prop;
> > +
> > +			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +			if (!prop)
> > +				return NULL;
> > +			prop->name = "status";
> > +			prop->length = sizeof("disabled");
> > +			prop->value = "disabled";
>
> How about (see drivers/of/unittest.c)?
> prop->value = "disabled";
> prop->length = strlen(prop->value);

It's better. I will do that.

thanks,
Qi Hou
>
> > +			of_add_property(np, prop);
> > +		}
> >  		return np;
> >  	}
> >  
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best regards,
Qi Hou
Phone number: +86-10-8477-8608
Address: Floor 15, Building B, Wangjing Plaza, No.9 Zhong-Huan Nanlu, Chaoyang District


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH, v2] arm: omap2: timer: fix a kmemleak caused in omap_get_timer_dt

[qhou]

On 2018?01?09? 19:52, Ladislav Michl wrote:
> On Tue, Jan 09, 2018 at 05:24:21PM +0800, Qi Hou wrote:
> > When more than one GP timers are used as kernel system timers and the
> > corresponding nodes in device-tree are marked with the same "disabled"
> > property, then the "attr" field of the property will be initialized
> > more than once as the property being added to sys file system via
> > __of_add_property_sysfs().
> > 
> > In __of_add_property_sysfs(), the "name" field of pp->attr.attr is set
> > directly to the return value of safe_name(), without taking care of
> > whether it's already a valid pointer to a memory block. If it is, its
> > old value will always be overwritten by the new one and the memory block
> > allocated before will a "ghost", then a kmemleak happened.
>
> As timers does not seem to be deallocated, this does not matter in practice.
> Fix eats a bit more from heap.
Yes, there is no timers being deallocated, as I see.

But it is not eligible that sharing the same property among different 
device nodes.
And itexposes kernel info to users. That is not safe.

The fix causes a tiny consumption of heap, maybe it's worth.

--
best regards,
Qi Hou
>
> > That the same "disabled" property being added to different nodes of device
> > tree would cause that kind of kmemleak overhead, at leat once.
> > 
> > To fix it, allocate the property dynamically, and delete static one.
> >
> > Signed-off-by: Qi Hou <qi.hou@windriver.com>
> > ---
> >  arch/arm/mach-omap2/timer.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> > index ece09c9..206ae8d 100644
> > --- a/arch/arm/mach-omap2/timer.c
> > +++ b/arch/arm/mach-omap2/timer.c
> > @@ -156,12 +156,6 @@ static struct clock_event_device clockevent_gpt = {
> >  	.tick_resume		= omap2_gp_timer_shutdown,
> >  };
> >  
> > -static struct property device_disabled = {
> > -	.name = "status",
> > -	.length = sizeof("disabled"),
> > -	.value = "disabled",
> > -};
> > -
> >  static const struct of_device_id omap_timer_match[] __initconst = {
> >  	{ .compatible = "ti,omap2420-timer", },
> >  	{ .compatible = "ti,omap3430-timer", },
> > @@ -203,8 +197,17 @@ static struct device_node * __init omap_get_timer_dt(const struct of_device_id *
> >  				  of_get_property(np, "ti,timer-secure", NULL)))
> >  			continue;
> >  
> > -		if (!of_device_is_compatible(np, "ti,omap-counter32k"))
> > -			of_add_property(np, &device_disabled);
> > +		if (!of_device_is_compatible(np, "ti,omap-counter32k")) {
> > +			struct property *prop;
> > +
> > +			prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +			if (!prop)
> > +				return NULL;
> > +			prop->name = "status";
> > +			prop->length = sizeof("disabled");
> > +			prop->value = "disabled";
>
> How about (see drivers/of/unittest.c)?
> prop->value = "disabled";
> prop->length = strlen(prop->value);

It's better. I will do that.

thanks,
Qi Hou
>
> > +			of_add_property(np, prop);
> > +		}
> >  		return np;
> >  	}
> >  
> > -- 
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best regards,
Qi Hou
Phone number: +86-10-8477-8608
Address: Floor 15, Building B, Wangjing Plaza, No.9 Zhong-Huan Nanlu, Chaoyang District