Take a XPRT ref when hooking events #204

dang · 2020-02-21T18:28:47Z

The epoll loop needs a ref on the XPRT, so that it stays alive while in
epoll(). svc_rqst_rearm_events_locked() takes a ref, but the initial
svc_rqst_hook_events() does not. This means that, if the XPRT times
out, the cleanup will drop a ref, and the epoll loop will drop a ref,
causing one to many refs to be dropped. This leaves the client
potentially with a dangling ref.

Take the ref for the epoll loop when initially hooking the events.

Signed-off-by: Daniel Gryniewicz [email protected]

malahal

Will test it out and let you know.

madhuthorat · 2020-02-23T13:51:51Z

The fix helped to avoid the crash (avoiding use after free of xprt).
But when we ran cthon tests, they failed as lot of connections to statd remain open. Eventually we run out of FDs and SM_MON requests fail.

In /var/log/messages:
Feb 23 17:03:17 mynode rpc.statd[1545]: Failed to insert: creating /var/lib/nfs/statd/sm/10.0.100.51: Too many open files
Feb 23 17:03:17 mynode rpc.statd[1545]: STAT_FAIL to mynode.in.ibm.com for SM_MON of 10.0.100.51
Feb 23 17:03:17 mynode rpc.statd[1545]: Failed to insert: creating /var/lib/nfs/statd/sm/10.0.100.51: Too many open files
Feb 23 17:03:17 mynode rpc.statd[1545]: STAT_FAIL to mynode.in.ibm.com for SM_MON of 10.0.100.51

Following CRIT logs noticed:
2020-02-23 14:03:33 : epoch 00030945 : mynode : gpfs.ganesha.nfsd-32251[svc_30] nsm_connect :NLM :CRIT :connect to statd failed: RPC: Unknown protocol
2020-02-23 14:03:33 : epoch 00030945 : mynode : gpfs.ganesha.nfsd-32251[svc_30] nsm_monitor_noretry :NLM :CRIT :Monitor 10.0.100.52 nsm_connect failed
2020-02-23 14:03:34 : epoch 00030945 : mynode : gpfs.ganesha.nfsd-32251[svc_87] nsm_monitor_noretry :NLM :CRIT :Monitor 10.0.100.52 SM_MON failed (1)
2020-02-23 14:03:34 : epoch 00030945 : mynode : gpfs.ganesha.nfsd-32251[svc_87] nsm_monitor_noretry :NLM :CRIT :Monitor 10.0.100.52 SM_MON failed (1)

[mynode] # lsof -p $(pidof gpfs.ganesha.nfsd) | wc -l
1969

dang · 2020-02-28T20:03:13Z

@malahal Okay, this one seems to work properly. There's still a leak, but I think it's in Ganesha (I think the backchannel clients have a refcount bug). There's an associated Ganesha patch that needs to be merged along with this, or the listening sockets also leak:
dang/nfs-ganesha@55b9340

malahal · 2020-03-01T03:51:22Z

I read all documentation I could regarding epoll and threads! Looks like the API itself is broken. The best way to guaranty REF over epoll is taking REF while hooking and re-arming. If we only take REF while hooking and RELEASE while unhooking, unfortunately events CAN happen after unhook. There is no way to prevent execution of event code after unhook. This could lead to accessing freed memory!

The issue with REF/RELEASE while hooking/re-arming and event handing is that the "refcount" count can go to zero only after handing an "event". This seems to be the issue with leaking sockets with rpc.statd. A work around here could be to have an extra refcount in the back channel to avoid creating too many sockets.

malahal · 2020-03-01T05:17:19Z

Workaround for rpc.statd did work, but it leaks connections to portmapper and other sockets with remote clients with NLM. Maybe, unhook should have some fuzzy logic to wait for any additional events that could happen. Maybe, a sleep of one second (or socket pair based trick) may work.

dang · 2020-03-02T14:07:28Z

Hmm... This is an interesting problem. Let me think about it for a bit.

dang · 2020-03-03T19:32:47Z

Okay, this should close that hole. It works for me, and tests correctly (modulo the client leak mentioned above, which I need to look at next).

malahal · 2020-03-15T13:27:55Z

Found 2/3 issues with this code:
#1. UDP xprt refcount is going negative since it removed the additional ref that existed in old code
#2. TCP connections fron NFS clients are NOT getting closed. This code probably added an extra ref count for them. The old code was messy and somehow avoided this I think!
#3. connections initiated by ganesha to NFS clients (at nlockmgr port) are going to zero refcount but they are NOT getting closed due to not having CLNT_CREATE_FLAG_CLOSE flag. This might be an existing issue though.

https://github.com/malahal/ntirpc/commits/hook has fixes for #1, #2 and #3 (based on 2.7 code though). The top commit is just a clean up patch, but the following 3 commits should fix the issues reported here.

dang · 2020-03-17T14:32:00Z

I have 1 and 2 in my branch now. I'm not seeing how 3 can matter, since the hash table has a ref, and releases it at the end of DESTROY. If the RELEASE is freeing the xprt before the DESTROY, that's a bug.

malahal · 2020-03-20T05:56:51Z

I have 1 and 2 in my branch now. I'm not seeing how 3 can matter, since the hash table has a ref, and releases it at the end of DESTROY. If the RELEASE is freeing the xprt before the DESTROY, that's a bug.

The 3rd one is about not having SVC_XPRT_FLAG_CLOSE flag. The xprt memory gets freed but the fd would be still open without this flag, see "svc_vc_destroy_task()" for details.

If you are talking about my TOP commit that has re-order of destroy and release, then that is an issue if some other threads can call DESTROY on the xprt. I don't know if that can happen though. the idea seems to be that multiple threads could call SVC_DESTROY().

dang · 2020-03-23T13:46:50Z

Okay, so that branch doesn't have a fix for #3? I'm only seeing 3 commits there.

There is a race when unhooking events from epoll, where the event could be ready for delivery (or even delivered, but the thread not scheduled) and so the event is processed after the unhook, and therefore after the XPRT has been freed. To close this, stop putting a pointer to the rec in the event data, and instead put the FD in there and use it to look up the XPRT. This ensures that, if we got the XPRT from lookup, it's valid and ref'd for the duration of the event. Once we're no longer storing a XPRT pointer in the epoll event, we don't need a refcount across the hook/event/unhook series. Remove these refcounts, allowing a destroyed XPRT to just be freed. Signed-off-by: Daniel Gryniewicz <[email protected]>

malahal · 2020-04-04T17:44:03Z

I have 1 and 2 in my branch now. I'm not seeing how 3 can matter, since the hash table has a ref, and releases it at the end of DESTROY. If the RELEASE is freeing the xprt before the DESTROY, that's a bug.

Sorry, I must have rebased the branch. You suggested to fix the #3 in ganesha repo to avoid API change. The patch here in out ibm2.7 branch (ganesha repo though):

commit 5b67cd1
Author: Malahal Naineni [email protected]
Date: Tue Mar 17 01:13:56 2020 +0530

Fix closing fds started by ganesha as a client.

clnt_vc_ncreate() isn't passing CLNT_CREATE_FLAG_CLOSE flag leading to
sockets in CLOSE_WAIT state. Use the flag to close sockets when they
reach 0 refcount. Fix it by directly calling clnt_vc_ncreatef() with the
right flags.

Change-Id: I6d5b2de5d6afd0d543192bd6038db71bd52c20dd
Signed-off-by: Malahal Naineni <[email protected]>

dang · 2020-04-06T16:40:21Z

Okay, that looks good. I'll merge this, then.

malahal · 2020-04-07T11:53:59Z

Okay, that looks good. I'll merge this, then.

Thank you.

malahal reviewed Feb 21, 2020

View reviewed changes

dang force-pushed the hook branch 2 times, most recently from 3f4d585 to 80572c0 Compare February 28, 2020 19:55

dang force-pushed the hook branch from 80572c0 to 57aa974 Compare March 3, 2020 19:31

dang force-pushed the hook branch from 57aa974 to a3b420e Compare March 12, 2020 17:48

dang force-pushed the hook branch from a3b420e to 167c192 Compare March 17, 2020 14:42

dang force-pushed the hook branch from 167c192 to c7239b5 Compare March 23, 2020 13:50

malahal mentioned this pull request Apr 3, 2020

max connections exceeded nfs-ganesha/nfs-ganesha#561

Closed

dang merged commit 344a636 into nfs-ganesha:next Apr 6, 2020

dang deleted the hook branch April 6, 2020 16:40

dang mentioned this pull request Apr 6, 2020

VC - RELEASE after DESTROY #208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take a XPRT ref when hooking events #204

Take a XPRT ref when hooking events #204

dang commented Feb 21, 2020

malahal left a comment

madhuthorat commented Feb 23, 2020 •

edited

Loading

dang commented Feb 28, 2020

malahal commented Mar 1, 2020 •

edited

Loading

malahal commented Mar 1, 2020

dang commented Mar 2, 2020

dang commented Mar 3, 2020

malahal commented Mar 15, 2020 •

edited

Loading

dang commented Mar 17, 2020

malahal commented Mar 20, 2020

dang commented Mar 23, 2020

malahal commented Apr 4, 2020

dang commented Apr 6, 2020

malahal commented Apr 7, 2020

Take a XPRT ref when hooking events #204

Take a XPRT ref when hooking events #204

Conversation

dang commented Feb 21, 2020

malahal left a comment

Choose a reason for hiding this comment

madhuthorat commented Feb 23, 2020 • edited Loading

dang commented Feb 28, 2020

malahal commented Mar 1, 2020 • edited Loading

malahal commented Mar 1, 2020

dang commented Mar 2, 2020

dang commented Mar 3, 2020

malahal commented Mar 15, 2020 • edited Loading

dang commented Mar 17, 2020

malahal commented Mar 20, 2020

dang commented Mar 23, 2020

malahal commented Apr 4, 2020

dang commented Apr 6, 2020

malahal commented Apr 7, 2020

madhuthorat commented Feb 23, 2020 •

edited

Loading

malahal commented Mar 1, 2020 •

edited

Loading

malahal commented Mar 15, 2020 •

edited

Loading