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 Skipped
Unit
Tests Skipped

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

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

fs/fuse/fuse_vnops.c
2218

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

2270–2271

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

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

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

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

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

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

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

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

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 ↗(On Diff #47055)

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 ↗(On Diff #47055)

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

head/sys/fs/fuse/fuse_kernel.h
287 ↗(On Diff #47055)

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.