Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Olga Kornievskaia" <olga.kornievskaia@gmail.com>
Cc:     trond.myklebust@hammerspace.com, anna.schumaker@netapp.com,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH v10 10/13] sunrpc: add dst_attr attributes to the sysfs
 xprt directory
In-reply-to: <20210608195922.88655-11-olga.kornievskaia@gmail.com>
References: <20210608195922.88655-1-olga.kornievskaia@gmail.com>,
 <20210608195922.88655-11-olga.kornievskaia@gmail.com>
Date:   Mon, 28 Feb 2022 14:00:03 +1100
Message-id: <164601720393.12951.15824905969343983450@noble.neil.brown.name>
Precedence: bulk

On Wed, 09 Jun 2021, 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>
> ---
>  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