On 08/06/21(Tue) 23:32, Helg wrote:
> Hello tech@
>
> Due to the challenges of having a large diff reviewed I've had another
> think about how I can break up the FUSE changes so that they are smaller
> and easier to review.
>
> This is the first of these diffs.
>
> The current design uses a fixed size fusebuf that consists of a header
> and a union of structs that are used for different VFS operations. In
> addition, there may be data of variable size associated with an
> operation. e.g. the buffer passed to write(2). see fb_setup(9).
>
> If there is additional data to be exchanged between libfuse and the
> kernel then libfuse uses an ioctl on the device to read or write this
> variable sized data after the fusebuf has been read or written. This is
> not how the fuse protocol works on Linux. Instead, the fusebuf is read
> or written in a single read(2) or write(2). This change is the first
> step in setting the OpenBSD implementation up for improved compatibility
> in the fuse kernel interface.
>
> The fusebuf struct is shared between the kernel and libfuse but its
> layout differs slightly between the two. The kernel has knowledge of the
> size of data that it is sending or receiving (e.g. read, write, readdir,
> link, lookup) and so can malloc the exact amount of memory required.
> libfuse must read the entire fusebuf but doesn't know its size in
> advance so must have a buffer large enough to cater for the worst case
> scenario. Since libfuse now uses a fixed size fusebuf, it no longer
> needs to free the variable memory previously allocated for the data.
>
> stsp@ has been kind enough to provide initial feedback. Is it now ready
> for an official OK?
Please do not use MIN() provided by <sys/param.h> in userland. The
consensus is to define MINIMUM locally. If you need <sys/param.h>
please annotate why. You can grep for examples in src/bin...
With that fixed the diff is ok mpi@.
Note that your change is an ABI break, that said the fuse(4) device isn't
standard and I doubt anything use it outside of the base libfuse. So I
don't think any special care is needed. However I don't know if it is
worth mentioning it somehow that people don't end up with a mismatch of
userland/kernel.
> Index: lib/libfuse/fuse.c
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 fuse.c
> --- lib/libfuse/fuse.c 28 Jun 2019 13:32:42 -0000 1.51
> +++ lib/libfuse/fuse.c 8 Jun 2021 14:15:29 -0000
> @@ -154,9 +154,9 @@ fuse_loop(struct fuse *fuse)
> {
> struct fusebuf fbuf;
> struct fuse_context ctx;
> - struct fb_ioctl_xch ioexch;
> struct kevent event[5];
> struct kevent ev;
> + ssize_t fbuf_size;
> ssize_t n;
> int ret;
>
> @@ -201,29 +201,15 @@ fuse_loop(struct fuse *fuse)
> strsignal(signum));
> }
> } else if (ret > 0) {
> - n = read(fuse->fc->fd, &fbuf, sizeof(fbuf));
> - if (n != sizeof(fbuf)) {
> + n = read(fuse->fc->fd, &fbuf, FUSEBUFSIZE);
> + fbuf_size = sizeof(fbuf.fb_hdr) + sizeof(fbuf.FD) +
> + fbuf.fb_len;
> + if (n != fbuf_size) {
> fprintf(stderr, "%s: bad fusebuf read\n",
> __func__);
> return (-1);
> }
>
> - /* check if there is data something present */
> - if (fbuf.fb_len) {
> - fbuf.fb_dat = malloc(fbuf.fb_len);
> - if (fbuf.fb_dat == NULL)
> - return (-1);
> - ioexch.fbxch_uuid = fbuf.fb_uuid;
> - ioexch.fbxch_len = fbuf.fb_len;
> - ioexch.fbxch_data = fbuf.fb_dat;
> -
> - if (ioctl(fuse->fc->fd, FIOCGETFBDAT,
> - &ioexch) == -1) {
> - free(fbuf.fb_dat);
> - return (-1);
> - }
> - }
> -
> ctx.fuse = fuse;
> ctx.uid = fbuf.fb_uid;
> ctx.gid = fbuf.fb_gid;
> @@ -238,26 +224,13 @@ fuse_loop(struct fuse *fuse)
> return (-1);
> }
>
> - n = write(fuse->fc->fd, &fbuf, sizeof(fbuf));
> - if (fbuf.fb_len) {
> - if (fbuf.fb_dat == NULL) {
> - fprintf(stderr, "%s: fb_dat is Null\n",
> - __func__);
> - return (-1);
> - }
> - ioexch.fbxch_uuid = fbuf.fb_uuid;
> - ioexch.fbxch_len = fbuf.fb_len;
> - ioexch.fbxch_data = fbuf.fb_dat;
> -
> - if (ioctl(fuse->fc->fd, FIOCSETFBDAT, &ioexch)
> == -1) {
> - free(fbuf.fb_dat);
> - return (-1);
> - }
> - free(fbuf.fb_dat);
> - }
> + fbuf_size = sizeof(fbuf.fb_hdr) + sizeof(fbuf.FD) +
> + fbuf.fb_len;
> + n = write(fuse->fc->fd, &fbuf, fbuf_size);
> +
> ictx = NULL;
>
> - if (n != FUSEBUFSIZE) {
> + if (n != fbuf_size) {
> errno = EINVAL;
> return (-1);
> }
> Index: lib/libfuse/fuse_ops.c
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse_ops.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 fuse_ops.c
> --- lib/libfuse/fuse_ops.c 16 Jul 2018 13:10:53 -0000 1.35
> +++ lib/libfuse/fuse_ops.c 8 Jun 2021 14:15:29 -0000
> @@ -15,6 +15,8 @@
> * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> */
>
> +#include <sys/param.h>
> +
> #include <errno.h>
> #include <string.h>
> #include <stdlib.h>
> @@ -317,17 +319,9 @@ ifuse_ops_readdir(struct fuse *f, struct
> offset = fbuf->fb_io_off;
> size = fbuf->fb_io_len;
>
> - fbuf->fb_dat = calloc(1, size);
> -
> - if (fbuf->fb_dat == NULL) {
> - fbuf->fb_err = -errno;
> - return (0);
> - }
> -
> vn = tree_get(&f->vnode_tree, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> @@ -344,7 +338,6 @@ ifuse_ops_readdir(struct fuse *f, struct
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> @@ -362,9 +355,6 @@ ifuse_ops_readdir(struct fuse *f, struct
> else if (fd.full && fbuf->fb_len == 0)
> fbuf->fb_err = -ENOBUFS;
>
> - if (fbuf->fb_len == 0)
> - free(fbuf->fb_dat);
> -
> return (0);
> }
>
> @@ -526,7 +516,6 @@ ifuse_ops_lookup(struct fuse *f, struct
> fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
> set_vn(f, vn); /*XXX*/
> @@ -537,13 +526,11 @@ ifuse_ops_lookup(struct fuse *f, struct
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> fbuf->fb_err = update_attr(f, &fbuf->fb_attr, realname, vn);
> fbuf->fb_ino = vn->ino;
> - free(fbuf->fb_dat);
> free(realname);
>
> return (0);
> @@ -566,23 +553,15 @@ ifuse_ops_read(struct fuse *f, struct fu
> size = fbuf->fb_io_len;
> offset = fbuf->fb_io_off;
>
> - fbuf->fb_dat = malloc(size);
> - if (fbuf->fb_dat == NULL) {
> - fbuf->fb_err = -errno;
> - return (0);
> - }
> -
> vn = tree_get(&f->vnode_tree, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> @@ -593,9 +572,6 @@ ifuse_ops_read(struct fuse *f, struct fu
> else
> fbuf->fb_err = ret;
>
> - if (fbuf->fb_len == 0)
> - free(fbuf->fb_dat);
> -
> return (0);
> }
>
> @@ -621,20 +597,17 @@ ifuse_ops_write(struct fuse *f, struct f
> vn = tree_get(&f->vnode_tree, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> ret = f->op.write(realname, (char *)fbuf->fb_dat, size, offset, &ffi);
> free(realname);
> - free(fbuf->fb_dat);
>
> if (ret >= 0)
> fbuf->fb_io_len = ret;
> @@ -657,11 +630,9 @@ ifuse_ops_mkdir(struct fuse *f, struct f
> vn = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> - free(fbuf->fb_dat);
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> @@ -690,11 +661,9 @@ ifuse_ops_rmdir(struct fuse *f, struct f
> vn = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> - free(fbuf->fb_dat);
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> @@ -738,13 +707,8 @@ ifuse_ops_readlink(struct fuse *f, struc
>
> fbuf->fb_err = ret;
> if (!ret) {
> - len = strnlen(name, PATH_MAX);
> + len = strnlen(name, MIN(PATH_MAX, FUSEBUFMAXSIZE));
> fbuf->fb_len = len;
> - fbuf->fb_dat = malloc(fbuf->fb_len);
> - if (fbuf->fb_dat == NULL) {
> - fbuf->fb_err = -errno;
> - return (0);
> - }
> memcpy(fbuf->fb_dat, name, len);
> } else
> fbuf->fb_len = 0;
> @@ -762,12 +726,10 @@ ifuse_ops_unlink(struct fuse *f, struct
>
> vn = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
> if (vn == NULL) {
> - free(fbuf->fb_dat);
> fbuf->fb_err = -errno;
> return (0);
> }
>
> - free(fbuf->fb_dat);
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> @@ -821,11 +783,9 @@ ifuse_ops_link(struct fuse *f, struct fu
> vn = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> - free(fbuf->fb_dat);
> realname = build_realname(f, oldnodeid);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> @@ -863,14 +823,12 @@ ifuse_ops_setattr(struct fuse *f, struct
> vn = tree_get(&f->vnode_tree, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
> io = fbtod(fbuf, struct fb_io *);
> @@ -923,7 +881,6 @@ ifuse_ops_setattr(struct fuse *f, struct
> if (!fbuf->fb_err)
> fbuf->fb_err = update_attr(f, &fbuf->fb_attr, realname, vn);
> free(realname);
> - free(fbuf->fb_dat);
>
> return (0);
> }
> @@ -940,7 +897,6 @@ ifuse_ops_symlink(unused struct fuse *f,
> vn = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> @@ -949,7 +905,6 @@ ifuse_ops_symlink(unused struct fuse *f,
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> @@ -957,7 +912,6 @@ ifuse_ops_symlink(unused struct fuse *f,
> fbuf->fb_err = f->op.symlink((const char *)&fbuf->fb_dat[len + 1],
> realname);
> fbuf->fb_ino = vn->ino;
> - free(fbuf->fb_dat);
> free(realname);
>
> return (0);
> @@ -978,7 +932,6 @@ ifuse_ops_rename(struct fuse *f, struct
> vnf = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
> if (vnf == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> @@ -986,12 +939,9 @@ ifuse_ops_rename(struct fuse *f, struct
> fbuf->fb_io_ino);
> if (vnt == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> - free(fbuf->fb_dat);
> -
> realnamef = build_realname(f, vnf->ino);
> if (realnamef == NULL) {
> fbuf->fb_err = -errno;
> @@ -1060,11 +1010,9 @@ ifuse_ops_mknod(struct fuse *f, struct f
> vn = get_vn_by_name_and_parent(f, fbuf->fb_dat, fbuf->fb_ino);
> if (vn == NULL) {
> fbuf->fb_err = -errno;
> - free(fbuf->fb_dat);
> return (0);
> }
>
> - free(fbuf->fb_dat);
> realname = build_realname(f, vn->ino);
> if (realname == NULL) {
> fbuf->fb_err = -errno;
> Index: sys/miscfs/fuse/fuse_device.c
> ===================================================================
> RCS file: /cvs/src/sys/miscfs/fuse/fuse_device.c,v
> retrieving revision 1.36
> diff -u -p -r1.36 fuse_device.c
> --- sys/miscfs/fuse/fuse_device.c 11 Mar 2021 13:31:35 -0000 1.36
> +++ sys/miscfs/fuse/fuse_device.c 8 Jun 2021 14:15:33 -0000
> @@ -383,7 +383,6 @@ fuseread(dev_t dev, struct uio *uio, int
> struct fuse_d *fd;
> struct fusebuf *fbuf;
> struct fb_hdr hdr;
> - void *tmpaddr;
> int error = 0;
>
> fd = fuse_lookup(minor(dev));
> @@ -399,32 +398,39 @@ fuseread(dev_t dev, struct uio *uio, int
> fbuf = SIMPLEQ_FIRST(&fd->fd_fbufs_in);
>
> /* We get the whole fusebuf or nothing */
> - if (uio->uio_resid != FUSEBUFSIZE)
> + if (uio->uio_resid < FUSEBUFSIZE + fbuf->fb_len)
> return (EINVAL);
>
> /* Do not send kernel pointers */
> memcpy(&hdr.fh_next, &fbuf->fb_next, sizeof(fbuf->fb_next));
> memset(&fbuf->fb_next, 0, sizeof(fbuf->fb_next));
> - tmpaddr = fbuf->fb_dat;
> - fbuf->fb_dat = NULL;
> - error = uiomove(fbuf, FUSEBUFSIZE, uio);
> +
> + error = uiomove(&fbuf->fb_hdr, sizeof(fbuf->fb_hdr), uio);
> + if (error)
> + goto end;
> + error = uiomove(&fbuf->FD, sizeof(fbuf->FD), uio);
> if (error)
> goto end;
> + if (fbuf->fb_len > 0) {
> + error = uiomove(fbuf->fb_dat, fbuf->fb_len, uio);
> + if (error)
> + goto end;
> + }
>
> #ifdef FUSE_DEBUG
> fuse_dump_buff((char *)fbuf, FUSEBUFSIZE);
> #endif
> /* Restore kernel pointers */
> memcpy(&fbuf->fb_next, &hdr.fh_next, sizeof(fbuf->fb_next));
> - fbuf->fb_dat = tmpaddr;
>
> - /* Remove the fbuf if it does not contains data */
> - if (fbuf->fb_len == 0) {
> - SIMPLEQ_REMOVE_HEAD(&fd->fd_fbufs_in, fb_next);
> - stat_fbufs_in--;
> - SIMPLEQ_INSERT_TAIL(&fd->fd_fbufs_wait, fbuf, fb_next);
> - stat_fbufs_wait++;
> - }
> + free(fbuf->fb_dat, M_FUSEFS, fbuf->fb_len);
> + fbuf->fb_dat = NULL;
> +
> + /* Move the fbuf to the wait queue */
> + SIMPLEQ_REMOVE_HEAD(&fd->fd_fbufs_in, fb_next);
> + stat_fbufs_in--;
> + SIMPLEQ_INSERT_TAIL(&fd->fd_fbufs_wait, fbuf, fb_next);
> + stat_fbufs_wait++;
>
> end:
> return (error);
> @@ -443,13 +449,17 @@ fusewrite(dev_t dev, struct uio *uio, in
> if (fd == NULL)
> return (ENXIO);
>
> - /* We get the whole fusebuf or nothing */
> - if (uio->uio_resid != FUSEBUFSIZE)
> - return (EINVAL);
> -
> if ((error = uiomove(&hdr, sizeof(hdr), uio)) != 0)
> return (error);
>
> + /* Check for sanity */
> + if (hdr.fh_len > FUSEBUFMAXSIZE)
> + return (EINVAL);
> +
> + /* We get the whole fusebuf or nothing */
> + if (uio->uio_resid != sizeof(fbuf->FD) + hdr.fh_len)
> + return (EINVAL);
> +
> /* looking for uuid in fd_fbufs_wait */
> SIMPLEQ_FOREACH(fbuf, &fd->fd_fbufs_wait, fb_next) {
> if (fbuf->fb_uuid == hdr.fh_uuid)
> @@ -474,10 +484,19 @@ fusewrite(dev_t dev, struct uio *uio, in
> }
>
> /* Get the missing data from the fbuf */
> - error = uiomove(&fbuf->FD, uio->uio_resid, uio);
> + error = uiomove(&fbuf->FD, sizeof(fbuf->FD), uio);
> if (error)
> return error;
> - fbuf->fb_dat = NULL;
> +
> + fbuf->fb_dat = malloc(fbuf->fb_len, M_FUSEFS,
> + M_WAITOK | M_ZERO);
> + error = uiomove(fbuf->fb_dat, fbuf->fb_len, uio);
> + if (error) {
> + free(fbuf->fb_dat, M_FUSEFS, fbuf->fb_len);
> + fbuf->fb_dat = NULL;
> + return (error);
> + }
> +
> #ifdef FUSE_DEBUG
> fuse_dump_buff((char *)fbuf, FUSEBUFSIZE);
> #endif
> @@ -491,19 +510,17 @@ fusewrite(dev_t dev, struct uio *uio, in
> break ;
> }
> end:
> - /* Remove the fbuf if it does not contains data */
> - if (fbuf->fb_len == 0) {
> - if (fbuf == SIMPLEQ_FIRST(&fd->fd_fbufs_wait))
> - SIMPLEQ_REMOVE_HEAD(&fd->fd_fbufs_wait, fb_next);
> - else
> - SIMPLEQ_REMOVE_AFTER(&fd->fd_fbufs_wait, lastfbuf,
> - fb_next);
> - stat_fbufs_wait--;
> - if (fbuf->fb_type == FBT_INIT)
> - fb_delete(fbuf);
> - else
> - wakeup(fbuf);
> - }
> + /* Remove the fbuf from the wait queue */
> + if (fbuf == SIMPLEQ_FIRST(&fd->fd_fbufs_wait))
> + SIMPLEQ_REMOVE_HEAD(&fd->fd_fbufs_wait, fb_next);
> + else
> + SIMPLEQ_REMOVE_AFTER(&fd->fd_fbufs_wait, lastfbuf,
> + fb_next);
> + stat_fbufs_wait--;
> + if (fbuf->fb_type == FBT_INIT)
> + fb_delete(fbuf);
> + else
> + wakeup(fbuf);
>
> return (error);
> }
> Index: sys/sys/fusebuf.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/fusebuf.h,v
> retrieving revision 1.13
> diff -u -p -r1.13 fusebuf.h
> --- sys/sys/fusebuf.h 19 Jun 2018 11:27:54 -0000 1.13
> +++ sys/sys/fusebuf.h 8 Jun 2021 14:15:33 -0000
> @@ -69,7 +69,11 @@ struct fusebuf {
> struct stat FD_attr; /* for attr vnops */
> struct fb_io FD_io; /* for file io vnops */
> } FD;
> +#ifdef _KERNEL
> uint8_t *fb_dat; /* data's */
> +#else
> + uint8_t fb_dat[FUSEBUFMAXSIZE]; /* userland does i/o with fixed
> size buffer */
> +#endif
> };
>
> #define fb_next fb_hdr.fh_next
>
