{"action":"created","issue":{"url":"https://api.github.com/repos/mar-file-system/marfs/issues/77","labels_url":"https://api.github.com/repos/mar-file-system/marfs/issues/77/labels{/name}","comments_url":"https://api.github.com/repos/mar-file-system/marfs/issues/77/comments","events_url":"https://api.github.com/repos/mar-file-system/marfs/issues/77/events","html_url":"https://github.com/mar-file-system/marfs/issues/77","id":125277952,"number":77,"title":"fuse thread can get interrupted in the middle of changing euid (allowing escalation?)","user":{"login":"jti-lanl","id":8238841,"avatar_url":"https://avatars.githubusercontent.com/u/8238841?v=3","gravatar_id":"","url":"https://api.github.com/users/jti-lanl","html_url":"https://github.com/jti-lanl","followers_url":"https://api.github.com/users/jti-lanl/followers","following_url":"https://api.github.com/users/jti-lanl/following{/other_user}","gists_url":"https://api.github.com/users/jti-lanl/gists{/gist_id}","starred_url":"https://api.github.com/users/jti-lanl/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jti-lanl/subscriptions","organizations_url":"https://api.github.com/users/jti-lanl/orgs","repos_url":"https://api.github.com/users/jti-lanl/repos","events_url":"https://api.github.com/users/jti-lanl/events{/privacy}","received_events_url":"https://api.github.com/users/jti-lanl/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/mar-file-system/marfs/labels/bug","name":"bug","color":"fc2929"}],"state":"open","locked":false,"assignee":{"login":"jti-lanl","id":8238841,"avatar_url":"https://avatars.githubusercontent.com/u/8238841?v=3","gravatar_id":"","url":"https://api.github.com/users/jti-lanl","html_url":"https://github.com/jti-lanl","followers_url":"https://api.github.com/users/jti-lanl/followers","following_url":"https://api.github.com/users/jti-lanl/following{/other_user}","gists_url":"https://api.github.com/users/jti-lanl/gists{/gist_id}","starred_url":"https://api.github.com/users/jti-lanl/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jti-lanl/subscriptions","organizations_url":"https://api.github.com/users/jti-lanl/orgs","repos_url":"https://api.github.com/users/jti-lanl/repos","events_url":"https://api.github.com/users/jti-lanl/events{/privacy}","received_events_url":"https://api.github.com/users/jti-lanl/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/mar-file-system/marfs/milestones/2","html_url":"https://github.com/mar-file-system/marfs/milestones/Open%20Science%20Campaign%20Storage","labels_url":"https://api.github.com/repos/mar-file-system/marfs/milestones/2/labels","id":1456238,"number":2,"title":"Open Science Campaign Storage","description":"These are features needed to support the Trinity Open Science computing campaign.","creator":{"login":"brettkettering","id":1959564,"avatar_url":"https://avatars.githubusercontent.com/u/1959564?v=3","gravatar_id":"","url":"https://api.github.com/users/brettkettering","html_url":"https://github.com/brettkettering","followers_url":"https://api.github.com/users/brettkettering/followers","following_url":"https://api.github.com/users/brettkettering/following{/other_user}","gists_url":"https://api.github.com/users/brettkettering/gists{/gist_id}","starred_url":"https://api.github.com/users/brettkettering/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brettkettering/subscriptions","organizations_url":"https://api.github.com/users/brettkettering/orgs","repos_url":"https://api.github.com/users/brettkettering/repos","events_url":"https://api.github.com/users/brettkettering/events{/privacy}","received_events_url":"https://api.github.com/users/brettkettering/received_events","type":"User","site_admin":false},"open_issues":10,"closed_issues":10,"state":"open","created_at":"2015-12-11T16:11:05Z","updated_at":"2016-01-06T22:28:55Z","due_on":"2016-01-04T07:00:00Z","closed_at":null},"comments":6,"created_at":"2016-01-06T22:28:55Z","updated_at":"2016-01-07T01:14:42Z","closed_at":null,"body":"I think the title is the best remaining explanation for what is going on in logs like the included one.  (Line numbers were added.)  I think the way this is provoked is to do a tab-complete while creating the command-line (which does a opendir/readdir/closedir), then run the command (which does a write).  The release_dir then might happen at the same time as the write.  This is all happening over an NFS export of the marfs FUSE mount.\r\n\r\nJust look at the push_user/pop_user calls.  Lines 3,4 and 24,25 show the normal push-pop behavior.  \r\n\r\nThen there's a weird scramble.  Log lines aren't necessarily an exact reflection of the real order of events, but this appears meaningful.  Line 30 is in the middle of push_user, having pushed the egid, and stored 0 (the root euid) for later recovery.  But this is interrupted (I presume) by a call to fuse_release, which calls push_user again, called as root, at line 34.  It must be called as root, because the destination gid is 0, which would have come from the fuse context.  This may be on behalf of the NFS mount (mounted as root on the client).\r\n\r\nThe push euid at line 35 must be for the getattr (because it is pushing a user ID, not uid 0).  Then the seteuid at line 36 is on behalf of root.\r\n\r\nSo, I think lines 30 and 35 go together, then the interrupt, then lines 31-36 (minus 35).\r\n\r\nIf these were just different threads writing to the log, then the values being stored for later recovery would be in different per-thread spots, so there should be no interference.  Each thread would restore its own defaults in pop_user().  But the pop_user calls are screwed up now, I think, having mixed up data.\r\n\r\nAt line 88, push_user is now escalating from 493 to root.  I put a question-mark on whether this is actually \"escalation\" or not, because I think this is actually the opposite -- the thread was running with euid 493 when root calls, at which point it is able to set euid to 0 on behalf of root.\r\n\r\nI've gone to extremes to make push and pop thread-safe, so I'm thinking this has to be the same thread handling one push_user, while in the middle of handling another one, which sounds like an interrupt.\r\n\r\nThis is awkward to engineer in the debugger.  Maybe a better approach is to add some log details, to show the thread ID and addresses of storage.  If it is indeed the same thread, then we can go to locking, or something.\r\n\r\nMaybe this only happens when NFS is exported on top of fuse?  We do not start fuse with '-o intr'.\r\n\r\n[foo.txt](https://github.com/mar-file-system/marfs/files/80425/foo.txt)"},"comment":{"url":"https://api.github.com/repos/mar-file-system/marfs/issues/comments/169515184","html_url":"https://github.com/mar-file-system/marfs/issues/77#issuecomment-169515184","issue_url":"https://api.github.com/repos/mar-file-system/marfs/issues/77","id":169515184,"user":{"login":"jti-lanl","id":8238841,"avatar_url":"https://avatars.githubusercontent.com/u/8238841?v=3","gravatar_id":"","url":"https://api.github.com/users/jti-lanl","html_url":"https://github.com/jti-lanl","followers_url":"https://api.github.com/users/jti-lanl/followers","following_url":"https://api.github.com/users/jti-lanl/following{/other_user}","gists_url":"https://api.github.com/users/jti-lanl/gists{/gist_id}","starred_url":"https://api.github.com/users/jti-lanl/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jti-lanl/subscriptions","organizations_url":"https://api.github.com/users/jti-lanl/orgs","repos_url":"https://api.github.com/users/jti-lanl/repos","events_url":"https://api.github.com/users/jti-lanl/events{/privacy}","received_events_url":"https://api.github.com/users/jti-lanl/received_events","type":"User","site_admin":false},"created_at":"2016-01-07T01:14:42Z","updated_at":"2016-01-07T01:14:42Z","body":"Okay, some insight.  The thread-safety is fine.  And there is no interrupt.  It really is two different threads running at the same time.\n\nHere's the joke:  when any thread calls seteuid(), it affects the entire process!   Therefore, whoever calls it last wins.   [... sound of palm striking forehead]\n\nThere's another thing called setfsuid() that looks like maybe what we want.   (Hmm.  \"set file-system uid\".   Curious.)\n\nThis page refers to some issues with access() and quota(), but it dates back to 2009.  They also mention some ugliness with capabilities.\n\n    http://stackoverflow.com/questions/1223600/change-uid-gid-only-of-one-thread-in-linux\n\n\n\nOn Jan 6, 2016, at 4:27 PM, Jeff Inman <jti@lanl.gov> wrote:\n\n> I thought it would be thread-safe in the current implementation, in which saved_egid and saved_euid both come from per-thread storage allocated via pthread_key_create() and friends.\n> \n> I only went to the bizarre extent of using pthread primitives to allocate per-thread storage because I was running into (something like) this same problem when I was using local variables, and assumed ... well, it was dumb.  As we discussed, you'd expect local variables to live on the stack and also be thread-safe.\n> \n> So, that makes me think this really is thread-safe, which implies that it must be the same thread that is handling the two different calls that appear mixed together in the log. That's why I figured this must be some kind of interrupt thing.  I could still be wrong about all this.  I'm setting up some tests printing thread-IDs into the log.\n> \n> Thanks for the link, and the discussion.  The comment you referred to seems to suggest the same thing I was starting to suspect, which is that I might need locking.  I assumed that if I needed locking I was doing it wrong.   Dang it.  But it looks like PLFS fuse had to do it, too.\n> \n> Thanks.\n> \n> \n> On Jan 6, 2016, at 4:09 PM, John Bent <notifications@github.com> wrote:\n>> Perhaps at line 172 in https://github.com/mar-file-system/marfs/blob/master/fuse/src/main.c, the call to *saved_egid = getegid(); is not thread-safe. So if you get it after someone else has temporarily set it, then you get the wrong value and subsequently restore it. You may need a mutex or something. Hmm, that actually shouldn't be the case since getegid() is supposed to be different for each thread so one thread calling setegid() shouldn't screw up another thread calling getegid(). It could only be screwed up if the same thread was interrupted by a signal handler or something which seems unlikely. In any case, if you add process and thread ids to your nice foo.txt log file, that might help debug this as well.\n>> \n>> Generally, I wonder also if you shouldn't call getpwuid() instead of getegid()? getpwuid() won't be influenced by calls to setegid() so it should be safer.\n>> \n>> It's not super straightforward but you might try reading the comment at line 698 in the corresponding PLFS code: https://github.com/plfs/plfs-core/blob/master/fuse/plfs_fuse.cpp. I think it's related and I think that function basically implements correctly what you need here. Although I have to admit I'm generally confused by this so this might all be wrong...\n>> \n>> —\n>> Reply to this email directly or view it on GitHub.\n>> \n> \n\n"}}