[PATCH 1/2] sunrpc: handle ENOMEM in rpcb_getport_async
From: "J. Bruce Fields"
If we ignore the error we'll hit a null dereference a little later.
Reported-by: syzbot+4b98281f2401ab849...@syzkaller.appspotmail.com
Signed-off-by: J. Bruce Fields
---
net/sunrpc/rpcb_clnt.c | 8
1 file changed, 8 insertions(+)
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index c526f8fb37c9..82c120e51d64 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -771,6 +771,12 @@ void rpcb_getport_async(struct rpc_task *task)
case RPCBVERS_3:
map->r_netid = xprt->address_strings[RPC_DISPLAY_NETID];
map->r_addr = rpc_sockaddr2uaddr(sap, GFP_ATOMIC);
+ if (!map->r_addr) {
+ status = -ENOMEM;
+ dprintk("RPC: %5u %s: no memory available\n",
+ task->tk_pid, __func__);
+ goto bailout_free_args;
+ }
map->r_owner = "";
break;
case RPCBVERS_2:
@@ -793,6 +799,8 @@ void rpcb_getport_async(struct rpc_task *task)
rpc_put_task(child);
return;
+bailout_free_args:
+ kfree(map);
bailout_release_client:
rpc_release_client(rpcb_clnt);
bailout_nofree:
--
2.17.0
Re: general protection fault in encode_rpcb_string
From: "J. Bruce Fields"
Date: Tue, 8 May 2018 11:47:03 -0400
Subject: [PATCH 2/2] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS
It's OK to sleep here, we just don't want to recurse into the filesystem
as this writeout could be waiting on this.
As a next step: the documentation for GFP_NOFS says "Please try to avoid
using this flag directly and instead use memalloc_nofs_{save,restore} to
mark the whole scope which cannot/shouldn't recurse into the FS layer
with a short explanation why. All allocation requests will inherit
GFP_NOFS implicitly."
But I'm not sure where to do this. Should the workqueue could be
arranging that for us in the case of workqueues created with
WQ_MEM_RECLAIM?
Reported-by: Trond Myklebust
Signed-off-by: J. Bruce Fields
---
net/sunrpc/rpcb_clnt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
On Tue, Apr 17, 2018 at 09:54:36PM +, Trond Myklebust wrote:
> Yes, and we can probably convert it, and the other GFP_ATOMIC
> allocations in the rpcbind client to use GFP_NOFS in order to improve
> reliability.
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c
index 82c120e51d64..576e84a1adee 100644
--- a/net/sunrpc/rpcb_clnt.c
+++ b/net/sunrpc/rpcb_clnt.c
@@ -752,7 +752,7 @@ void rpcb_getport_async(struct rpc_task *task)
goto bailout_nofree;
}
- map = kzalloc(sizeof(struct rpcbind_args), GFP_ATOMIC);
+ map = kzalloc(sizeof(struct rpcbind_args), GFP_NOFS);
if (!map) {
status = -ENOMEM;
dprintk("RPC: %5u %s: no memory available\n",
@@ -770,7 +770,7 @@ void rpcb_getport_async(struct rpc_task *task)
case RPCBVERS_4:
case RPCBVERS_3:
map->r_netid = xprt->address_strings[RPC_DISPLAY_NETID];
- map->r_addr = rpc_sockaddr2uaddr(sap, GFP_ATOMIC);
+ map->r_addr = rpc_sockaddr2uaddr(sap, GFP_NOFS);
if (!map->r_addr) {
status = -ENOMEM;
dprintk("RPC: %5u %s: no memory available\n",
--
2.17.0
Re: general protection fault in encode_rpcb_string
On Tue, Apr 17, 2018 at 09:54:36PM +, Trond Myklebust wrote: > Yes, and we can probably convert it, and the other GFP_ATOMIC > allocations in the rpcbind client to use GFP_NOFS in order to improve > reliability. Chuck, I think the GFP_ATOMIC is unnecessary here as well? --b. diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c index e8adad33d0bb..de90c6c90cde 100644 --- a/net/sunrpc/xprtrdma/rpc_rdma.c +++ b/net/sunrpc/xprtrdma/rpc_rdma.c @@ -228,7 +228,7 @@ rpcrdma_convert_iovs(struct rpcrdma_xprt *r_xprt, struct xdr_buf *xdrbuf, /* XXX: Certain upper layer operations do * not provide receive buffer pages. */ - *ppages = alloc_page(GFP_ATOMIC); + *ppages = alloc_page(GFP_NOFS); if (!*ppages) return -EAGAIN; }
Re: [PATCH net-next] SUNRPC: drop pointless static qualifier in xdr_get_next_encode_buffer()
On Thu, Nov 08, 2018 at 03:13:25AM +, Trond Myklebust wrote:
> On Thu, 2018-11-08 at 02:04 +, YueHaibing wrote:
> > There is no need to have the '__be32 *p' variable static since new
> > value
> > always be assigned before use it.
Applying for 4.20 and stable, thanks!
> >
> > Signed-off-by: YueHaibing
> > ---
> > net/sunrpc/xdr.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 2bbb8d3..d80b156 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -546,7 +546,7 @@ void xdr_commit_encode(struct xdr_stream *xdr)
> > static __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr,
> > size_t nbytes)
> > {
> > - static __be32 *p;
> > + __be32 *p;
> > int space_left;
> > int frag1bytes, frag2bytes;
> >
>
> Ouch, that's a really nasty bug that could definitely cause corruption
> if you have 2 threads simultaneously calling this function! This really
> deserves to be a stable patch.
Agreed. Looks like I introduced that in 3.16, over 5 years ago, so I'm
a little surprised not to have seen a bug report that this would
explain. Maybe it's just that the critical section is only a few lines
of arithemtic at the end of the function. Also it only gets called when
an xdr reply other than a read reaches the end of a page. So you'd need
a lot of concurrent READDIRs of large directories or something. Still,
I'd think it would be possible.
> Thank you, YueHaibing!
>
> Bruce, do you want to shepherd this one in?
Yes, I've got 3 bugfixes queued up now, I should send them along later
today or tomorrow.
--b.
