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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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

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 ↗(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?

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 ↗(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?

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

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

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 ↗(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.

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

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.

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