fusefs: support FUSE_IOCTL #1470

CismonX · 2024-10-14T20:41:08Z

No description provided.

asomers

Interesting PR! Do you know of any real-world fuse file systems that implement FUSE_IOCTL? I couldn't find any back in 2019, which is why I never implemented this command.

asomers · 2024-10-14T20:44:30Z

tests/sys/fs/fusefs/ioctl.cc

+ * Copyright (c) 2019 The FreeBSD Foundation
+ *
+ * Portions of this software was developed by BFF Storage Systems, LLC under
+ * sponsorship from the FreeBSD Foundation.


You should update this copy/pasted copyright section.

I've updated the copyright year, and removed the "sponsored by" lines.

Speaking of which, I wonder if it's the correct way to transfer copyright to the FreeBSD Foundation.
Some projects require contributors to sign off a DCO for that, however, I didn't see anything like that
in the wikis.

We don't expect or require copyright assignment. We do encourage Signed-off-by on contributions via GitHub -- from CONTRIBUTING.md:

Commits should include one or more Signed-off-by: lines with full name and email address certifying Developer Certificate of Origin.

(That said, if there is a reason you do not want to be the copyright holder I can discuss that separately.)

CismonX · 2024-10-14T21:30:32Z

@asomers

any real-world fuse file systems that implement FUSE_IOCTL

None that I'm aware of.

However, one of my work-in-progress hobby projects (which is a pseudo filesystem for managing web browser bookmarks) requires ioctl for some of its major features.

I'd like to make it work on FreeBSD, otherwise it would be Linux-only (which is inconvenient for myself, since I'm primarily using FreeBSD for daily work and entertainment).

CismonX · 2024-10-29T14:13:49Z

Recently I came across two FUSE filesystems that implement FUSE_IOCTL.

One of them is bindfs. When launched with the -o enable-ioctl option, it enables ioctl forwarding.

However, it only works for the ioctls that do not require copying extra data between kernel and user address space, since FUSE servers are not allowed to perform "unrestricted ioctl".

Another is ntfs-3g. The only supported ioctl is FITRIM.

Although there is currently no FITRIM equivalent on FreeBSD, it's not difficult to port: Just adding a new entry in sys/filio.h, and porting fstrim(8) from util-linux would suffice.

Note: For the TRIM feature, ntfs-3g server requires BLKDISCARD internally. Since we already have DIOCGDELETE as its equivalent, this won't be a problem.

asomers · 2024-10-30T15:14:29Z

Recently I came across two FUSE filesystems that implement FUSE_IOCTL.

One of them is bindfs. When launched with the -o enable-ioctl option, it enables ioctl forwarding.

However, it only works for the ioctls that do not require copying extra data between kernel and user address space, since FUSE servers are not allowed to perform "unrestricted ioctl".

That's certainly an interesting use case.

Another is ntfs-3g. The only supported ioctl is FITRIM.

Although there is currently no FITRIM equivalent on FreeBSD, it's not difficult to port: Just adding a new entry in sys/filio.h, and porting fstrim(8) from util-linux would suffice.

Note: For the TRIM feature, ntfs-3g server requires BLKDISCARD internally. Since we already have DIOCGDELETE as its equivalent, this won't be a problem.

That wouldn't work very well, because few to no FreeBSD applications will use FITRIM. A far better option would be for ntfs-3g to switch from libfuse2 to libfuse3. Then it could use the native FUSE_FALLOCATE request for hole-punching.

asomers

You should send FUSE_HAS_IOCTL_DIR during fuse_internal_send_init.
This looks like a good start, but I haven't reviewed the tests yet.

asomers · 2024-10-30T15:15:40Z

tests/sys/fs/fusefs/ioctl.cc

+/*-
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2024 The FreeBSD Foundation


As emaste said, no copyright assignment is expected. Better to put your own name on this line.

asomers · 2024-10-30T15:29:28Z

sys/fs/fuse/fuse_vnops.c

+			return (ENOTTY);
+		flags |= FUSE_IOCTL_DIR;
+	}
+#ifndef __LP64__


The correct symbol to check is COMPAT_FREEBSD32. And why ifndef instead of ifdef?

asomers · 2024-10-30T15:37:38Z

sys/fs/fuse/fuse_vnops.c

+	fii->in_size = insize;
+	fii->out_size = outsize;
+	if (insize > 0)
+		memcpy((char *)fii + sizeof(*fii), arg, insize);


Is this copy coming from userspace? If so you should use copying instead of memcpy.

sys_ioctl() does all the copyin()/copyout(), and arg is guaranteed to be pointing to a kernel space buffer (except for IOC_VOID).

asomers · 2024-10-30T15:40:03Z

sys/fs/fuse/fuse_vnops.c

+			err = EIO;
+			goto out;
+		}
+		memcpy(arg, (char *)fio + sizeof(*fio), realoutsize);


Similarly, this must be copyout if arg points to userspace.

asomers · 2024-10-30T15:42:08Z

sys/fs/fuse/fuse_vnops.c

+	uint32_t outsize = 0;
+	int err;
+
+	err = fuse_filehandle_get_anyflags(vp, &fufh, cred, td->td_proc->p_pid);


Should this be fuse_filehandle_getrw instead?

I believe this is not necessary, since kern_ioctl() already checks whether the file is opened for reading or writing.

That's true for all good fuse file systems. But one (anti-, IMHO) feature of the fuse protocol is that the file system itself can choose to take responsibility for permission checks on open(). That's why fuse file handles exist. So we need to be careful to get a file handle of the correct type.

I don't quite get it here. Which type of file handle are we expecting?

For example, if a process opens a file twice, one with O_RDONLY, the other with O_WRONLY, the two file handles are equally eligible to be selected. The only thing a FUSE client can do is arbitrarily choose one and pass it to the server, since it has no idea which open flags a specific ioctl may require.

I took a closer look at the VFS. vn_ioctl() does set ap->a_fflag.
My mistake here.

However, it seems that it should be fuse_filehandle_get()
instead of fuse_filehandle_getrw().

Patch updated. Please let me know if there are more problems.

CismonX · 2024-10-30T21:22:33Z

Patch updated with the following changes:

Update the copyright notice in the added test file.
Add a Signed-off-by: line in all commit messages.
Add FUSE_HAS_IOCTL_DIR flag in fuse_internal_send_init().
Take COMPAT_FREEBSD32 into consideration when setting FUSE_IOCTL_32BIT.

Also removed the bogus "XXX" comment lines - The ioctl(2) man-page does not say that ioctl always returns 0 on success. I must have mistaken that for something else...

asomers · 2024-10-31T01:46:57Z

sys/fs/fuse/fuse_internal.c

@@ -1115,7 +1114,7 @@ fuse_internal_send_init(struct fuse_data *data, struct thread *td)
 	 * FUSE_MAX_PAGES: not yet implemented
 	 */
 	fiii->flags = FUSE_ASYNC_READ | FUSE_POSIX_LOCKS | FUSE_EXPORT_SUPPORT
-		| FUSE_BIG_WRITES | FUSE_WRITEBACK_CACHE
+		| FUSE_BIG_WRITES | FUSE_HAS_IOCTL_DIR | FUSE_WRITEBACK_CACHE


I think we also need to check for FUSE_HAS_IOCTL_DIR during fuse_internal_init_callback. If not present, then we should return ENOTTY whenever a user tries to use an ioctl on a directory.

That would make it incompatible with the Linux FUSE implementation, since FUSE_HAS_IOCTL_DIR is only meant to indicate whether the kernel supports this feature. If the server sets this flag, it should be ignored.

Also see this patch: #1509

asomers · 2024-10-31T01:55:46Z

tests/sys/fs/fusefs/ioctl.cc

+/*-
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (c) 2024 CismonX <[email protected]>


Is CismonX your legal name? An alias cannot own a copyright.

Legally they can. Pseudonyms are explicitly allowed. The project strongly prefers a legal entity, though.

Well, "CismonX" is an alias I've been using for ~10 years,
and always associated with this email, thus it is more
identifiable than my real name, since I rarely use that online.

Personally, I'm not against disclosing my legal name, however,
that would be multibyte characters - you certainly do not want
that in the source files and commit messages.
Meanwhile, using something that pronounces like my real name
does not make it more "legal" than an alias.

Signed-off-by: CismonX <[email protected]>

asomers reviewed Oct 14, 2024

View reviewed changes

asomers self-assigned this Oct 14, 2024

CismonX force-pushed the feat/fusefs-ioctl branch from cf075af to 56769ac Compare October 14, 2024 21:14

asomers suggested changes Oct 30, 2024

View reviewed changes

CismonX force-pushed the feat/fusefs-ioctl branch from 56769ac to a570e7f Compare October 30, 2024 21:21

asomers suggested changes Oct 31, 2024

View reviewed changes

CismonX added 3 commits December 23, 2024 03:24

fusefs: support FUSE_IOCTL

a4f9cc1

Signed-off-by: CismonX <[email protected]>

tests/fusefs: mockfs: support FUSE_IOCTL

4f13d44

Signed-off-by: CismonX <[email protected]>

tests/fusefs: add test cases for FUSE_IOCTL

761741e

Signed-off-by: CismonX <[email protected]>

CismonX force-pushed the feat/fusefs-ioctl branch from a570e7f to 761741e Compare December 22, 2024 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fusefs: support FUSE_IOCTL #1470

fusefs: support FUSE_IOCTL #1470

CismonX commented Oct 14, 2024

asomers left a comment

asomers Oct 14, 2024

CismonX Oct 14, 2024

emaste Oct 15, 2024

emaste Oct 30, 2024

CismonX commented Oct 14, 2024

CismonX commented Oct 29, 2024

asomers commented Oct 30, 2024

asomers left a comment

asomers Oct 30, 2024

asomers Oct 30, 2024

asomers Oct 30, 2024

CismonX Oct 30, 2024

asomers Oct 30, 2024

asomers Oct 30, 2024

CismonX Oct 30, 2024

asomers Oct 31, 2024

CismonX Oct 31, 2024

CismonX Dec 22, 2024

CismonX commented Oct 30, 2024

asomers Oct 31, 2024

CismonX Nov 2, 2024

asomers Oct 31, 2024

bsdimp Oct 31, 2024

CismonX Dec 22, 2024

fusefs: support FUSE_IOCTL #1470

Are you sure you want to change the base?

fusefs: support FUSE_IOCTL #1470

Conversation

CismonX commented Oct 14, 2024

asomers left a 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

CismonX commented Oct 14, 2024

CismonX commented Oct 29, 2024

asomers commented Oct 30, 2024

asomers left a 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

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

CismonX commented Oct 30, 2024

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