[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-nfs
Subject:    Re: [PATCH v3 11/13] sunrpc: add dst_attr attributes to the sysfs xprt directory
From:       Trond Myklebust <trondmy () hammerspace ! com>
Date:       2021-04-27 13:26:56
Message-ID: 5c089dfd503381f00856d8ecf9e836c58179517d.camel () hammerspace ! com
[Download RAW message or body]

On Mon, 2021-04-26 at 13:19 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Allow to query and set the destination's address of a transport.
> Setting of the destination address is allowed only for TCP or RDMA
> based connections.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  net/sunrpc/sysfs.c | 72
> ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
> index 682def4293f2..076f777db3cd 100644
> --- a/net/sunrpc/sysfs.c
> +++ b/net/sunrpc/sysfs.c
> @@ -4,6 +4,9 @@
>   */
>  #include <linux/sunrpc/clnt.h>
>  #include <linux/kobject.h>
> +#include <linux/sunrpc/addr.h>
> +#include <linux/sunrpc/xprtsock.h>
> +
>  #include "sysfs.h"
>  
>  static struct kset *rpc_sunrpc_kset;
> @@ -42,6 +45,66 @@ static struct kobject
> *rpc_sysfs_object_alloc(const char *name,
>         return NULL;
>  }
>  
> +static inline struct rpc_xprt *
> +rpc_sysfs_xprt_kobj_get_xprt(struct kobject *kobj)
> +{
> +       struct rpc_sysfs_xprt_switch_xprt *x = container_of(kobj,
> +               struct rpc_sysfs_xprt_switch_xprt, kobject);
> +
> +       return xprt_get(x->xprt);
> +}
> +
> +static ssize_t rpc_sysfs_xprt_dstaddr_show(struct kobject *kobj,
> +                                          struct kobj_attribute
> *attr,
> +                                          char *buf)
> +{
> +       struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
> +       ssize_t ret;
> +
> +       if (!xprt)
> +               return 0;
> +       if (xprt->xprt_class->ident == XPRT_TRANSPORT_LOCAL)
> +               ret = sprintf(buf, "localhost");

This makes it look like it is a loopback socket, whereas in reality it
is a named socket. Why not just use the xprt-
>address_strings[RPC_DISPLAY_ADDR] here?

> +       else
> +               ret = rpc_ntop((struct sockaddr *)&xprt->addr, buf,
> PAGE_SIZE);
> +       buf[ret] = '\n';
> +       xprt_put(xprt);
> +       return ret + 1;
> +}
> +
> +static ssize_t rpc_sysfs_xprt_dstaddr_store(struct kobject *kobj,
> +                                           struct kobj_attribute
> *attr,
> +                                           const char *buf, size_t
> count)
> +{
> +       struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
> +       struct sockaddr *saddr;
> +       int port;
> +
> +       if (!xprt)
> +               return 0;
> +       if (!(xprt->xprt_class->ident == XPRT_TRANSPORT_TCP ||
> +             xprt->xprt_class->ident == XPRT_TRANSPORT_RDMA)) {
> +               xprt_put(xprt);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE);
> +       saddr = (struct sockaddr *)&xprt->addr;
> +       port = rpc_get_port(saddr);
> +
> +       kfree(xprt->address_strings[RPC_DISPLAY_ADDR]);

This is not allowed. The xprt->address_strings can only be freed in a
manner that is safe w.r.t. the rcu_read_lock(). Otherwise, various
users of rpc_peeraddr2str() are prone to crash.

So if you're going to replace these strings, then you have to replace
them using something like rcu_replace_pointer(), then you have to free
them using call_rcu() or kfree_rcu}().

> +       xprt->address_strings[RPC_DISPLAY_ADDR] = kstrndup(buf, count
> - 1,
> +                                                         
> GFP_KERNEL);
> +       xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1,
> saddr,
> +                                sizeof(*saddr));
> +       rpc_set_port(saddr, port);

The above is also a bit dodgy w.r.t. rpc_peeraddr() since it is non-
atomic. However it looks like it should be OK in practice.

> +
> +       xprt->ops->connect(xprt, NULL);
> +       clear_bit(XPRT_LOCKED, &xprt->state);
> +       xprt_put(xprt);
> +       return count;
> +}
> +
>  int rpc_sysfs_init(void)
>  {
>         rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL,
> kernel_kobj);
> @@ -105,6 +168,14 @@ static const void
> *rpc_sysfs_xprt_switch_xprt_namespace(struct kobject *kobj)
>                             kobject)->net;
>  }
>  
> +static struct kobj_attribute rpc_sysfs_xprt_dstaddr =
> __ATTR(dstaddr,
> +       0644, rpc_sysfs_xprt_dstaddr_show,
> rpc_sysfs_xprt_dstaddr_store);
> +
> +static struct attribute *rpc_sysfs_xprt_attrs[] = {
> +       &rpc_sysfs_xprt_dstaddr.attr,
> +       NULL,
> +};
> +
>  static struct kobj_type rpc_sysfs_client_type = {
>         .release = rpc_sysfs_client_release,
>         .sysfs_ops = &kobj_sysfs_ops,
> @@ -119,6 +190,7 @@ static struct kobj_type
> rpc_sysfs_xprt_switch_type = {
>  
>  static struct kobj_type rpc_sysfs_xprt_switch_xprt_type = {
>         .release = rpc_sysfs_xprt_switch_xprt_release,
> +       .default_attrs = rpc_sysfs_xprt_attrs,
>         .sysfs_ops = &kobj_sysfs_ops,
>         .namespace = rpc_sysfs_xprt_switch_xprt_namespace,
>  };

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



[prev in list] [next in list] [prev in thread] [next in thread]