> On Mar 24, 2022, at 5:33 PM, [email protected] wrote:
>
> From: Trond Myklebust <[email protected]>
>
> 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: [email protected]
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> 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?
> + 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
On Thu, 2022-03-24 at 21:42 +0000, Chuck Lever III wrote:
>
>
> > On Mar 24, 2022, at 5:33 PM, [email protected] wrote:
> >
> > From: Trond Myklebust <[email protected]>
> >
> > 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: [email protected]
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > 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
[email protected]