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.

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

Event Timeline

fsu created this revision.Dec 18 2017, 8:08 AM
pfg added a comment.Dec 18 2017, 2:52 PM

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

pfg accepted this revision.Dec 18 2017, 3:00 PM

LGTM

This revision is now accepted and ready to land.Dec 18 2017, 3:00 PM
pfg added inline comments.Dec 18 2017, 3:05 PM
fs/fuse/fuse_vnops.c
2218

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

cem added inline comments.Dec 19 2017, 3:24 AM
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?

fsu updated this revision to Diff 36756.Dec 19 2017, 7:32 AM

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

This revision now requires review to proceed.Dec 19 2017, 7:32 AM
cem added inline comments.Jan 5 2018, 11:48 PM
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?

fsu updated this revision to Diff 37622.Jan 8 2018, 8:57 AM

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

fsu added inline comments.Jan 8 2018, 9:15 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?

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).

cem added inline comments.Jan 8 2018, 5:31 PM
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)?

fsu updated this revision to Diff 37667.Jan 9 2018, 6:37 AM
  • 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().
fsu added inline comments.Jan 9 2018, 6:50 AM
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.

cem accepted this revision.Aug 8 2018, 6:46 PM

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
pfg accepted this revision.Aug 9 2018, 2:11 AM

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

fsu added inline comments.Aug 9 2018, 8:06 AM
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.

cem added inline comments.Aug 25 2018, 9:37 PM
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.