On Thu, 2022-03-24 at 21:42 +0000, Chuck Lever III wrote:
>
>
> > On Mar 24, 2022, at 5:33 PM, trondmy@xxxxxxxxxx wrote:
> >
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > Do not cast the struct xprt to a sock_xprt unless we know it is a
> > UDP or
> > TCP transport. Otherwise the call to lock the mutex will scribble
> > over
> > whatever structure is actually there. This has been seen to cause
> > hard
> > system lockups when the underlying transport was RDMA.
> >
> > Fixes: e44773daf851 ("SUNRPC: Add srcaddr as a file in sysfs")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > net/sunrpc/sysfs.c | 32 ++++++++++++++++++++++----------
> > 1 file changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
> > index 05c758da6a92..8ce053f84421 100644
> > --- a/net/sunrpc/sysfs.c
> > +++ b/net/sunrpc/sysfs.c
> > @@ -107,22 +107,34 @@ static ssize_t
> > rpc_sysfs_xprt_srcaddr_show(struct kobject *kobj,
> > struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
> > struct sockaddr_storage saddr;
> > struct sock_xprt *sock;
> > + const char *fmt = "<closed>\n";
> > ssize_t ret = -1;
> >
> > - if (!xprt || !xprt_connected(xprt)) {
> > - xprt_put(xprt);
> > - return -ENOTCONN;
> > - }
> > + if (!xprt || !xprt_connected(xprt))
> > + goto out;
> >
> > - sock = container_of(xprt, struct sock_xprt, xprt);
> > - mutex_lock(&sock->recv_mutex);
> > - if (sock->sock == NULL ||
> > - kernel_getsockname(sock->sock, (struct sockaddr
> > *)&saddr) < 0)
>
> This would be a little nicer if it went through the transport
> switch instead. But isn't the socket address available in the
> rpc_xprt?
>
I agree that a follow up patch could move this and the other socket-
specific code to the switch. However the point of this patch is to
provide a stable fix for the memory scribble.
...and no, the source address is not available in the rpc_xprt. Only
the destination address is stored there.
> > + switch (xprt->xprt_class->ident) {
> > + case XPRT_TRANSPORT_UDP:
> > + case XPRT_TRANSPORT_TCP:
> > + break;
> > + default:
> > + fmt = "<not a socket>\n";
> > goto out;
> > + };
> >
> > - ret = sprintf(buf, "%pISc\n", &saddr);
> > -out:
> > + sock = container_of(xprt, struct sock_xprt, xprt);
> > + mutex_lock(&sock->recv_mutex);
> > + if (sock->sock != NULL) {
> > + ret = kernel_getsockname(sock->sock, (struct
> > sockaddr *)&saddr);
> > + if (ret >= 0) {
> > + ret = sprintf(buf, "%pISc\n", &saddr);
> > + fmt = NULL;
> > + }
> > + }
> > mutex_unlock(&sock->recv_mutex);
> > +out:
> > + if (fmt)
> > + ret = sprintf(buf, fmt);
> > xprt_put(xprt);
> > return ret + 1;
> > }
> > --
> > 2.35.1
> >
>
> --
> Chuck Lever
>
>
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
