* [PATCH] sunrpc: handle ENOMEM in rpcb_getport_async @ 2018-12-20 15:35 J. Bruce Fields 2018-12-20 15:42 ` [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2018-12-20 15:35 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs From: "J. Bruce Fields" <bfields@redhat.com> If we ignore the error we'll hit a null dereference a little later. Reported-by: syzbot+4b98281f2401ab849f4b@syzkaller.appspotmail.com Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- net/sunrpc/rpcb_clnt.c | 8 ++++++++ 1 file changed, 8 insertions(+) Looks like this is still a bug? diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index c7872bc13860..08b5fa4a2852 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.19.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS 2018-12-20 15:35 [PATCH] sunrpc: handle ENOMEM in rpcb_getport_async J. Bruce Fields @ 2018-12-20 15:42 ` J. Bruce Fields 2018-12-20 15:47 ` Chuck Lever 0 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2018-12-20 15:42 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs From: "J. Bruce Fields" <bfields@redhat.com> 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? Reported-by: Trond Myklebust <trondmy@hammer.space> Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- 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.) diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index 08b5fa4a2852..41a971ac1c63 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.19.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS 2018-12-20 15:42 ` [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS J. Bruce Fields @ 2018-12-20 15:47 ` Chuck Lever 2018-12-20 15:52 ` Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: Chuck Lever @ 2018-12-20 15:47 UTC (permalink / raw) To: Bruce Fields; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List > On Dec 20, 2018, at 10:42 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > 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" > 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? There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. That sounds like a big project. > Reported-by: Trond Myklebust <trondmy@hammer.space> > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > 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.) > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > index 08b5fa4a2852..41a971ac1c63 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.19.2 > -- Chuck Lever ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS 2018-12-20 15:47 ` Chuck Lever @ 2018-12-20 15:52 ` Bruce Fields 2018-12-20 20:13 ` Bruce Fields 2018-12-21 6:37 ` Trond Myklebust 0 siblings, 2 replies; 6+ messages in thread From: Bruce Fields @ 2018-12-20 15:52 UTC (permalink / raw) To: Chuck Lever; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List On Thu, Dec 20, 2018 at 10:47:25AM -0500, Chuck Lever wrote: > > > > On Dec 20, 2018, at 10:42 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > 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. > > 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? > > There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. > That sounds like a big project. Yeah, just noting it for future reference. --b. > > Reported-by: Trond Myklebust <trondmy@hammer.space> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > 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.) > > > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > > index 08b5fa4a2852..41a971ac1c63 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.19.2 > > > > -- > Chuck Lever > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS 2018-12-20 15:52 ` Bruce Fields @ 2018-12-20 20:13 ` Bruce Fields 2018-12-21 6:37 ` Trond Myklebust 1 sibling, 0 replies; 6+ messages in thread From: Bruce Fields @ 2018-12-20 20:13 UTC (permalink / raw) To: Chuck Lever; +Cc: Trond Myklebust, Anna Schumaker, Linux NFS Mailing List On Thu, Dec 20, 2018 at 10:52:19AM -0500, Bruce Fields wrote: > On Thu, Dec 20, 2018 at 10:47:25AM -0500, Chuck Lever wrote: > > > > > > > On Dec 20, 2018, at 10:42 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > 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. > > > > 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? > > > > There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. > > That sounds like a big project. > > Yeah, just noting it for future reference. > > --b. > > > > Reported-by: Trond Myklebust <trondmy@hammer.space> > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > --- > > > 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.) > > > > > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > > > index 08b5fa4a2852..41a971ac1c63 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.19.2 > > > > > > > -- > > Chuck Lever > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS 2018-12-20 15:52 ` Bruce Fields 2018-12-20 20:13 ` Bruce Fields @ 2018-12-21 6:37 ` Trond Myklebust 1 sibling, 0 replies; 6+ messages in thread From: Trond Myklebust @ 2018-12-21 6:37 UTC (permalink / raw) To: bfields, chuck.lever; +Cc: schumakeranna, linux-nfs On Thu, 2018-12-20 at 10:52 -0500, Bruce Fields wrote: > On Thu, Dec 20, 2018 at 10:47:25AM -0500, Chuck Lever wrote: > > > > > On Dec 20, 2018, at 10:42 AM, J. Bruce Fields < > > > bfields@fieldses.org> wrote: > > > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > 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. > > > > 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? > > > > There seem to be plenty of uses of GFP_NOFS in 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. -- Trond Myklebust Linux NFS client maintainer, Hammerspace trond.myklebust@hammerspace.com ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-21 6:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-20 15:35 [PATCH] sunrpc: handle ENOMEM in rpcb_getport_async J. Bruce Fields 2018-12-20 15:42 ` [PATCH] sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS J. Bruce Fields 2018-12-20 15:47 ` Chuck Lever 2018-12-20 15:52 ` Bruce Fields 2018-12-20 20:13 ` Bruce Fields 2018-12-21 6:37 ` Trond Myklebust
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).