On Wed, 09 Jun 2021, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> include/linux/sunrpc/xprt.h | 1 +
> net/sunrpc/sysfs.c | 105 ++++++++++++++++++++++++++++++++++++
> net/sunrpc/xprt.c | 4 +-
> net/sunrpc/xprtmultipath.c | 2 -
> 4 files changed, 109 insertions(+), 3 deletions(-)
>
....
> +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;
> + size_t buf_len;
> +
> + 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;
> + }
A bit late to this party, however...
This use of wait_on_bit_lock() is not safe. Wake_up events are only
generated on XPRT_LOCKED when connect completes (xprt_unlock_connect())
and when xprt_autoclose() runs.
Most of the time when a socket is active, this flag is often set and
cleared without any wakeup - or without a wakeup which would wake this
waiter. There is usually an rpc_wakeup_first_on_queue().
So if this sysfs function is called while XPRT_LOCKED it set, it will
block until killed or the connection is closed.
xs_enable_swap() has a similar problem, and this has failed in practice.
An easy fix would be to add a wake_up_bit() call in xprt_clear_locked()
so that we always get that wakeup. I'm not sure that is a good idea
though, is this wakeup is almost always unnecessary.
For xs_enable_swap() a better solution is to use ->recv_mutex to provide
the required exclusion.
It isn't clear to me what exclusion this function actually needs, so I
cannot suggest an alternate way to provide it.
rpc_sysfs_xprt_state_change() has the same problem.
NeilBrown
