[PATCH] sunrpc: make debugfs file creation failure non-fatal

[PATCH] sunrpc: make debugfs file creation failure non-fatal

[Jeff Layton]

We currently have a problem that SELinux policy is being enforced when
creating debugfs files. If a debugfs file is created as a side effect of
doing some syscall, then that creation can fail if the SELinux policy
for that process prevents it.

This seems wrong. We don't do that for files under /proc, for instance,
so Bruce has proposed a patch to fix that.

While discussing that patch however, Greg K.H. stated:

    "No kernel code should care / fail if a debugfs function fails, so
     please fix up the sunrpc code first."

This patch converts all of the sunrpc debugfs setup code to be void
return functins, and the callers to not look for errors from those
functions.

This should allow rpc_clnt and rpc_xprt creation to work, even if the
kernel fails to create debugfs files for some reason.

Cc: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 include/linux/sunrpc/debug.h | 18 +++++++++---------
 net/sunrpc/clnt.c            |  4 +---
 net/sunrpc/debugfs.c         | 34 +++++++++++++---------------------
 net/sunrpc/sunrpc_syms.c     |  7 +------
 net/sunrpc/xprt.c            |  7 +------
 5 files changed, 25 insertions(+), 45 deletions(-)

diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
index c57d8ea0716c..59a7889e15db 100644
--- a/include/linux/sunrpc/debug.h
+++ b/include/linux/sunrpc/debug.h
@@ -60,17 +60,17 @@ struct rpc_xprt;
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 void		rpc_register_sysctl(void);
 void		rpc_unregister_sysctl(void);
-int		sunrpc_debugfs_init(void);
+void		sunrpc_debugfs_init(void);
 void		sunrpc_debugfs_exit(void);
-int		rpc_clnt_debugfs_register(struct rpc_clnt *);
+void		rpc_clnt_debugfs_register(struct rpc_clnt *);
 void		rpc_clnt_debugfs_unregister(struct rpc_clnt *);
-int		rpc_xprt_debugfs_register(struct rpc_xprt *);
+void		rpc_xprt_debugfs_register(struct rpc_xprt *);
 void		rpc_xprt_debugfs_unregister(struct rpc_xprt *);
 #else
-static inline int
+static inline void
 sunrpc_debugfs_init(void)
 {
-	return 0;
+	return;
 }
 
 static inline void
@@ -79,10 +79,10 @@ sunrpc_debugfs_exit(void)
 	return;
 }
 
-static inline int
+static inline void
 rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 {
-	return 0;
+	return;
 }
 
 static inline void
@@ -91,10 +91,10 @@ rpc_clnt_debugfs_unregister(struct rpc_clnt *clnt)
 	return;
 }
 
-static inline int
+static inline void
 rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
 {
-	return 0;
+	return;
 }
 
 static inline void
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 612aa73bbc60..e6ce1517367f 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -303,9 +303,7 @@ static int rpc_client_register(struct rpc_clnt *clnt,
 	struct super_block *pipefs_sb;
 	int err;
 
-	err = rpc_clnt_debugfs_register(clnt);
-	if (err)
-		return err;
+	rpc_clnt_debugfs_register(clnt);
 
 	pipefs_sb = rpc_get_sb_net(net);
 	if (pipefs_sb) {
diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
index e811f390f9f6..1c2e3960b939 100644
--- a/net/sunrpc/debugfs.c
+++ b/net/sunrpc/debugfs.c
@@ -129,32 +129,30 @@ static const struct file_operations tasks_fops = {
 	.release	= tasks_release,
 };
 
-int
+void
 rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 {
-	int len, err;
+	int len;
 	char name[24]; /* enough for "../../rpc_xprt/ + 8 hex digits + NULL */
 
 	/* Already registered? */
 	if (clnt->cl_debugfs)
-		return 0;
+		return;
 
 	len = snprintf(name, sizeof(name), "%x", clnt->cl_clid);
 	if (len >= sizeof(name))
-		return -EINVAL;
+		return;
 
 	/* make the per-client dir */
 	clnt->cl_debugfs = debugfs_create_dir(name, rpc_clnt_dir);
 	if (!clnt->cl_debugfs)
-		return -ENOMEM;
+		return;
 
 	/* make tasks file */
-	err = -ENOMEM;
 	if (!debugfs_create_file("tasks", S_IFREG | S_IRUSR, clnt->cl_debugfs,
 				 clnt, &tasks_fops))
 		goto out_err;
 
-	err = -EINVAL;
 	rcu_read_lock();
 	len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
 			rcu_dereference(clnt->cl_xprt)->debugfs->d_name.name);
@@ -162,15 +160,13 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
 	if (len >= sizeof(name))
 		goto out_err;
 
-	err = -ENOMEM;
 	if (!debugfs_create_symlink("xprt", clnt->cl_debugfs, name))
 		goto out_err;
 
-	return 0;
+	return;
 out_err:
 	debugfs_remove_recursive(clnt->cl_debugfs);
 	clnt->cl_debugfs = NULL;
-	return err;
 }
 
 void
@@ -226,7 +222,7 @@ static const struct file_operations xprt_info_fops = {
 	.release	= xprt_info_release,
 };
 
-int
+void
 rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
 {
 	int len, id;
@@ -237,22 +233,19 @@ rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
 
 	len = snprintf(name, sizeof(name), "%x", id);
 	if (len >= sizeof(name))
-		return -EINVAL;
+		return;
 
 	/* make the per-client dir */
 	xprt->debugfs = debugfs_create_dir(name, rpc_xprt_dir);
 	if (!xprt->debugfs)
-		return -ENOMEM;
+		return;
 
 	/* make tasks file */
 	if (!debugfs_create_file("info", S_IFREG | S_IRUSR, xprt->debugfs,
 				 xprt, &xprt_info_fops)) {
 		debugfs_remove_recursive(xprt->debugfs);
 		xprt->debugfs = NULL;
-		return -ENOMEM;
 	}
-
-	return 0;
 }
 
 void
@@ -266,14 +259,15 @@ void __exit
 sunrpc_debugfs_exit(void)
 {
 	debugfs_remove_recursive(topdir);
+	topdir = NULL;
 }
 
-int __init
+void __init
 sunrpc_debugfs_init(void)
 {
 	topdir = debugfs_create_dir("sunrpc", NULL);
 	if (!topdir)
-		goto out;
+		return;
 
 	rpc_clnt_dir = debugfs_create_dir("rpc_clnt", topdir);
 	if (!rpc_clnt_dir)
@@ -283,10 +277,8 @@ sunrpc_debugfs_init(void)
 	if (!rpc_xprt_dir)
 		goto out_remove;
 
-	return 0;
+	return;
 out_remove:
 	debugfs_remove_recursive(topdir);
 	topdir = NULL;
-out:
-	return -ENOMEM;
 }
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index e37fbed87956..ee5d3d253102 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -98,10 +98,7 @@ init_sunrpc(void)
 	if (err)
 		goto out4;
 
-	err = sunrpc_debugfs_init();
-	if (err)
-		goto out5;
-
+	sunrpc_debugfs_init();
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 	rpc_register_sysctl();
 #endif
@@ -109,8 +106,6 @@ init_sunrpc(void)
 	init_socket_xprt();	/* clnt sock transport */
 	return 0;
 
-out5:
-	unregister_rpc_pipefs();
 out4:
 	unregister_pernet_subsys(&sunrpc_net_ops);
 out3:
diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index e3015aede0d9..9949722d99ce 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1331,7 +1331,6 @@ static void xprt_init(struct rpc_xprt *xprt, struct net *net)
  */
 struct rpc_xprt *xprt_create_transport(struct xprt_create *args)
 {
-	int err;
 	struct rpc_xprt	*xprt;
 	struct xprt_class *t;
 
@@ -1372,11 +1371,7 @@ found:
 		return ERR_PTR(-ENOMEM);
 	}
 
-	err = rpc_xprt_debugfs_register(xprt);
-	if (err) {
-		xprt_destroy(xprt);
-		return ERR_PTR(err);
-	}
+	rpc_xprt_debugfs_register(xprt);
 
 	dprintk("RPC:       created transport %p with %u slots\n", xprt,
 			xprt->max_reqs);
-- 
2.1.0

Re: [PATCH] sunrpc: make debugfs file creation failure non-fatal

[J. Bruce Fields]

ACK.--b.

On Mon, Mar 30, 2015 at 05:58:18PM -0400, Jeff Layton wrote:
> We currently have a problem that SELinux policy is being enforced when
> creating debugfs files. If a debugfs file is created as a side effect of
> doing some syscall, then that creation can fail if the SELinux policy
> for that process prevents it.
> 
> This seems wrong. We don't do that for files under /proc, for instance,
> so Bruce has proposed a patch to fix that.
> 
> While discussing that patch however, Greg K.H. stated:
> 
>     "No kernel code should care / fail if a debugfs function fails, so
>      please fix up the sunrpc code first."
> 
> This patch converts all of the sunrpc debugfs setup code to be void
> return functins, and the callers to not look for errors from those
> functions.
> 
> This should allow rpc_clnt and rpc_xprt creation to work, even if the
> kernel fails to create debugfs files for some reason.
> 
> Cc: "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> Signed-off-by: Jeff Layton <jeff.layton-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
> ---
>  include/linux/sunrpc/debug.h | 18 +++++++++---------
>  net/sunrpc/clnt.c            |  4 +---
>  net/sunrpc/debugfs.c         | 34 +++++++++++++---------------------
>  net/sunrpc/sunrpc_syms.c     |  7 +------
>  net/sunrpc/xprt.c            |  7 +------
>  5 files changed, 25 insertions(+), 45 deletions(-)
> 
> diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
> index c57d8ea0716c..59a7889e15db 100644
> --- a/include/linux/sunrpc/debug.h
> +++ b/include/linux/sunrpc/debug.h
> @@ -60,17 +60,17 @@ struct rpc_xprt;
>  #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>  void		rpc_register_sysctl(void);
>  void		rpc_unregister_sysctl(void);
> -int		sunrpc_debugfs_init(void);
> +void		sunrpc_debugfs_init(void);
>  void		sunrpc_debugfs_exit(void);
> -int		rpc_clnt_debugfs_register(struct rpc_clnt *);
> +void		rpc_clnt_debugfs_register(struct rpc_clnt *);
>  void		rpc_clnt_debugfs_unregister(struct rpc_clnt *);
> -int		rpc_xprt_debugfs_register(struct rpc_xprt *);
> +void		rpc_xprt_debugfs_register(struct rpc_xprt *);
>  void		rpc_xprt_debugfs_unregister(struct rpc_xprt *);
>  #else
> -static inline int
> +static inline void
>  sunrpc_debugfs_init(void)
>  {
> -	return 0;
> +	return;
>  }
>  
>  static inline void
> @@ -79,10 +79,10 @@ sunrpc_debugfs_exit(void)
>  	return;
>  }
>  
> -static inline int
> +static inline void
>  rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
>  {
> -	return 0;
> +	return;
>  }
>  
>  static inline void
> @@ -91,10 +91,10 @@ rpc_clnt_debugfs_unregister(struct rpc_clnt *clnt)
>  	return;
>  }
>  
> -static inline int
> +static inline void
>  rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
>  {
> -	return 0;
> +	return;
>  }
>  
>  static inline void
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index 612aa73bbc60..e6ce1517367f 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -303,9 +303,7 @@ static int rpc_client_register(struct rpc_clnt *clnt,
>  	struct super_block *pipefs_sb;
>  	int err;
>  
> -	err = rpc_clnt_debugfs_register(clnt);
> -	if (err)
> -		return err;
> +	rpc_clnt_debugfs_register(clnt);
>  
>  	pipefs_sb = rpc_get_sb_net(net);
>  	if (pipefs_sb) {
> diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
> index e811f390f9f6..1c2e3960b939 100644
> --- a/net/sunrpc/debugfs.c
> +++ b/net/sunrpc/debugfs.c
> @@ -129,32 +129,30 @@ static const struct file_operations tasks_fops = {
>  	.release	= tasks_release,
>  };
>  
> -int
> +void
>  rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
>  {
> -	int len, err;
> +	int len;
>  	char name[24]; /* enough for "../../rpc_xprt/ + 8 hex digits + NULL */
>  
>  	/* Already registered? */
>  	if (clnt->cl_debugfs)
> -		return 0;
> +		return;
>  
>  	len = snprintf(name, sizeof(name), "%x", clnt->cl_clid);
>  	if (len >= sizeof(name))
> -		return -EINVAL;
> +		return;
>  
>  	/* make the per-client dir */
>  	clnt->cl_debugfs = debugfs_create_dir(name, rpc_clnt_dir);
>  	if (!clnt->cl_debugfs)
> -		return -ENOMEM;
> +		return;
>  
>  	/* make tasks file */
> -	err = -ENOMEM;
>  	if (!debugfs_create_file("tasks", S_IFREG | S_IRUSR, clnt->cl_debugfs,
>  				 clnt, &tasks_fops))
>  		goto out_err;
>  
> -	err = -EINVAL;
>  	rcu_read_lock();
>  	len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
>  			rcu_dereference(clnt->cl_xprt)->debugfs->d_name.name);
> @@ -162,15 +160,13 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
>  	if (len >= sizeof(name))
>  		goto out_err;
>  
> -	err = -ENOMEM;
>  	if (!debugfs_create_symlink("xprt", clnt->cl_debugfs, name))
>  		goto out_err;
>  
> -	return 0;
> +	return;
>  out_err:
>  	debugfs_remove_recursive(clnt->cl_debugfs);
>  	clnt->cl_debugfs = NULL;
> -	return err;
>  }
>  
>  void
> @@ -226,7 +222,7 @@ static const struct file_operations xprt_info_fops = {
>  	.release	= xprt_info_release,
>  };
>  
> -int
> +void
>  rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
>  {
>  	int len, id;
> @@ -237,22 +233,19 @@ rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
>  
>  	len = snprintf(name, sizeof(name), "%x", id);
>  	if (len >= sizeof(name))
> -		return -EINVAL;
> +		return;
>  
>  	/* make the per-client dir */
>  	xprt->debugfs = debugfs_create_dir(name, rpc_xprt_dir);
>  	if (!xprt->debugfs)
> -		return -ENOMEM;
> +		return;
>  
>  	/* make tasks file */
>  	if (!debugfs_create_file("info", S_IFREG | S_IRUSR, xprt->debugfs,
>  				 xprt, &xprt_info_fops)) {
>  		debugfs_remove_recursive(xprt->debugfs);
>  		xprt->debugfs = NULL;
> -		return -ENOMEM;
>  	}
> -
> -	return 0;
>  }
>  
>  void
> @@ -266,14 +259,15 @@ void __exit
>  sunrpc_debugfs_exit(void)
>  {
>  	debugfs_remove_recursive(topdir);
> +	topdir = NULL;
>  }
>  
> -int __init
> +void __init
>  sunrpc_debugfs_init(void)
>  {
>  	topdir = debugfs_create_dir("sunrpc", NULL);
>  	if (!topdir)
> -		goto out;
> +		return;
>  
>  	rpc_clnt_dir = debugfs_create_dir("rpc_clnt", topdir);
>  	if (!rpc_clnt_dir)
> @@ -283,10 +277,8 @@ sunrpc_debugfs_init(void)
>  	if (!rpc_xprt_dir)
>  		goto out_remove;
>  
> -	return 0;
> +	return;
>  out_remove:
>  	debugfs_remove_recursive(topdir);
>  	topdir = NULL;
> -out:
> -	return -ENOMEM;
>  }
> diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
> index e37fbed87956..ee5d3d253102 100644
> --- a/net/sunrpc/sunrpc_syms.c
> +++ b/net/sunrpc/sunrpc_syms.c
> @@ -98,10 +98,7 @@ init_sunrpc(void)
>  	if (err)
>  		goto out4;
>  
> -	err = sunrpc_debugfs_init();
> -	if (err)
> -		goto out5;
> -
> +	sunrpc_debugfs_init();
>  #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>  	rpc_register_sysctl();
>  #endif
> @@ -109,8 +106,6 @@ init_sunrpc(void)
>  	init_socket_xprt();	/* clnt sock transport */
>  	return 0;
>  
> -out5:
> -	unregister_rpc_pipefs();
>  out4:
>  	unregister_pernet_subsys(&sunrpc_net_ops);
>  out3:
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index e3015aede0d9..9949722d99ce 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1331,7 +1331,6 @@ static void xprt_init(struct rpc_xprt *xprt, struct net *net)
>   */
>  struct rpc_xprt *xprt_create_transport(struct xprt_create *args)
>  {
> -	int err;
>  	struct rpc_xprt	*xprt;
>  	struct xprt_class *t;
>  
> @@ -1372,11 +1371,7 @@ found:
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> -	err = rpc_xprt_debugfs_register(xprt);
> -	if (err) {
> -		xprt_destroy(xprt);
> -		return ERR_PTR(err);
> -	}
> +	rpc_xprt_debugfs_register(xprt);
>  
>  	dprintk("RPC:       created transport %p with %u slots\n", xprt,
>  			xprt->max_reqs);
> -- 
> 2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sunrpc: make debugfs file creation failure non-fatal

[J. Bruce Fields]

On Mon, Mar 30, 2015 at 07:47:53PM -0400, J. Bruce Fields wrote:
> ACK.--b.

But note the result after this is that the debugfs directories will
always miss gss-proxy clients on selinux-enforcing systems.  That could
be really confusing.

So we should still fix debugfs's permission checking.  It doesn't make
sense to me as is.

--b.

> 
> On Mon, Mar 30, 2015 at 05:58:18PM -0400, Jeff Layton wrote:
> > We currently have a problem that SELinux policy is being enforced when
> > creating debugfs files. If a debugfs file is created as a side effect of
> > doing some syscall, then that creation can fail if the SELinux policy
> > for that process prevents it.
> > 
> > This seems wrong. We don't do that for files under /proc, for instance,
> > so Bruce has proposed a patch to fix that.
> > 
> > While discussing that patch however, Greg K.H. stated:
> > 
> >     "No kernel code should care / fail if a debugfs function fails, so
> >      please fix up the sunrpc code first."
> > 
> > This patch converts all of the sunrpc debugfs setup code to be void
> > return functins, and the callers to not look for errors from those
> > functions.
> > 
> > This should allow rpc_clnt and rpc_xprt creation to work, even if the
> > kernel fails to create debugfs files for some reason.
> > 
> > Cc: "J. Bruce Fields" <bfields@fieldses.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > ---
> >  include/linux/sunrpc/debug.h | 18 +++++++++---------
> >  net/sunrpc/clnt.c            |  4 +---
> >  net/sunrpc/debugfs.c         | 34 +++++++++++++---------------------
> >  net/sunrpc/sunrpc_syms.c     |  7 +------
> >  net/sunrpc/xprt.c            |  7 +------
> >  5 files changed, 25 insertions(+), 45 deletions(-)
> > 
> > diff --git a/include/linux/sunrpc/debug.h b/include/linux/sunrpc/debug.h
> > index c57d8ea0716c..59a7889e15db 100644
> > --- a/include/linux/sunrpc/debug.h
> > +++ b/include/linux/sunrpc/debug.h
> > @@ -60,17 +60,17 @@ struct rpc_xprt;
> >  #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
> >  void		rpc_register_sysctl(void);
> >  void		rpc_unregister_sysctl(void);
> > -int		sunrpc_debugfs_init(void);
> > +void		sunrpc_debugfs_init(void);
> >  void		sunrpc_debugfs_exit(void);
> > -int		rpc_clnt_debugfs_register(struct rpc_clnt *);
> > +void		rpc_clnt_debugfs_register(struct rpc_clnt *);
> >  void		rpc_clnt_debugfs_unregister(struct rpc_clnt *);
> > -int		rpc_xprt_debugfs_register(struct rpc_xprt *);
> > +void		rpc_xprt_debugfs_register(struct rpc_xprt *);
> >  void		rpc_xprt_debugfs_unregister(struct rpc_xprt *);
> >  #else
> > -static inline int
> > +static inline void
> >  sunrpc_debugfs_init(void)
> >  {
> > -	return 0;
> > +	return;
> >  }
> >  
> >  static inline void
> > @@ -79,10 +79,10 @@ sunrpc_debugfs_exit(void)
> >  	return;
> >  }
> >  
> > -static inline int
> > +static inline void
> >  rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
> >  {
> > -	return 0;
> > +	return;
> >  }
> >  
> >  static inline void
> > @@ -91,10 +91,10 @@ rpc_clnt_debugfs_unregister(struct rpc_clnt *clnt)
> >  	return;
> >  }
> >  
> > -static inline int
> > +static inline void
> >  rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
> >  {
> > -	return 0;
> > +	return;
> >  }
> >  
> >  static inline void
> > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > index 612aa73bbc60..e6ce1517367f 100644
> > --- a/net/sunrpc/clnt.c
> > +++ b/net/sunrpc/clnt.c
> > @@ -303,9 +303,7 @@ static int rpc_client_register(struct rpc_clnt *clnt,
> >  	struct super_block *pipefs_sb;
> >  	int err;
> >  
> > -	err = rpc_clnt_debugfs_register(clnt);
> > -	if (err)
> > -		return err;
> > +	rpc_clnt_debugfs_register(clnt);
> >  
> >  	pipefs_sb = rpc_get_sb_net(net);
> >  	if (pipefs_sb) {
> > diff --git a/net/sunrpc/debugfs.c b/net/sunrpc/debugfs.c
> > index e811f390f9f6..1c2e3960b939 100644
> > --- a/net/sunrpc/debugfs.c
> > +++ b/net/sunrpc/debugfs.c
> > @@ -129,32 +129,30 @@ static const struct file_operations tasks_fops = {
> >  	.release	= tasks_release,
> >  };
> >  
> > -int
> > +void
> >  rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
> >  {
> > -	int len, err;
> > +	int len;
> >  	char name[24]; /* enough for "../../rpc_xprt/ + 8 hex digits + NULL */
> >  
> >  	/* Already registered? */
> >  	if (clnt->cl_debugfs)
> > -		return 0;
> > +		return;
> >  
> >  	len = snprintf(name, sizeof(name), "%x", clnt->cl_clid);
> >  	if (len >= sizeof(name))
> > -		return -EINVAL;
> > +		return;
> >  
> >  	/* make the per-client dir */
> >  	clnt->cl_debugfs = debugfs_create_dir(name, rpc_clnt_dir);
> >  	if (!clnt->cl_debugfs)
> > -		return -ENOMEM;
> > +		return;
> >  
> >  	/* make tasks file */
> > -	err = -ENOMEM;
> >  	if (!debugfs_create_file("tasks", S_IFREG | S_IRUSR, clnt->cl_debugfs,
> >  				 clnt, &tasks_fops))
> >  		goto out_err;
> >  
> > -	err = -EINVAL;
> >  	rcu_read_lock();
> >  	len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
> >  			rcu_dereference(clnt->cl_xprt)->debugfs->d_name.name);
> > @@ -162,15 +160,13 @@ rpc_clnt_debugfs_register(struct rpc_clnt *clnt)
> >  	if (len >= sizeof(name))
> >  		goto out_err;
> >  
> > -	err = -ENOMEM;
> >  	if (!debugfs_create_symlink("xprt", clnt->cl_debugfs, name))
> >  		goto out_err;
> >  
> > -	return 0;
> > +	return;
> >  out_err:
> >  	debugfs_remove_recursive(clnt->cl_debugfs);
> >  	clnt->cl_debugfs = NULL;
> > -	return err;
> >  }
> >  
> >  void
> > @@ -226,7 +222,7 @@ static const struct file_operations xprt_info_fops = {
> >  	.release	= xprt_info_release,
> >  };
> >  
> > -int
> > +void
> >  rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
> >  {
> >  	int len, id;
> > @@ -237,22 +233,19 @@ rpc_xprt_debugfs_register(struct rpc_xprt *xprt)
> >  
> >  	len = snprintf(name, sizeof(name), "%x", id);
> >  	if (len >= sizeof(name))
> > -		return -EINVAL;
> > +		return;
> >  
> >  	/* make the per-client dir */
> >  	xprt->debugfs = debugfs_create_dir(name, rpc_xprt_dir);
> >  	if (!xprt->debugfs)
> > -		return -ENOMEM;
> > +		return;
> >  
> >  	/* make tasks file */
> >  	if (!debugfs_create_file("info", S_IFREG | S_IRUSR, xprt->debugfs,
> >  				 xprt, &xprt_info_fops)) {
> >  		debugfs_remove_recursive(xprt->debugfs);
> >  		xprt->debugfs = NULL;
> > -		return -ENOMEM;
> >  	}
> > -
> > -	return 0;
> >  }
> >  
> >  void
> > @@ -266,14 +259,15 @@ void __exit
> >  sunrpc_debugfs_exit(void)
> >  {
> >  	debugfs_remove_recursive(topdir);
> > +	topdir = NULL;
> >  }
> >  
> > -int __init
> > +void __init
> >  sunrpc_debugfs_init(void)
> >  {
> >  	topdir = debugfs_create_dir("sunrpc", NULL);
> >  	if (!topdir)
> > -		goto out;
> > +		return;
> >  
> >  	rpc_clnt_dir = debugfs_create_dir("rpc_clnt", topdir);
> >  	if (!rpc_clnt_dir)
> > @@ -283,10 +277,8 @@ sunrpc_debugfs_init(void)
> >  	if (!rpc_xprt_dir)
> >  		goto out_remove;
> >  
> > -	return 0;
> > +	return;
> >  out_remove:
> >  	debugfs_remove_recursive(topdir);
> >  	topdir = NULL;
> > -out:
> > -	return -ENOMEM;
> >  }
> > diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
> > index e37fbed87956..ee5d3d253102 100644
> > --- a/net/sunrpc/sunrpc_syms.c
> > +++ b/net/sunrpc/sunrpc_syms.c
> > @@ -98,10 +98,7 @@ init_sunrpc(void)
> >  	if (err)
> >  		goto out4;
> >  
> > -	err = sunrpc_debugfs_init();
> > -	if (err)
> > -		goto out5;
> > -
> > +	sunrpc_debugfs_init();
> >  #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
> >  	rpc_register_sysctl();
> >  #endif
> > @@ -109,8 +106,6 @@ init_sunrpc(void)
> >  	init_socket_xprt();	/* clnt sock transport */
> >  	return 0;
> >  
> > -out5:
> > -	unregister_rpc_pipefs();
> >  out4:
> >  	unregister_pernet_subsys(&sunrpc_net_ops);
> >  out3:
> > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > index e3015aede0d9..9949722d99ce 100644
> > --- a/net/sunrpc/xprt.c
> > +++ b/net/sunrpc/xprt.c
> > @@ -1331,7 +1331,6 @@ static void xprt_init(struct rpc_xprt *xprt, struct net *net)
> >   */
> >  struct rpc_xprt *xprt_create_transport(struct xprt_create *args)
> >  {
> > -	int err;
> >  	struct rpc_xprt	*xprt;
> >  	struct xprt_class *t;
> >  
> > @@ -1372,11 +1371,7 @@ found:
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> >  
> > -	err = rpc_xprt_debugfs_register(xprt);
> > -	if (err) {
> > -		xprt_destroy(xprt);
> > -		return ERR_PTR(err);
> > -	}
> > +	rpc_xprt_debugfs_register(xprt);
> >  
> >  	dprintk("RPC:       created transport %p with %u slots\n", xprt,
> >  			xprt->max_reqs);
> > -- 
> > 2.1.0

Re: [PATCH] sunrpc: make debugfs file creation failure non-fatal

[Greg KH]

On Tue, Mar 31, 2015 at 10:09:16AM -0400, J. Bruce Fields wrote:
> On Mon, Mar 30, 2015 at 07:47:53PM -0400, J. Bruce Fields wrote:
> > ACK.--b.
> 
> But note the result after this is that the debugfs directories will
> always miss gss-proxy clients on selinux-enforcing systems.  That could
> be really confusing.

So, you shouldn't be relying on debugfs :)

> So we should still fix debugfs's permission checking.  It doesn't make
> sense to me as is.

I don't really understand what the problem is here.  Is selinux
preventing some debugfs files to be created?  If so, great, it's allowed
to do that, go fix up your selinux config files to not do that.
Otherwise, to go around selinux/LSM seems like a bad idea for debugfs to
be doing, don't you think?

thanks,

greg k-h

Re: [PATCH] sunrpc: make debugfs file creation failure non-fatal

[Jeff Layton]

On Tue, 31 Mar 2015 16:26:41 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Mar 31, 2015 at 10:09:16AM -0400, J. Bruce Fields wrote:
> > On Mon, Mar 30, 2015 at 07:47:53PM -0400, J. Bruce Fields wrote:
> > > ACK.--b.
> > 
> > But note the result after this is that the debugfs directories will
> > always miss gss-proxy clients on selinux-enforcing systems.  That could
> > be really confusing.
> 
> So, you shouldn't be relying on debugfs :)
> 

These files are nice to have things for debugging, so debugfs seemed
like the appropriate place for them.

> > So we should still fix debugfs's permission checking.  It doesn't make
> > sense to me as is.
> 
> I don't really understand what the problem is here.  Is selinux
> preventing some debugfs files to be created?  If so, great, it's allowed
> to do that, go fix up your selinux config files to not do that.
> Otherwise, to go around selinux/LSM seems like a bad idea for debugfs to
> be doing, don't you think?
>

gssproxy is SELinux contained and the current policy forbids it from
creating the debugfs files. gssproxy isn't actually creating them
directly however -- the kernel is creating them as a side effect of the
rpc_clnt/rpc_xprt creation.

The question is whether enforcing SELinux policy on debugfs files makes
any sense at all. AFAICT, userland can't really create files directly
on debugfs, can it? So why should we prevent the kernel from doing so
just because it happens to be occurring in the context of a contained
SELinux process?

We certainly can update the selinux policy to allow gssproxy to do
this, but:

a) it's a pain

...and..

b) it seems like we're working around nonsensical debugfs behavior


-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH] sunrpc: make debugfs file creation failure non-fatal

[Boaz Harrosh]

On 03/31/2015 06:11 PM, Jeff Layton wrote:
> On Tue, 31 Mar 2015 16:26:41 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
<>
> We certainly can update the selinux policy to allow gssproxy to do
> this, but:
> 

Or can we update the selinux policy to allow any user access to
debugfs, since as you said it is always Kernel created ?


Thanks
Boaz

Re: [PATCH] sunrpc: make debugfs file creation failure non-fatal

[J. Bruce Fields]

On Tue, Mar 31, 2015 at 06:48:09PM +0300, Boaz Harrosh wrote:
> On 03/31/2015 06:11 PM, Jeff Layton wrote:
> > On Tue, 31 Mar 2015 16:26:41 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> <>
> > We certainly can update the selinux policy to allow gssproxy to do
> > this, but:
> > 
> 
> Or can we update the selinux policy to allow any user access to
> debugfs, since as you said it is always Kernel created ?

As I said, it's actually directory search permissions that selinux is
denying.

Denying gss-proxy permissions to read debugfs actually sounds reasonable
to me--most daemons probably don't need to read debugfs, so why take the
chance there might be some inadvertent information exposure in debugfs
that could be useful to an attacker?

--b.

Re: [PATCH] sunrpc: make debugfs file creation failure non-fatal

[J. Bruce Fields]

On Tue, Mar 31, 2015 at 04:26:41PM +0200, Greg KH wrote:
> On Tue, Mar 31, 2015 at 10:09:16AM -0400, J. Bruce Fields wrote:
> > On Mon, Mar 30, 2015 at 07:47:53PM -0400, J. Bruce Fields wrote:
> > > ACK.--b.
> > 
> > But note the result after this is that the debugfs directories will
> > always miss gss-proxy clients on selinux-enforcing systems.  That could
> > be really confusing.
> 
> So, you shouldn't be relying on debugfs :)
>
> > So we should still fix debugfs's permission checking.  It doesn't make
> > sense to me as is.
> 
> I don't really understand what the problem is here.  Is selinux
> preventing some debugfs files to be created?

debugfs doesn't actually check permission on the create itself, it only
checks for permission to lookup in the directory.  But the effect is to
prevent some creates, yes.

> If so, great, it's allowed
> to do that, go fix up your selinux config files to not do that.
> Otherwise, to go around selinux/LSM seems like a bad idea for debugfs to
> be doing, don't you think?

As far as I can tell, other synthetic filesystems that allow kernel
subsystems to create files skip permissions checking (based on just a
quick look at proc, sysfs, and rpc_pipefs).  Even in the debugfs case
the permissions-checking appears to be an accident.

To take an extreme case, we wouldn't want fork() to check the caller's
permissions on /proc.

It's less crazy in this case, but I think it still violates the the
principle of least surprise.

If there's some real requirement for permissions checking here, then I'd
like to understand what that requirement is.  And then:

	- create should be checking for the correct permissions (not
	  just search/execute).

	- we should document that callers need to ignore errors from
	  debugfs_create_*, to avoid situations like this where adding
	  debugging files to some bit of kernel infrastructure causes
	  regressions.  ("You need to update your selinux policy to
	  access this new debugging feature" is maybe OK, but "You need
	  to update your selinux policy to fix a kernel regression"
	  definitely isn't.)

--b.