Page MenuHomeFreeBSD
Differential D21287 Diff 60868 sys/fs/fuse/fuse_vnops.c

Changeset View

Standalone View

View Options

sys/fs/fuse/fuse_vnops.c

Show First 20 Lines • Show All 2,303 Lines • ▼ Show 20 Lines if (linux_list_len == 0) {
goto out; goto out;
} }
/* /*
* Retrieve Linux / FUSE compatible list values. * Retrieve Linux / FUSE compatible list values.
*/ */
fdisp_refresh_vp(&fdi, FUSE_LISTXATTR, vp, td, cred); fdisp_refresh_vp(&fdi, FUSE_LISTXATTR, vp, td, cred);
list_xattr_in = fdi.indata; list_xattr_in = fdi.indata;
list_xattr_in->size = linux_list_len + sizeof(*list_xattr_out); list_xattr_in->size = linux_list_len;
asomersAuthorUnsubmitted
Done
Inline Actions

This was the error. list_xattr_in->size refers to the size of the attribute list, not the size of the FUSE transfer. Though neither fuse_kernel.h, the linux kernel, nor libfuse document it, the intention is clear from reading libfuse's passthrough example.

asomers: This was the error. list_xattr_in->size refers to the size of the attribute list, not the size…
attr_str = (char *)fdi.indata + sizeof(*list_xattr_in); attr_str = (char *)fdi.indata + sizeof(*list_xattr_in);
snprintf(attr_str, len, "%s%c", prefix, extattr_namespace_separator); snprintf(attr_str, len, "%s%c", prefix, extattr_namespace_separator);
err = fdisp_wait_answ(&fdi); err = fdisp_wait_answ(&fdi);
if (err != 0) if (err != 0)
goto out; goto out;
linux_list = fdi.answ; linux_list = fdi.answ;
linux_list_len = fdi.iosize;
cemUnsubmitted
Not Done
Inline Actions

Do we not still need this?

cem: Do we not still need this?
/* /*
* Retrieve the BSD compatible list values. * Retrieve the BSD compatible list values.
cemUnsubmitted
Not Done
Inline Actions

(1) Hm. This seems like a vector where an adversarial userspace FS could cause a kernel infinite loop, preventing victim process termination. Maybe we should limit the number of retries in some way?

If some threshold is exceeded, we could return ERESTART, which at least gets us out to userspace and allows signal delivery (like KILL/INT).

(2) In the slightly less adversarial case, sometimes a busy file might just have rapidly changing extattrs? (a) One common way we accommodate racy dynamic buffers is to size the request (fdi.indata->size) to double linux_list_len at each iteration.

(b) Another option would be to just move the retry: label down to line ca. 2320 and exponentially scale the provided buffer size (saving a FUSE IPC round-trip per attempt).

Both of (a) and (b) assume FUSE filesystems will not reject buffers that are not perfectly sized (on retry). Will FUSE filesystems reject that? If they are permitted to return short reads (handled below), I would be a little surprised if they rejected too large output buffers. But it is a possibility worth double-checking.

So my tl;dr suggestion is:

  1. Abort after N (5?) retries with some kind of return to userspace
  2. Allocate more than we need (at least on retry)
cem: (1) Hm. This seems like a vector where an adversarial userspace FS could cause a kernel…
asomersAuthorUnsubmitted
Done
Inline Actions

Good idea; I didn't think about the malicious case.

  1. Returning short data is allowed
  2. I don't think that skipping checking the required length on retry is a good idea, because the attribute could easily go from 1 to 1024 bytes, which would require many iterations if we blindly double the buffer size
  3. ERESTART seems like an appropriate error. I'll do that.
asomers: Good idea; I didn't think about the malicious case. 1) Returning short data is allowed 2) I…
cemUnsubmitted
Not Done
Inline Actions

Cool.

(2) is a reasonable argument for that approach.

One refinement of pure exponential growth would be to just start the bidding at something like PAGE_SIZE. (Sure, extattr list could gow from PAGE_SIZE to 1024 * PAGE_SIZE too, but in the non-adversarial case it's probably less likely than 1->1000.) But I'm perfectly happy with either option.

cem: Cool. (2) is a reasonable argument for that approach. One refinement of pure exponential…
* The Linux / FUSE attribute list format isn't the same * The Linux / FUSE attribute list format isn't the same
cemUnsubmitted
Not Done
Inline Actions

suggest adding trailing "\n" to avoid mangling up some other kernel message

cem: suggest adding trailing "\n" to avoid mangling up some other kernel message
* as FreeBSD's format. So we need to transform it into * as FreeBSD's format. So we need to transform it into
* FreeBSD's format before giving it to the user. * FreeBSD's format before giving it to the user.
*/ */
bsd_list = malloc(linux_list_len, M_TEMP, M_WAITOK); bsd_list = malloc(linux_list_len, M_TEMP, M_WAITOK);
err = fuse_xattrlist_convert(prefix, linux_list, linux_list_len, err = fuse_xattrlist_convert(prefix, linux_list, linux_list_len,
bsd_list, &bsd_list_len); bsd_list, &bsd_list_len);
if (err != 0) if (err != 0)
goto out; goto out;
▲ Show 20 Lines • Show All 138 Lines • Show Last 20 Lines