[PATCH] kernfs: handle null pointers while printing node name and path

[PATCH] kernfs: handle null pointers while printing node name and path

[Konstantin Khlebnikov]

Null kernfs nodes could be found at cgroups during construction.
It seems safer to handle these null pointers right in kernfs in
the same way as printf prints "(null)" for null pointer string.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Acked-by: Tejun Heo <tj@kernel.org>
---
 fs/kernfs/dir.c               |   10 ++++++++++
 include/trace/events/cgroup.h |   20 ++++++--------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index cf4c636ff4da..439b946c4808 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -41,6 +41,9 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
 
 static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
 {
+	if (!kn)
+		return strlcpy(buf, "(null)", buflen);
+
 	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
 }
 
@@ -110,6 +113,8 @@ static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a,
  * kn_to:   /n1/n2/n3         [depth=3]
  * result:  /../..
  *
+ * [3] when @kn_to is NULL result will be "(null)"
+ *
  * Returns the length of the full path.  If the full length is equal to or
  * greater than @buflen, @buf contains the truncated path with the trailing
  * '\0'.  On error, -errno is returned.
@@ -123,6 +128,9 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
 	size_t depth_from, depth_to, len = 0;
 	int i, j;
 
+	if (!kn_to)
+		return strlcpy(buf, "(null)", buflen);
+
 	if (!kn_from)
 		kn_from = kernfs_root(kn_to)->kn;
 
@@ -166,6 +174,8 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
  * similar to strlcpy().  It returns the length of @kn's name and if @buf
  * isn't long enough, it's filled upto @buflen-1 and nul terminated.
  *
+ * Fills buffer with "(null)" if @kn is NULL.
+ *
  * This function can be called from any context.
  */
 int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
index ab68640a18d0..c226f50e88fa 100644
--- a/include/trace/events/cgroup.h
+++ b/include/trace/events/cgroup.h
@@ -61,19 +61,15 @@ DECLARE_EVENT_CLASS(cgroup,
 		__field(	int,		id			)
 		__field(	int,		level			)
 		__dynamic_array(char,		path,
-				cgrp->kn ? cgroup_path(cgrp, NULL, 0) + 1
-					 : strlen("(null)"))
+				cgroup_path(cgrp, NULL, 0) + 1)
 	),
 
 	TP_fast_assign(
 		__entry->root = cgrp->root->hierarchy_id;
 		__entry->id = cgrp->id;
 		__entry->level = cgrp->level;
-		if (cgrp->kn)
-			cgroup_path(cgrp, __get_dynamic_array(path),
-				    __get_dynamic_array_len(path));
-		else
-			__assign_str(path, "(null)");
+		cgroup_path(cgrp, __get_dynamic_array(path),
+				  __get_dynamic_array_len(path));
 	),
 
 	TP_printk("root=%d id=%d level=%d path=%s",
@@ -119,8 +115,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
 		__field(	int,		dst_id			)
 		__field(	int,		dst_level		)
 		__dynamic_array(char,		dst_path,
-				dst_cgrp->kn ? cgroup_path(dst_cgrp, NULL, 0) + 1
-					     : strlen("(null)"))
+				cgroup_path(dst_cgrp, NULL, 0) + 1)
 		__field(	int,		pid			)
 		__string(	comm,		task->comm		)
 	),
@@ -129,11 +124,8 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
 		__entry->dst_root = dst_cgrp->root->hierarchy_id;
 		__entry->dst_id = dst_cgrp->id;
 		__entry->dst_level = dst_cgrp->level;
-		if (dst_cgrp->kn)
-			cgroup_path(dst_cgrp, __get_dynamic_array(dst_path),
-				    __get_dynamic_array_len(dst_path));
-		else
-			__assign_str(dst_path, "(null)");
+		cgroup_path(dst_cgrp, __get_dynamic_array(dst_path),
+				      __get_dynamic_array_len(dst_path));
 		__entry->pid = task->pid;
 		__assign_str(comm, task->comm);
 	),

Re: [PATCH] kernfs: handle null pointers while printing node name and path

[Greg Kroah-Hartman]

On Wed, Feb 08, 2017 at 02:28:55PM +0300, Konstantin Khlebnikov wrote:
> Null kernfs nodes could be found at cgroups during construction.

Really?  Does this happen today?  Is this an issue for older kernels as
well?

> It seems safer to handle these null pointers right in kernfs in
> the same way as printf prints "(null)" for null pointer string.
> 
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
> Acked-by: Tejun Heo <tj@kernel.org>
> ---
>  fs/kernfs/dir.c               |   10 ++++++++++
>  include/trace/events/cgroup.h |   20 ++++++--------------
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index cf4c636ff4da..439b946c4808 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -41,6 +41,9 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
>  
>  static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
>  {
> +	if (!kn)
> +		return strlcpy(buf, "(null)", buflen);

Why not return an error?

> +
>  	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
>  }
>  
> @@ -110,6 +113,8 @@ static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a,
>   * kn_to:   /n1/n2/n3         [depth=3]
>   * result:  /../..
>   *
> + * [3] when @kn_to is NULL result will be "(null)"
> + *
>   * Returns the length of the full path.  If the full length is equal to or
>   * greater than @buflen, @buf contains the truncated path with the trailing
>   * '\0'.  On error, -errno is returned.
> @@ -123,6 +128,9 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>  	size_t depth_from, depth_to, len = 0;
>  	int i, j;
>  
> +	if (!kn_to)
> +		return strlcpy(buf, "(null)", buflen);
> +
>  	if (!kn_from)
>  		kn_from = kernfs_root(kn_to)->kn;
>  
> @@ -166,6 +174,8 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>   * similar to strlcpy().  It returns the length of @kn's name and if @buf
>   * isn't long enough, it's filled upto @buflen-1 and nul terminated.
>   *
> + * Fills buffer with "(null)" if @kn is NULL.
> + *
>   * This function can be called from any context.
>   */
>  int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
> diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
> index ab68640a18d0..c226f50e88fa 100644
> --- a/include/trace/events/cgroup.h
> +++ b/include/trace/events/cgroup.h
> @@ -61,19 +61,15 @@ DECLARE_EVENT_CLASS(cgroup,
>  		__field(	int,		id			)
>  		__field(	int,		level			)
>  		__dynamic_array(char,		path,
> -				cgrp->kn ? cgroup_path(cgrp, NULL, 0) + 1
> -					 : strlen("(null)"))
> +				cgroup_path(cgrp, NULL, 0) + 1)

Ah, you are trying to make this "simpler", is that the case?

So this is just a "cleanup", not a bugfix?

thanks,

greg k-h

Re: [PATCH] kernfs: handle null pointers while printing node name and path

[Konstantin Khlebnikov]

On 08.02.2017 15:51, Greg Kroah-Hartman wrote:
> On Wed, Feb 08, 2017 at 02:28:55PM +0300, Konstantin Khlebnikov wrote:
>> Null kernfs nodes could be found at cgroups during construction.
>
> Really?  Does this happen today?  Is this an issue for older kernels as
> well?

This happended again recently: https://lkml.org/lkml/2017/1/26/126
LTP easily crashes everything since 4.6

>
>> It seems safer to handle these null pointers right in kernfs in
>> the same way as printf prints "(null)" for null pointer string.
>>
>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>> Acked-by: Tejun Heo <tj@kernel.org>
>> ---
>>  fs/kernfs/dir.c               |   10 ++++++++++
>>  include/trace/events/cgroup.h |   20 ++++++--------------
>>  2 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
>> index cf4c636ff4da..439b946c4808 100644
>> --- a/fs/kernfs/dir.c
>> +++ b/fs/kernfs/dir.c
>> @@ -41,6 +41,9 @@ static bool kernfs_lockdep(struct kernfs_node *kn)
>>
>>  static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
>>  {
>> +	if (!kn)
>> +		return strlcpy(buf, "(null)", buflen);
>
> Why not return an error?

because hunks below

>
>> +
>>  	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
>>  }
>>
>> @@ -110,6 +113,8 @@ static struct kernfs_node *kernfs_common_ancestor(struct kernfs_node *a,
>>   * kn_to:   /n1/n2/n3         [depth=3]
>>   * result:  /../..
>>   *
>> + * [3] when @kn_to is NULL result will be "(null)"
>> + *
>>   * Returns the length of the full path.  If the full length is equal to or
>>   * greater than @buflen, @buf contains the truncated path with the trailing
>>   * '\0'.  On error, -errno is returned.
>> @@ -123,6 +128,9 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>>  	size_t depth_from, depth_to, len = 0;
>>  	int i, j;
>>
>> +	if (!kn_to)
>> +		return strlcpy(buf, "(null)", buflen);
>> +
>>  	if (!kn_from)
>>  		kn_from = kernfs_root(kn_to)->kn;
>>
>> @@ -166,6 +174,8 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>>   * similar to strlcpy().  It returns the length of @kn's name and if @buf
>>   * isn't long enough, it's filled upto @buflen-1 and nul terminated.
>>   *
>> + * Fills buffer with "(null)" if @kn is NULL.
>> + *
>>   * This function can be called from any context.
>>   */
>>  int kernfs_name(struct kernfs_node *kn, char *buf, size_t buflen)
>> diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
>> index ab68640a18d0..c226f50e88fa 100644
>> --- a/include/trace/events/cgroup.h
>> +++ b/include/trace/events/cgroup.h
>> @@ -61,19 +61,15 @@ DECLARE_EVENT_CLASS(cgroup,
>>  		__field(	int,		id			)
>>  		__field(	int,		level			)
>>  		__dynamic_array(char,		path,
>> -				cgrp->kn ? cgroup_path(cgrp, NULL, 0) + 1
>> -					 : strlen("(null)"))
>> +				cgroup_path(cgrp, NULL, 0) + 1)
>
> Ah, you are trying to make this "simpler", is that the case?
>
> So this is just a "cleanup", not a bugfix?

This was proposed as bugfix but Tejun fixed bug in cgroup differently.
Fix for another bug in cpu cgroups is still pending.

Plus I've found these kludges in tracing.
So this patch cleanups them and probably prevents some future bugs.

Re: [PATCH] kernfs: handle null pointers while printing node name and path

[Greg Kroah-Hartman]

On Wed, Feb 08, 2017 at 03:59:06PM +0300, Konstantin Khlebnikov wrote:
> 
> 
> On 08.02.2017 15:51, Greg Kroah-Hartman wrote:
> > On Wed, Feb 08, 2017 at 02:28:55PM +0300, Konstantin Khlebnikov wrote:
> > > Null kernfs nodes could be found at cgroups during construction.
> > 
> > Really?  Does this happen today?  Is this an issue for older kernels as
> > well?
> 
> This happended again recently: https://lkml.org/lkml/2017/1/26/126
> LTP easily crashes everything since 4.6

Why did you drop the "Fixes:" and cc: stable line from the patch you
sent me?  That would have saved me asking you this :(

Care to fix this up and send a new version with that info in it?

thanks,

greg k-h

Re: [PATCH] kernfs: handle null pointers while printing node name and path

[Konstantin Khlebnikov]

On 08.02.2017 16:23, Greg Kroah-Hartman wrote:
> On Wed, Feb 08, 2017 at 03:59:06PM +0300, Konstantin Khlebnikov wrote:
>>
>>
>> On 08.02.2017 15:51, Greg Kroah-Hartman wrote:
>>> On Wed, Feb 08, 2017 at 02:28:55PM +0300, Konstantin Khlebnikov wrote:
>>>> Null kernfs nodes could be found at cgroups during construction.
>>>
>>> Really?  Does this happen today?  Is this an issue for older kernels as
>>> well?
>>
>> This happended again recently: https://lkml.org/lkml/2017/1/26/126
>> LTP easily crashes everything since 4.6
>
> Why did you drop the "Fixes:" and cc: stable line from the patch you
> sent me?  That would have saved me asking you this :(

No, this _was_ proposed as a fix but now bug fixed differently right in cgroups.
Now this patch just a cleanup / safer api.

>
> Care to fix this up and send a new version with that info in it?
>
> thanks,
>
> greg k-h
>