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

[Trond Myklebust <trond.myklebust@primarydata.com>]

On Tue, Mar 31, 2015 at 1:12 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> Trond, this is kind of your baliwick, but do you mind if I take it
> instead?  It's kind of a pain for my testing currently.

That's fine with me. I don't foresee any other changes to that code
for this next merge window, so no conflicts are expected.

> --b.
>
> On Tue, Mar 31, 2015 at 12:03:28PM -0400, Jeff Layton wrote:
>> v2: gracefully handle the case where some dentry pointers end up NULL
>>     and be more dilligent about zeroing out dentry pointers
>>
>> 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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Acked-by: "J. Bruce Fields" <bfields@fieldses.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         | 52 ++++++++++++++++++++++++--------------------
>>  net/sunrpc/sunrpc_syms.c     |  7 +-----
>>  net/sunrpc/xprt.c            |  7 +-----
>>  5 files changed, 41 insertions(+), 47 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..82962f7e6e88 100644
>> --- a/net/sunrpc/debugfs.c
>> +++ b/net/sunrpc/debugfs.c
>> @@ -129,48 +129,52 @@ 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 */
>> +     struct rpc_xprt *xprt;
>>
>>       /* Already registered? */
>> -     if (clnt->cl_debugfs)
>> -             return 0;
>> +     if (clnt->cl_debugfs || !rpc_clnt_dir)
>> +             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();
>> +     xprt = rcu_dereference(clnt->cl_xprt);
>> +     /* no "debugfs" dentry? Don't bother with the symlink. */
>> +     if (!xprt->debugfs) {
>> +             rcu_read_unlock();
>> +             return;
>> +     }
>>       len = snprintf(name, sizeof(name), "../../rpc_xprt/%s",
>> -                     rcu_dereference(clnt->cl_xprt)->debugfs->d_name.name);
>> +                     xprt->debugfs->d_name.name);
>>       rcu_read_unlock();
>> +
>>       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,33 +230,33 @@ 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;
>>       static atomic_t cur_id;
>>       char            name[9]; /* 8 hex digits + NULL term */
>>
>> +     if (!rpc_xprt_dir)
>> +             return;
>> +
>>       id = (unsigned int)atomic_inc_return(&cur_id);
>>
>>       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 +270,17 @@ void __exit
>>  sunrpc_debugfs_exit(void)
>>  {
>>       debugfs_remove_recursive(topdir);
>> +     topdir = NULL;
>> +     rpc_clnt_dir = NULL;
>> +     rpc_xprt_dir = 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 +290,9 @@ 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;
>> +     rpc_clnt_dir = NULL;
>>  }
>> 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