Page MenuHomeFreeBSD

fuse extattrs: fix issue when neither uio nor size were not passed to VOP_* (logic only).
ClosedPublic

Authored by fsu on Dec 18 2017, 8:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 22 2024, 1:39 PM
Unknown Object (File)
Feb 16 2024, 1:25 PM
Unknown Object (File)
Jan 27 2024, 4:30 AM
Unknown Object (File)
Dec 23 2023, 12:58 AM
Unknown Object (File)
Dec 12 2023, 7:05 PM
Unknown Object (File)
Oct 28 2023, 9:08 PM
Unknown Object (File)
Sep 18 2023, 1:22 AM
Unknown Object (File)
Jul 27 2023, 8:50 PM

Details

Summary
Test Plan

The fuse extattrs tests will be included to test plan for https://reviews.freebsd.org/D13209
when linuxulator xattrs will be finished.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Please add a description here to have an idea what the commit log would look like.

This revision is now accepted and ready to land.Dec 18 2017, 3:00 PM
fs/fuse/fuse_vnops.c
2218 ↗(On Diff #36706)

Perhaps rename to fuse_list_len: they are, after all, not necessarily from linux.

fs/fuse/fuse_vnops.c
2218 ↗(On Diff #36706)

I think it's ok. FUSE ABI uses linux-style attributes, and that's all this refers to.

2270–2271 ↗(On Diff #36706)

I'm not sure I understand what's going on here. Where is the linux_list_len buffer that the response is read into?

2285 ↗(On Diff #36706)

should this fdi.iosize be linux_list_len?

Add linux_list stack variable to make listxattr() function more readable.

This revision now requires review to proceed.Dec 19 2017, 7:32 AM
fs/fuse/fuse_vnops.c
2270–2271 ↗(On Diff #36756)

So get_xattr_in was initialized to fdi.indata for the previous instance of fdi. Is it valid to retain the pointer inside the fdi structure over a fdisk_make_vp() call?

(Cosmetic nit: should it be list_xattr_in rather than get_xattr_in?)

2274–2275 ↗(On Diff #36756)

Is this even possible, given we've already successfully invoked FUSE_LISTXATTR earlier in this function?

More deep reinitialization of struct fuse_dispatcher fdi before second call to fdisp_wait_answ() under fuse_vnop_listextattr().

fs/fuse/fuse_vnops.c
2270–2271 ↗(On Diff #36756)

So get_xattr_in was initialized to fdi.indata for the previous instance of fdi. Is it valid to retain the pointer inside the fdi structure over a fdisk_make_vp() call?

Yep, as I can see, here fdi.indata could be reallocated in the fdisp_make_vp() -> fdisp_make_pid() -> FUSE_DIMALLOC() in case of fdi.iosize.

(Cosmetic nit: should it be list_xattr_in rather than get_xattr_in?)

Not sure, because get_xattr_in has type struct fuse_getxattr_in, so it could be some sort of confusion, comparing with getxattr() path.
But, from other point of view it is possible to add struct fuse_listxattr_in to fuse_kernel.h additionally to struct fuse_getxattr_in and struct fuse_setxattr_in. So, if you prefer second option, I ready to make this update.

2274–2275 ↗(On Diff #36756)

I prefer to leave this check, in case, for example, on force unmount (mean when usb drive disconnected without unmount command call).

fs/fuse/fuse_vnops.c
2270–2271 ↗(On Diff #36756)

Leaving it as get_xattr_in for now is ok with me.

If you want to do that cleanup, though, I'm not opposed.

2274–2275 ↗(On Diff #36756)

Would the underlying layer return ENOSYS in that case, or another error (ENXIO / EIO)?

  • Declare struct fuse_listxattr_* to not use fuse_getxattr_* in case of list().
  • Remove ENOSYS error check in case of second fdisp_wait_answ() call under list().
fs/fuse/fuse_vnops.c
2274–2275 ↗(On Diff #36756)

Aha... the ENOSYS is redundant here.
But, in case of force unmount or another troubles with drive, it is difficult to say which error will be returned. Because, as I understand, it should be returned by underlying fuse filesystem after block read/write calls.

LGTM. I am not sure whether removing answ/iosize checks (both locations) is safe.

sys/fs/fuse/fuse_vnops.c
2287–2288 ↗(On Diff #37667)

Is it safe to remove these checks?

This revision is now accepted and ready to land.Aug 8 2018, 6:46 PM

(I thought this had been committed already :-/ )

sys/fs/fuse/fuse_vnops.c
2287–2288 ↗(On Diff #37667)

Ok, I spent some time with the source code inspecting and the question was, is it possible to have the fdi.answ == NULL in case the fdisp_wait_answ()
returned 0:

  • fdisp_wait_answ():

fdip->answ = fticket_resp(fdip->tick)->base;

  • fticket_resp():

return (&ftick->tk_aw_fiov);

The tk_aw_fiov come from:
struct fuse_ticket {
...
/* fields for handling answers coming from userspace */

struct fuse_iov              tk_aw_fiov;

...
};

The tk_aw_fiov is initialized inside the fticket_init(void *mem, int size, int flags) function
using:
fiov_init(&ftick->tk_aw_fiov, 0);
As could be seen in the code of this function, the malloc called to base pointer and the size come from
uint32_t msize = FU_AT_LEAST(size);
and it will be not equal 0.
So, as I understand, if the struct fuse_ticket in the struct fuse_dispatcher was initialized, it is guaranteed, that fdi.answ != NULL after successfull fdisp_wait_answ() call.

Also, the same check in the fuse_vnop_getextattr() will be removed before landing.

This revision was automatically updated to reflect the committed changes.

Sorry for the drive-by review, I found this review when writing an article about FUSE.

head/sys/fs/fuse/fuse_kernel.h
287

Shouldn't this line be __u32 padding, to match the previous use of fuse_getxattr_in for FUSE_LISTXATTR? Otherwise it's deviating from the ABI of mainline FUSE.

292

I think this should also be padding, to match fuse_getxattr_out.

head/sys/fs/fuse/fuse_kernel.h
287

Perhaps the name should match for stylistic reasons (or perhaps not, I don't know), but the name of a C struct member variable does not affect wire protocol ABI in any way.