sunrpc: handle ENOMEM in rpcb_getport_async
Subsystems (3)
PATCH 1/1 sunrpc: handle ENOMEM in rpcb_getport_async
From: "J. Bruce Fields" <[email protected]>
If we ignore the error we'll hit a null dereference a little later.
--- net/sunrpc/rpcb_clnt.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Looks like this is still a bug?
net/sunrpc/rpcb_clnt.c +8 -0
@@ -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:
Signature · 1 line
2.19.2
PATCH 1/1 sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS
From: "J. Bruce Fields" <[email protected]>
It's OK to sleep here, we just don't want to recurse into the filesystem as this writeout could be waiting on this.
Future work: 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 be arranging that for us in the case of workqueues created with WQ_MEM_RECLAIM?
--- net/sunrpc/rpcb_clnt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Also, I've still got this one. (And still haven't looked into whether it should be using a memalloc_nofs_{save,restore} elsewhere instead.)
net/sunrpc/rpcb_clnt.c +2 -2
@@ -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",
Signature · 1 line
2.19.2
Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS
Quoted 8 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
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 writeout"
Quoted 8 lines
Future work: the documentation for
GFP_NOFSsays "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 inheritGFP_NOFSimplicitly."But I'm not sure where to do this. Should the workqueue be arranging that for us in the case of workqueues created with
WQ_MEM_RECLAIM?
There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. That sounds like a big project.
Quoted 34 lines
Reported-by Trond Myklebust <[email protected]>Signed-off-by J. Bruce Fields <[email protected]>---
net/sunrpc/rpcb_clnt.c| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)Also, I've still got this one. (And still haven't looked into whether it should be using a memalloc_nofs_{save,restore} elsewhere instead.)
net/sunrpc/rpcb_clnt.c +2
-2@@ -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",Signature · 1 line
2.19.2
--
Chuck Lever
Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS
Quoted 10 lines · Chuck Lever
Quoted 8 lines · J. Bruce Fields
From: "J. Bruce Fields" <[email protected]>
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 writeout"
Oops, thanks.
Quoted 11 lines
Quoted 8 lines
Future work: the documentation for
GFP_NOFSsays "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 inheritGFP_NOFSimplicitly."But I'm not sure where to do this. Should the workqueue be arranging that for us in the case of workqueues created with
WQ_MEM_RECLAIM?There seem to be plenty of uses of
GFP_NOFSin NFS and sunrpc. That sounds like a big project.
Yeah, just noting it for future reference.
--b.
Quoted 39 lines
Quoted 34 lines
Reported-by Trond Myklebust <[email protected]>Signed-off-by J. Bruce Fields <[email protected]>---
net/sunrpc/rpcb_clnt.c| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)Also, I've still got this one. (And still haven't looked into whether it should be using a memalloc_nofs_{save,restore} elsewhere instead.)
net/sunrpc/rpcb_clnt.c +2
-2@@ -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",Signature · 1 line
2.19.2--
Chuck Lever
Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS
Quoted 5 lines · Bruce Fields
Quoted 4 lines · Chuck Lever
Quoted 4 lines · J. Bruce Fields
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
From: "J. Bruce Fields" <[email protected]>
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 writeout"
Oops, thanks.
(Trond or Anna, I'm assuming you can fix that up, but let me know if you'd rather I resent.)
--b.
Quoted 56 lines
Quoted 7 lines
Quoted 7 lines
Future work: the documentation for
GFP_NOFSsays "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 inheritGFP_NOFSimplicitly."Quoted 5 lines
Quoted 2 lines
But I'm not sure where to do this. Should the workqueue be arranging that for us in the case of workqueues created with
WQ_MEM_RECLAIM?There seem to be plenty of uses of
GFP_NOFSin NFS and sunrpc. That sounds like a big project.Yeah, just noting it for future reference.
--b.
Quoted 39 lines
Quoted 34 lines
Reported-by Trond Myklebust <[email protected]>Signed-off-by J. Bruce Fields <[email protected]>---
net/sunrpc/rpcb_clnt.c| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)Also, I've still got this one. (And still haven't looked into whether it should be using a memalloc_nofs_{save,restore} elsewhere instead.)
net/sunrpc/rpcb_clnt.c +2
-2@@ -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",Signature · 1 line
2.19.2--
Chuck Lever
Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS
In reply to Bruce FieldsQuoted 30 lines · Bruce Fields
Quoted 11 lines · Chuck Lever
Quoted 9 lines
On Dec 20, 2018, at 10:42 AM, J. Bruce Fields <
[email protected]> wrote:From: "J. Bruce Fields" <[email protected]>
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 writeout"
Oops, thanks.
Quoted 15 lines
Quoted 14 lines
Future work: the documentation for
GFP_NOFSsays "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 inheritGFP_NOFSimplicitly."But I'm not sure where to do this. Should the workqueue be arranging
that for us in the case of workqueues created withWQ_MEM_RECLAIM?
Quoted 5 lines
Quoted 2 lines
There seem to be plenty of uses of
GFP_NOFSin NFS and sunrpc. That sounds like a big project.Yeah, just noting it for future reference.
I'd suggest that we can probably just call memalloc_nofs_save() in rpc_execute(), and otherwise in those workqueue callback functions that are executed directly by rpciod and xprtiod. That doesn't make for too many callsites.
Signature · 3 lines
Trond Myklebust Linux NFS client maintainer, Hammerspace [email protected]