| Message ID | 20220324213345.5833-1-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] SUNRPC: Do not dereference non-socket transports in sysfs | expand |
> On Mar 24, 2022, at 5:33 PM, trondmy@kernel.org wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > 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@vger.kernel.org > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > 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, trondmy@kernel.org wrote: > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > 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@vger.kernel.org > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > 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 > > >
Hi,
I love your patch! Perhaps something to improve:
[auto build test WARNING on trondmy-nfs/linux-next]
[also build test WARNING on v5.17 next-20220324]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/trondmy-kernel-org/SUNRPC-Do-not-dereference-non-socket-transports-in-sysfs/20220325-054144
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: x86_64-randconfig-c002 (https://download.01.org/0day-ci/archive/20220325/202203250924.8HsqSPwP-lkp@intel.com/config)
compiler: gcc-9 (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
cocci warnings: (new ones prefixed by >>)
>> net/sunrpc/sysfs.c:123:2-3: Unneeded semicolon
Please review and possibly fold the followup patch.
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) + 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; }