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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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