| Message ID | 20210514141323.67922-11-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | create sysfs files for changing IP address | expand |
On Fri, May 14, 2021 at 10:13:21AM -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> .. > + saddr = (struct sockaddr *)&xprt->addr; > + port = rpc_get_port(saddr); > + > + dst_addr = kstrndup(buf, count - 1, GFP_KERNEL); > + if (!dst_addr) > + goto out_err; > + saved_addr = kzalloc(sizeof(*saved_addr), GFP_KERNEL); > + if (!saved_addr) > + goto out_err_free; > + saved_addr->addr = > + rcu_dereference_raw(xprt->address_strings[RPC_DISPLAY_ADDR]); > + rcu_assign_pointer(xprt->address_strings[RPC_DISPLAY_ADDR], dst_addr); > + call_rcu(&saved_addr->rcu, free_xprt_addr); > + xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr, > + sizeof(*saddr)); Hi Olga, How does this behave if rpc_pton fails? Perhaps this conversion being also a validation check on input given from user-space should be done before the xprt is being modified?
On Sat, May 15, 2021 at 8:42 AM Dan Aloni <dan@kernelim.com> wrote: > > On Fri, May 14, 2021 at 10:13:21AM -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> > .. > > + saddr = (struct sockaddr *)&xprt->addr; > > + port = rpc_get_port(saddr); > > + > > + dst_addr = kstrndup(buf, count - 1, GFP_KERNEL); > > + if (!dst_addr) > > + goto out_err; > > + saved_addr = kzalloc(sizeof(*saved_addr), GFP_KERNEL); > > + if (!saved_addr) > > + goto out_err_free; > > + saved_addr->addr = > > + rcu_dereference_raw(xprt->address_strings[RPC_DISPLAY_ADDR]); > > + rcu_assign_pointer(xprt->address_strings[RPC_DISPLAY_ADDR], dst_addr); > > + call_rcu(&saved_addr->rcu, free_xprt_addr); > > + xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr, > > + sizeof(*saddr)); > > Hi Olga, > > How does this behave if rpc_pton fails? Perhaps this conversion being > also a validation check on input given from user-space should be done > before the xprt is being modified? It's assumed that an administrator is providing a valid (and correct) address value. Transport would continue to be disconnected until a proper value is supplied. We can't validate for instance that the supplied value is a "correct" value for the v4.1 server (which would require sending an EXCHANGE_ID and checking that it's the same server as before). But yes perhaps a userland utility can do things like DNS resolution to get the IP, it can check format correctness and connectivity as well (but can't check the v4 requirement though). > > -- > Dan Aloni
On Mon, May 17, 2021 at 09:53:30AM -0400, Olga Kornievskaia wrote: > On Sat, May 15, 2021 at 8:42 AM Dan Aloni <dan@kernelim.com> wrote: > > > > On Fri, May 14, 2021 at 10:13:21AM -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> > > .. > > > + saddr = (struct sockaddr *)&xprt->addr; > > > + port = rpc_get_port(saddr); > > > + > > > + dst_addr = kstrndup(buf, count - 1, GFP_KERNEL); > > > + if (!dst_addr) > > > + goto out_err; > > > + saved_addr = kzalloc(sizeof(*saved_addr), GFP_KERNEL); > > > + if (!saved_addr) > > > + goto out_err_free; > > > + saved_addr->addr = > > > + rcu_dereference_raw(xprt->address_strings[RPC_DISPLAY_ADDR]); > > > + rcu_assign_pointer(xprt->address_strings[RPC_DISPLAY_ADDR], dst_addr); > > > + call_rcu(&saved_addr->rcu, free_xprt_addr); > > > + xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr, > > > + sizeof(*saddr)); > > > > Hi Olga, > > > > How does this behave if rpc_pton fails? Perhaps this conversion being > > also a validation check on input given from user-space should be done > > before the xprt is being modified? > > It's assumed that an administrator is providing a valid (and correct) > address value. Transport would continue to be disconnected until a > proper value is supplied. We can't validate for instance that the > supplied value is a "correct" value for the v4.1 server (which would > require sending an EXCHANGE_ID and checking that it's the same server > as before). I'm not sure it would work that way - looks like `rpc_pton` zeroes `saddr` before trying to fill it (or leaves it as it in other cases such as the INET_ADDRSTRLEN bound check). I think `0.0.0.0` routes to localhost by default? That might cause unexpected behavior from admin PoV. More to consider is that `xprt->address_strings[RPC_DISPLAY_ADDR]` should be consistent with the value of `saddr` in the error cases, otherwise debugging xprt state may be give confusing output.
On Mon, May 17, 2021 at 1:16 PM Dan Aloni <dan@kernelim.com> wrote: > > On Mon, May 17, 2021 at 09:53:30AM -0400, Olga Kornievskaia wrote: > > On Sat, May 15, 2021 at 8:42 AM Dan Aloni <dan@kernelim.com> wrote: > > > > > > On Fri, May 14, 2021 at 10:13:21AM -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> > > > .. > > > > + saddr = (struct sockaddr *)&xprt->addr; > > > > + port = rpc_get_port(saddr); > > > > + > > > > + dst_addr = kstrndup(buf, count - 1, GFP_KERNEL); > > > > + if (!dst_addr) > > > > + goto out_err; > > > > + saved_addr = kzalloc(sizeof(*saved_addr), GFP_KERNEL); > > > > + if (!saved_addr) > > > > + goto out_err_free; > > > > + saved_addr->addr = > > > > + rcu_dereference_raw(xprt->address_strings[RPC_DISPLAY_ADDR]); > > > > + rcu_assign_pointer(xprt->address_strings[RPC_DISPLAY_ADDR], dst_addr); > > > > + call_rcu(&saved_addr->rcu, free_xprt_addr); > > > > + xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr, > > > > + sizeof(*saddr)); > > > > > > Hi Olga, > > > > > > How does this behave if rpc_pton fails? Perhaps this conversion being > > > also a validation check on input given from user-space should be done > > > before the xprt is being modified? > > > > It's assumed that an administrator is providing a valid (and correct) > > address value. Transport would continue to be disconnected until a > > proper value is supplied. We can't validate for instance that the > > supplied value is a "correct" value for the v4.1 server (which would > > require sending an EXCHANGE_ID and checking that it's the same server > > as before). > > I'm not sure it would work that way - looks like `rpc_pton` zeroes > `saddr` before trying to fill it (or leaves it as it in other cases such > as the INET_ADDRSTRLEN bound check). I think `0.0.0.0` routes to > localhost by default? That might cause unexpected behavior from admin PoV. > > More to consider is that `xprt->address_strings[RPC_DISPLAY_ADDR]` > should be consistent with the value of `saddr` in the error cases, > otherwise debugging xprt state may be give confusing output. the reason for rpc_pton() to fail if an incorrect address is supplied. I again said that given that this is a privileged operation, we rely on the admin to provide a correct address. But are you arguing for checking return or rpt_pton() and then reverting address_string assignment back to the saved value and returning? Ok, I can do that. But what can't be done is that once xprt_force_disconnect() is called (1) we can't know if re-connect to the newly provided value is successful and (2) thus we can't roll back the address values to the old ones. Seems the added complexity is not gaining us much in return. As I said I think the userspace tool that would sit on top of this would do all the checks first before using the sysfs. > > -- > Dan Aloni
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h index 823663c6380a..579c42901fc8 100644 --- a/include/linux/sunrpc/xprt.h +++ b/include/linux/sunrpc/xprt.h @@ -412,6 +412,7 @@ void xprt_conditional_disconnect(struct rpc_xprt *xprt, unsigned int cookie); bool xprt_lock_connect(struct rpc_xprt *, struct rpc_task *, void *); void xprt_unlock_connect(struct rpc_xprt *, void *); +void xprt_release_write(struct rpc_xprt *, struct rpc_task *); /* * Reserved bit positions in xprt->state diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c index 20f75708594f..f330148433c8 100644 --- a/net/sunrpc/sysfs.c +++ b/net/sunrpc/sysfs.c @@ -4,8 +4,24 @@ */ #include <linux/sunrpc/clnt.h> #include <linux/kobject.h> +#include <linux/sunrpc/addr.h> +#include <linux/sunrpc/xprtsock.h> + #include "sysfs.h" +struct xprt_addr { + const char *addr; + struct rcu_head rcu; +}; + +static void free_xprt_addr(struct rcu_head *head) +{ + struct xprt_addr *addr = container_of(head, struct xprt_addr, rcu); + + kfree(addr->addr); + kfree(addr); +} + static struct kset *rpc_sunrpc_kset; static struct kobject *rpc_sunrpc_client_kobj, *rpc_sunrpc_xprt_switch_kobj; @@ -43,6 +59,81 @@ 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 *x = container_of(kobj, + struct rpc_sysfs_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; + ret = sprintf(buf, "%s\n", xprt->address_strings[RPC_DISPLAY_ADDR]); + 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; + char *dst_addr; + int port; + struct xprt_addr *saved_addr; + + if (!xprt) + return 0; + if (!(xprt->xprt_class->ident == XPRT_TRANSPORT_TCP || + xprt->xprt_class->ident == XPRT_TRANSPORT_RDMA)) { + xprt_put(xprt); + return -EOPNOTSUPP; + } + + if (wait_on_bit_lock(&xprt->state, XPRT_LOCKED, TASK_KILLABLE)) { + count = -EINTR; + goto out_put; + } + saddr = (struct sockaddr *)&xprt->addr; + port = rpc_get_port(saddr); + + dst_addr = kstrndup(buf, count - 1, GFP_KERNEL); + if (!dst_addr) + goto out_err; + saved_addr = kzalloc(sizeof(*saved_addr), GFP_KERNEL); + if (!saved_addr) + goto out_err_free; + saved_addr->addr = + rcu_dereference_raw(xprt->address_strings[RPC_DISPLAY_ADDR]); + rcu_assign_pointer(xprt->address_strings[RPC_DISPLAY_ADDR], dst_addr); + call_rcu(&saved_addr->rcu, free_xprt_addr); + xprt->addrlen = rpc_pton(xprt->xprt_net, buf, count - 1, saddr, + sizeof(*saddr)); + rpc_set_port(saddr, port); + + xprt_force_disconnect(xprt); +out: + xprt_release_write(xprt, NULL); +out_put: + xprt_put(xprt); + return count; +out_err_free: + kfree(dst_addr); +out_err: + count = -ENOMEM; + goto out; +} + int rpc_sysfs_init(void) { rpc_sunrpc_kset = kset_create_and_add("sunrpc", NULL, kernel_kobj); @@ -106,6 +197,14 @@ static const void *rpc_sysfs_xprt_namespace(struct kobject *kobj) kobject)->xprt->xprt_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, @@ -120,6 +219,7 @@ static struct kobj_type rpc_sysfs_xprt_switch_type = { static struct kobj_type rpc_sysfs_xprt_type = { .release = rpc_sysfs_xprt_release, + .default_attrs = rpc_sysfs_xprt_attrs, .sysfs_ops = &kobj_sysfs_ops, .namespace = rpc_sysfs_xprt_namespace, }; diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c index fd58a3a16add..b9beb7fc453d 100644 --- a/net/sunrpc/xprt.c +++ b/net/sunrpc/xprt.c @@ -442,7 +442,7 @@ void xprt_release_xprt_cong(struct rpc_xprt *xprt, struct rpc_task *task) } EXPORT_SYMBOL_GPL(xprt_release_xprt_cong); -static inline void xprt_release_write(struct rpc_xprt *xprt, struct rpc_task *task) +void xprt_release_write(struct rpc_xprt *xprt, struct rpc_task *task) { if (xprt->snd_task != task) return;