* [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs
@ 2022-03-24 21:33 trondmy
2022-03-24 21:33 ` [PATCH 2/2] SUNRPC: Don't return error values in sysfs read of closed files trondmy
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: trondmy @ 2022-03-24 21:33 UTC (permalink / raw)
To: linux-nfs
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)
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] SUNRPC: Don't return error values in sysfs read of closed files
2022-03-24 21:33 [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs trondmy
@ 2022-03-24 21:33 ` trondmy
2022-03-24 21:42 ` [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs Chuck Lever III
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: trondmy @ 2022-03-24 21:33 UTC (permalink / raw)
To: linux-nfs
From: Trond Myklebust <trond.myklebust@hammerspace.com>
Instead of returning an error value, which ends up being the return
value for the read() system call, it is more elegant to simply return
the error as a string value.
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
net/sunrpc/sysfs.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/sunrpc/sysfs.c b/net/sunrpc/sysfs.c
index 8ce053f84421..796e0f141282 100644
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -93,10 +93,13 @@ static ssize_t rpc_sysfs_xprt_dstaddr_show(struct kobject *kobj,
struct rpc_xprt *xprt = rpc_sysfs_xprt_kobj_get_xprt(kobj);
ssize_t ret;
- if (!xprt)
- return 0;
+ if (!xprt) {
+ ret = sprintf(buf, "<closed>\n");
+ goto out;
+ }
ret = sprintf(buf, "%s\n", xprt->address_strings[RPC_DISPLAY_ADDR]);
xprt_put(xprt);
+out:
return ret + 1;
}
@@ -147,8 +150,8 @@ static ssize_t rpc_sysfs_xprt_info_show(struct kobject *kobj,
ssize_t ret;
if (!xprt || !xprt_connected(xprt)) {
- xprt_put(xprt);
- return -ENOTCONN;
+ ret = sprintf(buf, "<closed>\n");
+ goto out;
}
ret = sprintf(buf, "last_used=%lu\ncur_cong=%lu\ncong_win=%lu\n"
@@ -165,6 +168,7 @@ static ssize_t rpc_sysfs_xprt_info_show(struct kobject *kobj,
atomic_long_read(&xprt->queuelen),
(xprt->xprt_class->ident == XPRT_TRANSPORT_TCP) ?
xprt->address_strings[RPC_DISPLAY_PORT] : "0");
+out:
xprt_put(xprt);
return ret + 1;
}
@@ -178,10 +182,7 @@ static ssize_t rpc_sysfs_xprt_state_show(struct kobject *kobj,
int locked, connected, connecting, close_wait, bound, binding,
closing, congested, cwnd_wait, write_space, offline, remove;
- if (!xprt)
- return 0;
-
- if (!xprt->state) {
+ if (!(xprt && xprt->state)) {
ret = sprintf(buf, "state=CLOSED\n");
} else {
locked = test_bit(XPRT_LOCKED, &xprt->state);
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs
2022-03-24 21:33 [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs trondmy
2022-03-24 21:33 ` [PATCH 2/2] SUNRPC: Don't return error values in sysfs read of closed files trondmy
@ 2022-03-24 21:42 ` Chuck Lever III
2022-03-24 22:01 ` Trond Myklebust
2022-03-25 1:58 ` [PATCH] SUNRPC: fix semicolon.cocci warnings kernel test robot
2022-03-25 2:03 ` [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs kernel test robot
3 siblings, 1 reply; 6+ messages in thread
From: Chuck Lever III @ 2022-03-24 21:42 UTC (permalink / raw)
To: trondmy@kernel.org; +Cc: Linux NFS Mailing List
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs
2022-03-24 21:42 ` [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs Chuck Lever III
@ 2022-03-24 22:01 ` Trond Myklebust
0 siblings, 0 replies; 6+ messages in thread
From: Trond Myklebust @ 2022-03-24 22:01 UTC (permalink / raw)
To: chuck.lever@oracle.com; +Cc: linux-nfs@vger.kernel.org
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
>
>
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] SUNRPC: fix semicolon.cocci warnings
2022-03-24 21:33 [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs trondmy
2022-03-24 21:33 ` [PATCH 2/2] SUNRPC: Don't return error values in sysfs read of closed files trondmy
2022-03-24 21:42 ` [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs Chuck Lever III
@ 2022-03-25 1:58 ` kernel test robot
2022-03-25 2:03 ` [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-03-25 1:58 UTC (permalink / raw)
To: trondmy, linux-nfs; +Cc: kbuild-all
From: kernel test robot <lkp@intel.com>
net/sunrpc/sysfs.c:123:2-3: Unneeded semicolon
Remove unneeded semicolon.
Generated by: scripts/coccinelle/misc/semicolon.cocci
CC: Trond Myklebust <trond.myklebust@hammerspace.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
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
:::::: branch date: 4 hours ago
:::::: commit date: 4 hours ago
net/sunrpc/sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/net/sunrpc/sysfs.c
+++ b/net/sunrpc/sysfs.c
@@ -120,7 +120,7 @@ static ssize_t rpc_sysfs_xprt_srcaddr_sh
default:
fmt = "<not a socket>\n";
goto out;
- };
+ }
sock = container_of(xprt, struct sock_xprt, xprt);
mutex_lock(&sock->recv_mutex);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs
2022-03-24 21:33 [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs trondmy
` (2 preceding siblings ...)
2022-03-25 1:58 ` [PATCH] SUNRPC: fix semicolon.cocci warnings kernel test robot
@ 2022-03-25 2:03 ` kernel test robot
3 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2022-03-25 2:03 UTC (permalink / raw)
To: trondmy, linux-nfs; +Cc: kbuild-all
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.
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-25 2:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-24 21:33 [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs trondmy
2022-03-24 21:33 ` [PATCH 2/2] SUNRPC: Don't return error values in sysfs read of closed files trondmy
2022-03-24 21:42 ` [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs Chuck Lever III
2022-03-24 22:01 ` Trond Myklebust
2022-03-25 1:58 ` [PATCH] SUNRPC: fix semicolon.cocci warnings kernel test robot
2022-03-25 2:03 ` [PATCH 1/2] SUNRPC: Do not dereference non-socket transports in sysfs kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox