Handle NULL in releasedir #575
Conversation
Whilst libfuse seems to indicate this should not occur when `nullpath_ok` is switched off, this does not always seem to be the case. A straightforward workaround is to handle the NULL case of `path` in `lxcfs_releasedir` such that no crash can occur in snaps. Signed-off-by: Matthew Ife <75210448+deleriux@users.noreply.github.com>
| @@ -771,6 +771,13 @@ static int lxcfs_releasedir(const char *path, struct fuse_file_info *fi) | |||
| { | |||
| int ret; | |||
|
|
|||
| if (path == NULL) { | |||
| up_users(); | |||
| ret = do_cg_releasedir(path, fi); | |||
There was a problem hiding this comment.
why we are calling do_cg_releasedir here? Why not do_sys_releasedir? Without path we can't determine what to call here. I think we have to print out some error to the LXCFS log and return -EINVAL.
There was a problem hiding this comment.
I'm not familiar with libfuse code but fi seems to always be initialized.
Both do_cg_releasedir and do_sys_releasedir just call (via dlopen()) do_release_file_info.
I am (potentially unsafely) assuming that whatever user data exists in fi will always be the same struct being released here.
The problem with not doing anything at all is may end up just slowly leaking memory.
Passing -EINVAL will result in unexpected behaviour in userspace applications. The POSIX norm for files unlinked/rmdir'd from VFS with existing open handles that are subsequently closed by the application is to return 0 to the application and not -EINVAL.
Spitting a log output might keep the process running but its pretty unlikely with the information kept in the userspace structure will reveal what the original path was that was rmdir'd. You could extend the userspace structure to add tracking though.
There was a problem hiding this comment.
I am (potentially unsafely) assuming that whatever user data exists in fi will always be the same struct being released here.
Yep, that's what I'm trying to say. Right now that makes no difference to call do_cg_releasedir or do_sys_releasedir but in the future, this may be changed. We can't rely on that.
The problem with not doing anything at all is may end up just slowly leaking memory.
Yep, but a memory leak combined with the warning message in logs is better than potentially incorrect memory free ;-)
Passing -EINVAL will result in unexpected behaviour in userspace applications.
I've checked how this retvalue is handled in libfuse... and... it's just ignored. :-)
https://github.com/libfuse/libfuse/blob/33736958b61d90d9db9b03441103035316f09abc/lib/fuse.c#L1982
Take a look on fuse_fs_releasedir() calls.
Spitting a log output might keep the process running but its pretty unlikely with the information kept in the userspace structure will reveal what the original path was that was rmdir'd. You could extend the userspace structure to add tracking though.
Yep, but this warning will be a sign of a problem. And yes, extending the log message with some additional info, in this case, is a good idea.
There was a problem hiding this comment.
I've checked how this retvalue is handled in libfuse... and... it's just ignored. :-)
Thats great! I agree in that case leak/EINVAL is nicer.
If I can get the time approved I shall try to work out what the condition is that results in this behaviour as it might reveal something more fundamental going on.
There was a problem hiding this comment.
If I can get the time approved I shall try to work out what the condition is that results in this behaviour as it might reveal something more fundamental going on.
I've checked libfuse code and I can only see 3 places where NULL path may be passed to fs callbacks:
releasedircallback https://github.com/libfuse/libfuse/blob/33736958b61d90d9db9b03441103035316f09abc/lib/fuse.c#L3731releasecallback https://github.com/libfuse/libfuse/blob/33736958b61d90d9db9b03441103035316f09abc/lib/fuse.c#L4075flushcallback https://github.com/libfuse/libfuse/blob/33736958b61d90d9db9b03441103035316f09abc/lib/fuse.c#L4097
I think we have to cover all of them from the LXCFS side. Are you ready to update this PR with covering these cases?
As we discussed before if we are getting a NULL path we can just throw an error and print a verbose log message with struct fuse_file_info fields, plus struct file_info too.
There was a problem hiding this comment.
BTW, we can use (struct file_info)->type to detect what to call do_cg_releasedir or do_sys_releasedir... Then we don't need to know the path at all in the release* callbacks!
There was a problem hiding this comment.
With a bit of hacking, I managed to resolve the userspace component of the coredumps I have in the backtraces.
These are 3 different crash examples.
(gdb) print *(struct file_info *)fi->fh
$2 = {controller = 0x7f0408105af0 "systemd",
cgroup = 0x7f04081075e0 "lxc.payload.infra-app-test-31/system.slice/snap.mount", file = 0x0 <foo>,
type = 0, buf = 0x0 <foo>, buflen = 0, size = 779116910, cached = 1853189997}
And
(gdb) print *(struct file_info *)fi->fh
$1 = {controller = 0x7f02f4106a50 "devices", cgroup = 0x7f02f4118740 "lxc.payload.infra-app-test-33/system.slice/sssd-pac.socket", file = 0x0 <foo>, type = 0, buf = 0x0 <foo>, buflen = 0,
size = 778989426, cached = 1987208563}
And
(gdb) print *(struct file_info *)fi->fh
$4 = {controller = 0x7f9f90000ca0 "systemd", cgroup = 0x7f9f9000d470 "lxc.payload.infra-app-test-31/system.slice/system-getty.slice", file = 0x0 <foo>, type = 0, buf = 0x0 <foo>, buflen = 0,
size = 779318626, cached = 1987208563}
We use a lot of cgroup v1 namespaces on these hosts, however (which I have no backtraces for unfortunately) we have another that exhibits the same behaviour that is using cgroupv2 only).
In any case they are always cgroup files that crash in my case.
We pretty much can hammer our hosts (having 150+ active containers at at time).
I will update the PR with code tomorrow that handles all 3 cases cleanly.
| @@ -1032,6 +1039,7 @@ static void *lxcfs_init(struct fuse_conn_info *conn) | |||
|
|
|||
| #if HAVE_FUSE3 | |||
| cfg->direct_io = 1; | |||
| cfg->nullpath_ok = 0; | |||
There was a problem hiding this comment.
I've asked that already https://github.com/lxc/lxd-pkg-snap/issues/110#issuecomment-1377472107
Did you catch the case when this option has a non-zero value? (I mean by printf-ing it's value or using gdb memory inspection).
|
Just to make a link #573 |
|
Let's close this PR cause we have #577 |
Whilst libfuse seems to indicate this should not occur when
nullpath_okis switched off, this does not always seem to be the case. A straightforward workaround is to handle the NULL case ofpathinlxcfs_releasedirsuch that no crash can occur in snaps.Example crash we caught a few days ago:
Signed-off-by: Matthew Ife 75210448+deleriux@users.noreply.github.com