lxcfs: handle NULL path in lxcfs_releasedir/lxcfs_release #577

deleriux · 2023-01-11T15:43:26Z

This fixes SEGV in lxcfs_release/releasedir when path=NULL.
Avoid the use of paths doing the release and prefer to inspect the type file_info struct.
Print an error in the case no valid type can be detected.

deleriux · 2023-01-11T15:47:14Z

This is the method I've approached this problem with we discussed yesterday.
I fleshed out the file type checks with some simple defines, then created a inline function to inspect the file type internally.

The release/releasedir code now primarily uses this mechanism to release files back to memory. It spits an error in the event it doesn't know what to do.

Flush is a noop so no work was required.

Might be a nice-to-have to extend the same checks into other callbacks as its a little quicker than a strcmp(), but that isn't the concern of this fix.

This still needs testing properly somewhere, I'll roll it out onto my own test systems tomorrow and check its not blowing up massively or leaking memory somehow.

mihalicyn · 2023-01-11T17:27:42Z

src/lxcfs.c

@@ -1032,6 +1059,7 @@ static void *lxcfs_init(struct fuse_conn_info *conn)

 #if HAVE_FUSE3
 	cfg->direct_io = 1;
+	cfg->nullpath_ok = 0;


it makes no sense to add this here, as we discussed before, cfg is always initialized by zeroes. If you want to add this just to make the code more explicit, then we need to explicitly initialize all other options from cfg structure, IMHO.

Dropped this change, agree its unnecessary and implies a cause where none exists.

mihalicyn · 2023-01-11T17:27:54Z

src/lxcfs.c

 		up_users();
 		ret = do_sys_release(path, fi);
 		down_users();
-		return ret;
+		return ret;	


extra spaces after ;

mihalicyn · 2023-01-11T17:45:23Z

This commit cb5f18a looks strange because later you are removing files from it 27f2cfd

Hint: you can use git rebase -i HEAD~N_commits to edit particular commits, and also you can use git add -i (interactive staging) simplify splitting changes between commits. Then git push -f for PR update.

deleriux · 2023-01-12T09:11:35Z

Thanks. Whilst I'm fairly proficient in C my git-etiquette is not unfortunately that great! I'll try to clean up my commits and remove the bits that are unnecessary.

As you can tell I took an indirect approach at first (to preserve the dlopen() consistency) but in the end I dont think it is necessary, extends the API without offering much to the end user and the extra runtime linker steps make it a slower constant approach than the strcmp() method.

I felt going to the effort of doing type-checks should at least potentially result in a slightly faster overall runtime experience.

mihalicyn · 2023-01-12T10:02:28Z

src/bindings.h

@@ -31,6 +31,7 @@
 #define LXCFS_NUMSTRLEN64 21

 enum lxcfs_virt_t {
+	LXC_TYPE_START,


I'm not sure that it's safe to change the start value of this enum. Cause we support live lxc library reload after this change LXC_TYPE_CGDIR becomes 1 instead of 0. These values getting placed in the memory and kept between library reloads. So, let's not change this enum declaration at all. You can just place a comment inside enum definition which attracts developer attention to the macros LXCFS_TYPE_CGROUP, LXCFS_TYPE_PROC, LXCFS_TYPE_SYS.

I agree here, totally didn't think of the case of reloading.

Generally we restart the service and rebind the mounts in the containers - there is a neat way to do this without restarting the containers, I'm still developing the software but am happy to share it later down the line.

I agree here, totally didn't think of the case of reloading.

Generally we restart the service and rebind the mounts in the containers - there is a neat way to do this without restarting the containers, I'm still developing the software but am happy to share it later down the line.

great! I'm working on a way to recover the lxcfs from crashes another way. With preserving fuse connection between userspace and kernel. It's also not ready yet :)

I'm not sure that it's safe to change the start value of this enum. Cause we support live lxc library reload after this change LXC_TYPE_CGDIR becomes 1 instead of 0. These values getting placed in the memory and kept between library reloads. So, let's not change this enum declaration at all.

I think we should explictly number it though. That's what I do nowadays for all enums I add becuase it eliminates a whole bunch of subtle bugs if you force the numbering.

mihalicyn · 2023-01-12T10:03:57Z

src/bindings.h

+#define LXCFS_TYPE_CGROUP(type) (type >= LXC_TYPE_CGDIR && type <= LXC_TYPE_CGFILE)
+#define LXCFS_TYPE_PROC(type) (type >= LXC_TYPE_PROC_MEMINFO && type <= LXC_TYPE_PROC_SLABINFO)
+#define LXCFS_TYPE_SYS(type) (type >= LXC_TYPE_SYS && type <= LXC_TYPE_SYS_DEVICES_SYSTEM_CPU_ONLINE)
+#define LXCFS_TYPE_OK(type) (type >= LXC_TYPE_START && type <= LXC_TYPE_END)


#define LXCFS_TYPE_OK(type) (type > LXC_TYPE_START && type < LXC_TYPE_END), correct?

But anyway, I think we don't need to have this separate values LXC_TYPE_START and LXC_TYPE_END.

#define LXCFS_TYPE_OK(type) (type >= LXC_TYPE_CGDIR && type <= LXC_TYPE_SYS_DEVICES_SYSTEM_CPU_ONLINE) looks sufficient

I saw and fixed this one earlier (the range checks being incorrect) might be best practice to keep the LXC_TYPE_END, it prevents needing to adjust LXCFS_TYPE_OK macro later on in the future and its pretty obvious to anyone going forwards not to add to the end of it.

agree with you :)

mihalicyn · 2023-01-12T10:09:34Z

src/lxcfs.c

+	if (path && strcmp(path, "/") == 0) 
+		return 0;
+
+	lxcfs_error("lxcfs_releasedir() unknown file type: path=%s, type=%d\n",


you don't need extra \n as lxcfs_error prints a new line automatically

Fixed in latest commit.

mihalicyn · 2023-01-12T10:10:06Z

src/lxcfs.c

 	}

+	lxcfs_error("lxcfs_release() unknown file type: path=%s, type=%d\n",


\n is excess

deleriux · 2023-01-12T11:14:47Z

I've made the changes and cleaned up the commit history somewhat. I'll test the deployment out and see how it fares today.

mihalicyn · 2023-01-12T11:19:56Z

src/bindings.h

+#define LXCFS_TYPE_CGROUP(type) (type >= LXC_TYPE_CGDIR && type <= LXC_TYPE_CGFILE)
+#define LXCFS_TYPE_PROC(type) (type >= LXC_TYPE_PROC_MEMINFO && type <= LXC_TYPE_PROC_SLABINFO)
+#define LXCFS_TYPE_SYS(type) (type >= LXC_TYPE_SYS && type <= LXC_TYPE_SYS_DEVICES_SYSTEM_CPU_ONLINE)
+#define LXCFS_TYPE_OK(type) (type > LXC_TYPE_CGDIR && type < LXC_TYPE_END)


#define LXCFS_TYPE_OK(type) (type >= LXC_TYPE_CGDIR && type < LXC_TYPE_END)?

Spotted and fixed myself already. Thanks.

mihalicyn · 2023-01-12T11:24:24Z

src/lxcfs.c

 		up_users();
 		ret = do_sys_release(path, fi);
 		down_users();
 		return ret;
 	}

+	lxcfs_error("lxcfs_release() unknown file type: path=%s, type=%d",


lxcfs_error already prints the function name from where it's called ;)

mihalicyn · 2023-01-12T11:24:49Z

src/lxcfs.c

+	if (path && strcmp(path, "/") == 0)
+		return 0;
+
+	lxcfs_error("lxcfs_releasedir() unknown file type: path=%s, type=%d",


function name is already printed inside the lxcfs_error

mihalicyn · 2023-01-12T11:30:19Z

Thanks @deleriux for working on that. I think it's also important to mention in the commit message and PR description that you are trying to fix SEGV with this patch :) Because right now, at first glance, it looks just like an improvement. But in fact, this is a bugfix. Let's rename PR too to reflect the reason why you are doing all of that :)

mihalicyn · 2023-01-12T11:31:20Z

Let's call other reviewers :)

cc @tych0

mihalicyn

the first line in the commit message should be like lxcfs: handle NULL path in lxcfs_releasedir/lxcfs_release.

And please rename PR accordingly.

deleriux · 2023-01-12T12:31:12Z

../src/lxcfs.c: 813: lxcfs_releasedir: unknown file type: path=/proc, type=-1
Looks like there is more work here to do to handle base paths in /proc which aren't typically designated with a type themselves.

deleriux · 2023-01-12T12:44:41Z

Fixed the latest minor niggle. There is no need to check for this in lxcfs_release since the inode is always a directory.
I also changed the commit message as reflected.

mihalicyn · 2023-01-12T13:18:56Z

src/lxcfs.c

 		up_users();
 		ret = do_sys_releasedir(path, fi);
 		down_users();
 		return ret;
 	}

+	if (LXCFS_TYPE_PROC(type))


you don't need this check at all then. Cause for /proc directory we don't fill fi with any info in lxcfs_opendir:

static int lxcfs_opendir(const char *path, struct fuse_file_info *fi) { int ret; if (strcmp(path, "/") == 0) return 0; // don't touch "fi" if (strncmp(path, "/cgroup", 7) == 0) { up_users(); ret = do_cg_opendir(path, fi); // << fills "fi" with something down_users(); return ret; } if (strcmp(path, "/proc") == 0) return 0; // don't touch "fi" if (strncmp(path, "/sys", 4) == 0) { up_users(); ret = do_sys_opendir(path, fi); // << fills "fi" with something down_users(); return ret; } return -ENOENT; }

Excellent point. I should try to extend my head-logic a bit further ;). Updated.

mihalicyn

LGTM

Thanks!

mihalicyn · 2023-01-12T14:02:24Z

And yes, one small suggestion. We can replace lxcfs_error("unknown file type: path=%s, type=%d", path, type); to lxcfs_error("unknown file type: path=%s, type=%d, fi->fh=%" PRIu64, path, type, fi->fh);

deleriux · 2023-01-12T14:12:28Z

Fixed to include the pointer address. I assume you really only care if its a 0 or close to it though...

mihalicyn · 2023-01-12T14:14:00Z

Fixed to include the pointer address. I assume you really only care if its a 0 or close to it though...

in fact yes. But if we will have a coredump in a particular case this address may help to debug.

mihalicyn · 2023-01-12T14:14:53Z

src/lxcfs.c

 		up_users();
 		ret = do_sys_release(path, fi);
 		down_users();
 		return ret;
 	}

+	lxcfs_error("unknown file type: path=%s, type=%d, type=%" PRIu64,


type= to fi->fh=

oof.. thanks.

brauner · 2023-01-12T14:23:13Z

src/bindings.h

@@ -67,8 +67,15 @@ enum lxcfs_virt_t {

 	LXC_TYPE_SYS_DEVICES_SYSTEM_CPU_ONLINE,
 #define LXC_TYPE_SYS_DEVICES_SYSTEM_CPU_ONLINE_PATH "/sys/devices/system/cpu/online"
+	LXC_TYPE_END,


Traditionally, we've suffixed things like that with MAX so I'd prefer LXC_TYPE_MAX.

Changed style from END to MAX.

Added the requested comment also.

brauner · 2023-01-12T14:31:06Z

src/bindings.h

+#define LXCFS_TYPE_CGROUP(type) (type >= LXC_TYPE_CGDIR && type <= LXC_TYPE_CGFILE)
+#define LXCFS_TYPE_PROC(type) (type >= LXC_TYPE_PROC_MEMINFO && type <= LXC_TYPE_PROC_SLABINFO)
+#define LXCFS_TYPE_SYS(type) (type >= LXC_TYPE_SYS && type <= LXC_TYPE_SYS_DEVICES_SYSTEM_CPU_ONLINE)
+#define LXCFS_TYPE_OK(type) (type >= LXC_TYPE_CGDIR && type < LXC_TYPE_END)


This is error prone as it depends on ordering in the enum. The ordering isn't enforced however. So there needs to be at least a comment on top of the enum explaining this. Also, we should explicitly number the enum. This also makes it harder to accidently break ordering requirements.

I thought about that too, after we extend this enum with new possible values it will be needed to fix these macros like that:
#define LXCFS_TYPE_PROC(type) ((type >= LXC_TYPE_PROC_MEMINFO && type <= LXC_TYPE_PROC_SLABINFO) || type == LXC_TYPE_PROC_SOMETHING). Not so fancy, but...

Technically, right now it's also not correct to add new values in between the enum. Cause we can perform reloading of liblxcfs in between open and close of the file descriptor. Then because enum values shift, some condition checks may become incorrect. For example https://github.com/lxc/lxcfs/blob/master/src/sysfs_fuse.c#L372

I also did give this some thought beforehand. I thought some ways to do this could be..

re-order the enum numbering explicitly granting space for future files. IE

enum lxcfs_virt_t { LXC_TYPE_CGROUP_BASE = 0x00000, ... LXC_TYPE_CGROUP_MAX, LXC_TYPE_PROC_BASE = 0x10000, ... LXC_TYPE_PROC_MAX, LXC_TYPE_SYS_BASE = 0x20000, ... LXC_TYPE_SYS_MAX, }

extend file_info to include a 'class' parameter, ditch the defines, remove file_info_type in favour of file_info_class.

For 2. Obviously you'd basically have to go through every file type already configured and put them in the right class, but at that point ordering no longer matters as you're relying on a different set of metadata to work out what goes where..

Also you could hijack the unused file_info.buf pointer and change it to a unsigned char abi_version[sizeof(char *)] that you could check going forwards. This would let you conditionally and smartly extend the runtime ABI yet be able to deal with previous versions of the ABI changing.
This is of course assuming *buf is zeroed on initialization currently (version 0 implictly).
This also felt like a whole load of architectural work for what is essentially a bugfix!

For 1. Its much simpler to implement and prevents in the future having to make a headache again. That however breaks the ABI in a manner which would not survive a reload so didn't imagine that would get any traction..

Just also thought you could actually do LXC_TYPE_BASE = 0x00000, copy all the file types as-is, then have new namespaces for each type and defines for which you use going forwards. That gives you both the means to maintain reload compatibility by checking if the type is in a 'base' namespace or the given correct namespace.

Its a little janky but probably simpler than doing some abi extension.

For now just leaving a comment is fine. We just want to fix the immediate bug. Reworking it to something better should be a separate step. But feel free to follow this up with a series.

Added the requested comment above the enum indicating only appending to the bottom (above LXC_TYPE_MAX).

tych0 · 2023-01-12T14:55:57Z

Can you talk about what case the path would be null? At first glance this seems like a weird call for libfuse to make.

mihalicyn · 2023-01-12T14:58:14Z

Can you talk about what case the path would be null? At first glance this seems like a weird call for libfuse to make.

#575 (comment)

tych0 · 2023-01-12T15:02:32Z

So I guess it's really the kernel, not libfuse, that's triggering these, given that they're just passthroughs from libfuse? I still don't understand why, but maybe it's a mystery for the ages...

mihalicyn · 2023-01-12T15:10:40Z

So I guess it's really the kernel, not libfuse, that's triggering these, given that they're just passthroughs from libfuse? I still don't understand why, but maybe it's a mystery for the ages...

As far as I know, the kernel doesn't know anything about the path, or, speaking more correctly, the kernel gives the path from open*() syscalls to fuse daemon only for the first time through a FUSE_LOOKUP [1] request, then userspace should provide the kernel with inode number for this file descriptor. The later kernel will only send this number as file identification [2]. So, libfuse handles this path resolve for our convenience. I feel that we have some bugs in libfuse, we have to add some workaround in our code to prevent crashes at least, IMHO.

[1] https://github.com/torvalds/linux/blob/764822972d64e7f3e6792278ecc7a3b3c81087cd/fs/fuse/dir.c#L190
[2] https://github.com/torvalds/linux/blob/764822972d64e7f3e6792278ecc7a3b3c81087cd/fs/fuse/file.c#L40

tych0 · 2023-01-12T15:13:53Z

Yeah, that's what I was wondering about. Makes sense, thanks. Patch LGTM.

Implement a file_info_type function. Create a series of defines for the file type Inspect the file type for each file handler and perform the relevant release code to free any memory. If the type cannot be determined, print an error and return -EINVAL. This should also be slightly faster than a strcmp() but its not likely that measurable. This code could be used elsewhere in the process to reduce the strcmp requirements, but for now just handle the release/releasedir case. This fixes SEGV in lxcfs_relase/releasedir when path=NULL but invoked a strcmp(). Signed-off-by: Matthew Ife <matthewi@mustardsystems.com>

deleriux force-pushed the file_info_api branch from afc9a6d to 27f2cfd Compare January 11, 2023 16:22

mihalicyn requested changes Jan 11, 2023

View reviewed changes

mihalicyn requested changes Jan 12, 2023

View reviewed changes

deleriux force-pushed the file_info_api branch 2 times, most recently from 0b749c9 to 04277d9 Compare January 12, 2023 11:13

mihalicyn requested changes Jan 12, 2023

View reviewed changes

deleriux force-pushed the file_info_api branch from 04277d9 to 4e09af4 Compare January 12, 2023 11:26

mihalicyn mentioned this pull request Jan 12, 2023

Handle NULL in releasedir #575

Closed

deleriux force-pushed the file_info_api branch 2 times, most recently from 6bc0e36 to 478e579 Compare January 12, 2023 11:48

mihalicyn reviewed Jan 12, 2023

View reviewed changes

deleriux force-pushed the file_info_api branch from 478e579 to d8a7f91 Compare January 12, 2023 12:42

deleriux changed the title ~~File info api~~ lxcfs: handle NULL path in lxcfs_releasedir/lxcfs_release Jan 12, 2023

mihalicyn requested changes Jan 12, 2023

View reviewed changes

deleriux force-pushed the file_info_api branch from d8a7f91 to 1ca4c46 Compare January 12, 2023 13:26

mihalicyn previously approved these changes Jan 12, 2023

View reviewed changes

deleriux dismissed mihalicyn’s stale review via 7791802 January 12, 2023 14:11

deleriux force-pushed the file_info_api branch from 1ca4c46 to 7791802 Compare January 12, 2023 14:11

mihalicyn requested changes Jan 12, 2023

View reviewed changes

deleriux force-pushed the file_info_api branch from 7791802 to 4979d6b Compare January 12, 2023 14:19

mihalicyn previously approved these changes Jan 12, 2023

View reviewed changes

brauner requested changes Jan 12, 2023

View reviewed changes

deleriux dismissed mihalicyn’s stale review via f4153a9 January 12, 2023 15:29

deleriux force-pushed the file_info_api branch from 4979d6b to f4153a9 Compare January 12, 2023 15:29

deleriux force-pushed the file_info_api branch from f4153a9 to cf02ccb Compare January 12, 2023 16:10

mihalicyn approved these changes Jan 12, 2023

View reviewed changes

brauner approved these changes Jan 12, 2023

View reviewed changes

brauner merged commit 0f7fe14 into lxc:master Jan 12, 2023

81981266 mentioned this pull request Apr 24, 2024

lxcfs: handle NULL in lxcfs_read (segfault at 0, code=killed, status=11/SEGV) #635

Open

		}

		lxcfs_error("lxcfs_release() unknown file type: path=%s, type=%d\n",

lxcfs: handle NULL path in lxcfs_releasedir/lxcfs_release #577

lxcfs: handle NULL path in lxcfs_releasedir/lxcfs_release #577

Conversation

deleriux commented Jan 11, 2023 • edited Loading

deleriux commented Jan 11, 2023

Choose a reason for hiding this comment

deleriux Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalicyn commented Jan 11, 2023 • edited Loading

deleriux commented Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalicyn Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deleriux commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalicyn commented Jan 12, 2023 • edited Loading

mihalicyn commented Jan 12, 2023 • edited Loading

mihalicyn left a comment

Choose a reason for hiding this comment

deleriux commented Jan 12, 2023

deleriux commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihalicyn left a comment

Choose a reason for hiding this comment

mihalicyn commented Jan 12, 2023

deleriux commented Jan 12, 2023

mihalicyn commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deleriux Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deleriux Jan 12, 2023 • edited Loading

Choose a reason for hiding this comment

tych0 commented Jan 12, 2023

mihalicyn commented Jan 12, 2023

tych0 commented Jan 12, 2023

mihalicyn commented Jan 12, 2023 • edited Loading

tych0 commented Jan 12, 2023

deleriux commented Jan 11, 2023 •

edited

Loading

deleriux Jan 12, 2023 •

edited

Loading

mihalicyn commented Jan 11, 2023 •

edited

Loading

deleriux commented Jan 12, 2023 •

edited

Loading

mihalicyn Jan 12, 2023 •

edited

Loading

mihalicyn commented Jan 12, 2023 •

edited

Loading

mihalicyn commented Jan 12, 2023 •

edited

Loading

deleriux Jan 12, 2023 •

edited

Loading

deleriux Jan 12, 2023 •

edited

Loading

mihalicyn commented Jan 12, 2023 •

edited

Loading