| Message ID | 20241002012230.4174585-5-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/9] xattr: switch to CLASS(fd) | expand |
On 10/1/24 7:22 PM, Al Viro wrote: > diff --git a/io_uring/xattr.c b/io_uring/xattr.c > index 71d9e2569a2f..7f6bbfd846b9 100644 > --- a/io_uring/xattr.c > +++ b/io_uring/xattr.c > int io_fsetxattr(struct io_kiocb *req, unsigned int issue_flags) > { > + struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); > int ret; > > WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); > > - ret = __io_setxattr(req, issue_flags, &req->file->f_path); > + ret = file_setxattr(req->file, &ix->ctx); > io_xattr_finish(req, ret); > return IOU_OK; This and ... -> > -retry: > - ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL); > - if (!ret) { > - ret = __io_setxattr(req, issue_flags, &path); > - path_put(&path); > - if (retry_estale(ret, lookup_flags)) { > - lookup_flags |= LOOKUP_REVAL; > - goto retry; > - } > - } > - > + ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); > io_xattr_finish(req, ret); > return IOU_OK; this looks like it needs an ix->filename = NULL, as filename_{s,g}xattr() drops the reference. The previous internal helper did not, and hence the cleanup always did it. But should work fine if ->filename is just zeroed. Otherwise looks good. I've skimmed the other patches and didn't see anything odd, I'll take a closer look tomorrow.
On Tue, Oct 01, 2024 at 07:34:12PM -0600, Jens Axboe wrote: > > -retry: > > - ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL); > > - if (!ret) { > > - ret = __io_setxattr(req, issue_flags, &path); > > - path_put(&path); > > - if (retry_estale(ret, lookup_flags)) { > > - lookup_flags |= LOOKUP_REVAL; > > - goto retry; > > - } > > - } > > - > > + ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); > > io_xattr_finish(req, ret); > > return IOU_OK; > > this looks like it needs an ix->filename = NULL, as > filename_{s,g}xattr() drops the reference. The previous internal helper > did not, and hence the cleanup always did it. But should work fine if > ->filename is just zeroed. > > Otherwise looks good. I've skimmed the other patches and didn't see > anything odd, I'll take a closer look tomorrow. Hmm... I wonder if we would be better off with file{,name}_setxattr() doing kvfree(cxt->kvalue) - it makes things easier both on the syscall and on io_uring side. I've added minimal fixes (zeroing ix->filename after filename_[sg]etxattr()) to 5/9 and 6/9 *and* added a followup calling conventions change at the end of the branch. See #work.xattr2 in the same tree; FWIW, the followup cleanup is below; note that putname(ERR_PTR(-Ewhatever)) is an explicit no-op, so there's no need to zero on getname() failures. commit 67896be9ac99b3fdef82708dd06e720332c13cdc Author: Al Viro <viro@zeniv.linux.org.uk> Date: Tue Oct 1 22:03:16 2024 -0400 saner calling conventions for file{,name}_setxattr() Have them consume ctx->kvalue. That simplifies both the path_setxattrat() and io_uring side of things - there io_xattr_finish() is just left to free the xattr name and that's it. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> diff --git a/fs/xattr.c b/fs/xattr.c index 59cdb524412e..6bb656941bce 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -672,6 +672,7 @@ int file_setxattr(struct file *f, struct kernel_xattr_ctx *ctx) error = do_setxattr(file_mnt_idmap(f), f->f_path.dentry, ctx); mnt_drop_write_file(f); } + kvfree(ctx->kvalue); return error; } @@ -697,6 +698,7 @@ int filename_setxattr(int dfd, struct filename *filename, } out: + kvfree(ctx->kvalue); putname(filename); return error; } @@ -731,14 +733,10 @@ static int path_setxattrat(int dfd, const char __user *pathname, if (!filename) { CLASS(fd, f)(dfd); if (fd_empty(f)) - error = -EBADF; - else - error = file_setxattr(fd_file(f), &ctx); - } else { - error = filename_setxattr(dfd, filename, lookup_flags, &ctx); + return -EBADF; + return file_setxattr(fd_file(f), &ctx); } - kvfree(ctx.kvalue); - return error; + return filename_setxattr(dfd, filename, lookup_flags, &ctx); } SYSCALL_DEFINE6(setxattrat, int, dfd, const char __user *, pathname, unsigned int, at_flags, diff --git a/io_uring/xattr.c b/io_uring/xattr.c index 90277246dbea..9f3ea12f628d 100644 --- a/io_uring/xattr.c +++ b/io_uring/xattr.c @@ -35,9 +35,10 @@ void io_xattr_cleanup(struct io_kiocb *req) static void io_xattr_finish(struct io_kiocb *req, int ret) { - req->flags &= ~REQ_F_NEED_CLEANUP; + struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); - io_xattr_cleanup(req); + kfree(ix->ctx.kname); + req->flags &= ~REQ_F_NEED_CLEANUP; io_req_set_res(req, ret, 0); } @@ -94,12 +95,9 @@ int io_getxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) path = u64_to_user_ptr(READ_ONCE(sqe->addr3)); ix->filename = getname(path); - if (IS_ERR(ix->filename)) { - ret = PTR_ERR(ix->filename); - ix->filename = NULL; - } - - return ret; + if (IS_ERR(ix->filename)) + return PTR_ERR(ix->filename); + return 0; } int io_fgetxattr(struct io_kiocb *req, unsigned int issue_flags) @@ -122,7 +120,6 @@ int io_getxattr(struct io_kiocb *req, unsigned int issue_flags) WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); ret = filename_getxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); - ix->filename = NULL; io_xattr_finish(req, ret); return IOU_OK; } @@ -172,12 +169,9 @@ int io_setxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) path = u64_to_user_ptr(READ_ONCE(sqe->addr3)); ix->filename = getname(path); - if (IS_ERR(ix->filename)) { - ret = PTR_ERR(ix->filename); - ix->filename = NULL; - } - - return ret; + if (IS_ERR(ix->filename)) + return PTR_ERR(ix->filename); + return 0; } int io_fsetxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) @@ -205,7 +199,6 @@ int io_setxattr(struct io_kiocb *req, unsigned int issue_flags) WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); - ix->filename = NULL; io_xattr_finish(req, ret); return IOU_OK; }
On 10/1/24 8:08 PM, Al Viro wrote: > On Tue, Oct 01, 2024 at 07:34:12PM -0600, Jens Axboe wrote: > >>> -retry: >>> - ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL); >>> - if (!ret) { >>> - ret = __io_setxattr(req, issue_flags, &path); >>> - path_put(&path); >>> - if (retry_estale(ret, lookup_flags)) { >>> - lookup_flags |= LOOKUP_REVAL; >>> - goto retry; >>> - } >>> - } >>> - >>> + ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); >>> io_xattr_finish(req, ret); >>> return IOU_OK; >> >> this looks like it needs an ix->filename = NULL, as >> filename_{s,g}xattr() drops the reference. The previous internal helper >> did not, and hence the cleanup always did it. But should work fine if >> ->filename is just zeroed. >> >> Otherwise looks good. I've skimmed the other patches and didn't see >> anything odd, I'll take a closer look tomorrow. > > Hmm... I wonder if we would be better off with file{,name}_setxattr() > doing kvfree(cxt->kvalue) - it makes things easier both on the syscall > and on io_uring side. > > I've added minimal fixes (zeroing ix->filename after filename_[sg]etxattr()) > to 5/9 and 6/9 *and* added a followup calling conventions change at the end > of the branch. See #work.xattr2 in the same tree; FWIW, the followup > cleanup is below; note that putname(ERR_PTR(-Ewhatever)) is an explicit > no-op, so there's no need to zero on getname() failures. Looks good to me, thanks Al!
On Wed, Oct 02, 2024 at 12:00:45PM -0600, Jens Axboe wrote: > On 10/1/24 8:08 PM, Al Viro wrote: > > On Tue, Oct 01, 2024 at 07:34:12PM -0600, Jens Axboe wrote: > > > >>> -retry: > >>> - ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL); > >>> - if (!ret) { > >>> - ret = __io_setxattr(req, issue_flags, &path); > >>> - path_put(&path); > >>> - if (retry_estale(ret, lookup_flags)) { > >>> - lookup_flags |= LOOKUP_REVAL; > >>> - goto retry; > >>> - } > >>> - } > >>> - > >>> + ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); > >>> io_xattr_finish(req, ret); > >>> return IOU_OK; > >> > >> this looks like it needs an ix->filename = NULL, as > >> filename_{s,g}xattr() drops the reference. The previous internal helper > >> did not, and hence the cleanup always did it. But should work fine if > >> ->filename is just zeroed. > >> > >> Otherwise looks good. I've skimmed the other patches and didn't see > >> anything odd, I'll take a closer look tomorrow. > > > > Hmm... I wonder if we would be better off with file{,name}_setxattr() > > doing kvfree(cxt->kvalue) - it makes things easier both on the syscall > > and on io_uring side. > > > > I've added minimal fixes (zeroing ix->filename after filename_[sg]etxattr()) > > to 5/9 and 6/9 *and* added a followup calling conventions change at the end > > of the branch. See #work.xattr2 in the same tree; FWIW, the followup > > cleanup is below; note that putname(ERR_PTR(-Ewhatever)) is an explicit > > no-op, so there's no need to zero on getname() failures. > > Looks good to me, thanks Al! I'm still not sure if the calling conventions change is right - in the current form the last commit in there leaks ctx.kvalue in -EBADF case. It's easy to fix up, but... as far as I'm concerned, a large part of the point of the exercise is to come up with the right model for the calling conventions for that family of APIs. I really want to get rid of that ad-hoc crap. If we are to have what amounts to the alternative syscall interface, we'd better get it right. I'm perfectly fine with having a set of "this is what the syscall is doing past marshalling arguments" primitives, but let's make sure they are properly documented and do not have landmines for callers to step into... Questions on the io_uring side: * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time. Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case? Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway... Am I missing something subtle here? * what's to guarantee that pointers fetched by io_file_get_fixed() called from io_assing_file() will stay valid? You do not bump the struct file refcount in this case, after all; what's to prevent unregistration from the main thread while the worker is getting through your request? Is that what the break on node->refs in the loop in io_rsrc_node_ref_zero() is about? Or am I barking at the wrong tree here? I realize that I'm about the last person to complain about the lack of documentation, but... FWIW, my impression is that you have a list of nodes corresponding to overall resource states (which includes the file reference table) and have each borrow bump the use count on the node corresponding to the current state (at the tail of the list?) Each removal adds new node to the tail of the list, sticks the file reference there and tries to trigger io_rsrc_node_ref_zero() (which, for some reason, takes node instead of the node->ctx, even though it doesn't give a rat's arse about anything else in its argument). If there are nodes at the head of the list with zero use count, that takes them out, stopping at the first in-use node. File reference stashed in a node is dropped when it's taken out. If the above is more or less correct (and I'm pretty sure that it misses quite a few critical points), the rules would be equivalent to + there is a use count associated with the table state. + before we borrow a file reference from the table, we must bump that use count (see the call of __io_req_set_rsrc_node() in io_file_get_fixed()) and arrange for dropping it once we are done with the reference (io_put_rsrc_node() when freeing request, in io_free_batch_list()) + any removals from the table will switch to new state; dropping the removed reference is guaranteed to be delayed until use counts on all earlier states drop to zero. How far are those rules from being accurate and how incomplete they are? I hadn't looked into the quiescence-related stuff, which might or might not be relevant...
On 10/2/24 3:19 PM, Al Viro wrote: > On Wed, Oct 02, 2024 at 12:00:45PM -0600, Jens Axboe wrote: >> On 10/1/24 8:08 PM, Al Viro wrote: >>> On Tue, Oct 01, 2024 at 07:34:12PM -0600, Jens Axboe wrote: >>> >>>>> -retry: >>>>> - ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL); >>>>> - if (!ret) { >>>>> - ret = __io_setxattr(req, issue_flags, &path); >>>>> - path_put(&path); >>>>> - if (retry_estale(ret, lookup_flags)) { >>>>> - lookup_flags |= LOOKUP_REVAL; >>>>> - goto retry; >>>>> - } >>>>> - } >>>>> - >>>>> + ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); >>>>> io_xattr_finish(req, ret); >>>>> return IOU_OK; >>>> >>>> this looks like it needs an ix->filename = NULL, as >>>> filename_{s,g}xattr() drops the reference. The previous internal helper >>>> did not, and hence the cleanup always did it. But should work fine if >>>> ->filename is just zeroed. >>>> >>>> Otherwise looks good. I've skimmed the other patches and didn't see >>>> anything odd, I'll take a closer look tomorrow. >>> >>> Hmm... I wonder if we would be better off with file{,name}_setxattr() >>> doing kvfree(cxt->kvalue) - it makes things easier both on the syscall >>> and on io_uring side. >>> >>> I've added minimal fixes (zeroing ix->filename after filename_[sg]etxattr()) >>> to 5/9 and 6/9 *and* added a followup calling conventions change at the end >>> of the branch. See #work.xattr2 in the same tree; FWIW, the followup >>> cleanup is below; note that putname(ERR_PTR(-Ewhatever)) is an explicit >>> no-op, so there's no need to zero on getname() failures. >> >> Looks good to me, thanks Al! > > I'm still not sure if the calling conventions change is right - in the > current form the last commit in there leaks ctx.kvalue in -EBADF case. > It's easy to fix up, but... as far as I'm concerned, a large part of > the point of the exercise is to come up with the right model for the > calling conventions for that family of APIs. The reason I liked the putname() is that it's unconditional - the caller can rely on it being put, regardless of the return value. So I'd say the same should be true for ctx.kvalue, and if not, the caller should still free it. That's the path of least surprise - no leak for the least tested error path, and no UAF in the success case. For the put case, most other abstractions end up being something ala: helper(struct file *file, ...) { actual actions } regular_sys_call(int fd, ...) { struct fd f; int ret = -EBADF; f = fdget(fd); if (f.file) { ret = helper(f.file, ...); fdput(f(); } return ret; } where io_uring will use helper(), and where the file reference is assumed to be valid for helper() and helper() will not put a reference to it. That's a bit different than your putname() case, but I think as long as it's consistent regardless of return value, then either approach is fine. Maybe just add a comment about that? At least for the consistent case, if it blows up, it'll blow up instantly rather than be a surprise down the line for "case x,y,z doesn't put it" or "case x,y,z always puts in, normal one does not". > I really want to get rid of that ad-hoc crap. If we are to have what > amounts to the alternative syscall interface, we'd better get it > right. I'm perfectly fine with having a set of "this is what the > syscall is doing past marshalling arguments" primitives, but let's > make sure they are properly documented and do not have landmines for > callers to step into... Fully agree. > Questions on the io_uring side: > * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time. > Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case? > Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway... > Am I missing something subtle here? Right, it could be allowed for fgetxattr on the io_uring side. Anything that passes in a struct file would be fair game to enable it on. Anything that passes in a path (eg a non-fd value), it obviously wouldn't make sense anyway. > * what's to guarantee that pointers fetched by io_file_get_fixed() > called from io_assing_file() will stay valid? You do not bump the struct > file refcount in this case, after all; what's to prevent unregistration > from the main thread while the worker is getting through your request? > Is that what the break on node->refs in the loop in io_rsrc_node_ref_zero() > is about? Or am I barking at the wrong tree here? I realize that I'm about > the last person to complain about the lack of documentation, but... > > FWIW, my impression is that you have a list of nodes corresponding > to overall resource states (which includes the file reference table) and > have each borrow bump the use count on the node corresponding to the current > state (at the tail of the list?) > Each removal adds new node to the tail of the list, sticks the > file reference there and tries to trigger io_rsrc_node_ref_zero() (which, > for some reason, takes node instead of the node->ctx, even though it > doesn't give a rat's arse about anything else in its argument). > If there are nodes at the head of the list with zero use count, > that takes them out, stopping at the first in-use node. File reference > stashed in a node is dropped when it's taken out. > > If the above is more or less correct (and I'm pretty sure that it > misses quite a few critical points), the rules would be equivalent to > + there is a use count associated with the table state. > + before we borrow a file reference from the table, we must bump > that use count (see the call of __io_req_set_rsrc_node() in > io_file_get_fixed()) and arrange for dropping it once we are done with > the reference (io_put_rsrc_node() when freeing request, in io_free_batch_list()) > + any removals from the table will switch to new state; dropping > the removed reference is guaranteed to be delayed until use counts on > all earlier states drop to zero. > > How far are those rules from being accurate and how incomplete > they are? I hadn't looked into the quiescence-related stuff, which might > or might not be relevant... That is pretty darn accurate. The ordering of the rsrc nodes and the break ensure that it stays valid until anything using it has completed. And yes it would be nice to document that code a bit, but honestly I'd much rather just make it more obviously referenced if that can be done cheaply enough. For now, I'll add some comments, and hope you do the same on your side! Because I don't ever remember seeing an Al comment. Great emails, for sure, but not really comments.
On Wed, Oct 02, 2024 at 04:55:22PM -0600, Jens Axboe wrote: > The reason I liked the putname() is that it's unconditional - the caller > can rely on it being put, regardless of the return value. So I'd say the > same should be true for ctx.kvalue, and if not, the caller should still > free it. That's the path of least surprise - no leak for the least > tested error path, and no UAF in the success case. The problem with ctx.kvalue is that on the syscall side there's a case when we do not call either file_setxattr() or filename_setxattr() - -EBADF. And it's a lot more convenient to do setxattr_copy() first, so we end up with a lovely landmine: filename = getname_xattr(pathname, at_flags); if (!filename) { CLASS(fd, f)(dfd); if (fd_empty(f)) { kfree(ctx.kvalue); // lest we leak return -EBADF; } return file_setxattr(fd_file(f), &ctx); } return filename_setxattr(dfd, filename, lookup_flags, &ctx); That's asking for trouble, obviously. So I think we ought to consume filename (in filename_...()) case, leave struct file reference alone (we have to - it might have been borrowed rather than cloned) and leave ->kvalue unchanged. Yes, it ends up being more clumsy, but at least it's consistent between the cases... As for consuming filename... On the syscall side it allows things like SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode) { return do_mkdirat(dfd, getname(pathname), mode); } which is better than the alternatives - I mean, that's SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode) { struct filename *filename = getname(pathname); int res = do_mkdirat(dfd, filename, mode); putname(filename); return ret; } or SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode) { struct filename *filename __free(putname) = getname(pathname); return do_mkdirat(dfd, filename, mode); } and both stink, if for different reasons ;-/ Having those things consume (unconditionally) is better, IMO. Hell knows; let's go with what I described above for now and see where it leads when more such helpers are regularized. > That's a bit different than your putname() case, but I think as long as > it's consistent regardless of return value, then either approach is > fine. Maybe just add a comment about that? At least for the consistent > case, if it blows up, it'll blow up instantly rather than be a surprise > down the line for "case x,y,z doesn't put it" or "case x,y,z always puts > in, normal one does not". Obviously. > > Questions on the io_uring side: > > * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time. > > Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case? > > Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway... > > Am I missing something subtle here? > > Right, it could be allowed for fgetxattr on the io_uring side. Anything > that passes in a struct file would be fair game to enable it on. > Anything that passes in a path (eg a non-fd value), it obviously > wouldn't make sense anyway. OK, done and force-pushed into #work.xattr.
On 10/5/24 11:28 PM, Al Viro wrote: > On Wed, Oct 02, 2024 at 04:55:22PM -0600, Jens Axboe wrote: > >> The reason I liked the putname() is that it's unconditional - the caller >> can rely on it being put, regardless of the return value. So I'd say the >> same should be true for ctx.kvalue, and if not, the caller should still >> free it. That's the path of least surprise - no leak for the least >> tested error path, and no UAF in the success case. > > The problem with ctx.kvalue is that on the syscall side there's a case when > we do not call either file_setxattr() or filename_setxattr() - -EBADF. > And it's a lot more convenient to do setxattr_copy() first, so we end > up with a lovely landmine: > filename = getname_xattr(pathname, at_flags); > if (!filename) { > CLASS(fd, f)(dfd); > if (fd_empty(f)) { > kfree(ctx.kvalue); // lest we leak > return -EBADF; > } > return file_setxattr(fd_file(f), &ctx); > } > return filename_setxattr(dfd, filename, lookup_flags, &ctx); > > That's asking for trouble, obviously. So I think we ought to consume > filename (in filename_...()) case, leave struct file reference alone > (we have to - it might have been borrowed rather than cloned) and leave > ->kvalue unchanged. Yes, it ends up being more clumsy, but at least > it's consistent between the cases... > > As for consuming filename... On the syscall side it allows things like > SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode) > { > return do_mkdirat(dfd, getname(pathname), mode); > } > which is better than the alternatives - I mean, that's > SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode) > { > struct filename *filename = getname(pathname); > int res = do_mkdirat(dfd, filename, mode); > putname(filename); > return ret; > } > or > SYSCALL_DEFINE3(mkdirat, int, dfd, const char __user *, pathname, umode_t, mode) > { > struct filename *filename __free(putname) = getname(pathname); > return do_mkdirat(dfd, filename, mode); > } > and both stink, if for different reasons ;-/ Having those things consume > (unconditionally) is better, IMO. > > Hell knows; let's go with what I described above for now and see where > it leads when more such helpers are regularized. Sounds like a plan. >>> Questions on the io_uring side: >>> * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time. >>> Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case? >>> Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway... >>> Am I missing something subtle here? >> >> Right, it could be allowed for fgetxattr on the io_uring side. Anything >> that passes in a struct file would be fair game to enable it on. >> Anything that passes in a path (eg a non-fd value), it obviously >> wouldn't make sense anyway. > > OK, done and force-pushed into #work.xattr. I just checked, and while I think this is fine to do for the 'fd' taking {s,g}etxattr, I don't think the path taking ones should allow IOSQE_FIXED_FILE being set. It's nonsensical, as they don't take a file descriptor. So I'd prefer if we kept it to just the f* variants. I can just make this tweak in my io_uring 6.12 branch and get it upstream this week, that'll take it out of your hands. What do you think?
On 10/7/24 12:09 PM, Jens Axboe wrote: >>>> Questions on the io_uring side: >>>> * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time. >>>> Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case? >>>> Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway... >>>> Am I missing something subtle here? >>> >>> Right, it could be allowed for fgetxattr on the io_uring side. Anything >>> that passes in a struct file would be fair game to enable it on. >>> Anything that passes in a path (eg a non-fd value), it obviously >>> wouldn't make sense anyway. >> >> OK, done and force-pushed into #work.xattr. > > I just checked, and while I think this is fine to do for the 'fd' taking > {s,g}etxattr, I don't think the path taking ones should allow > IOSQE_FIXED_FILE being set. It's nonsensical, as they don't take a file > descriptor. So I'd prefer if we kept it to just the f* variants. I can > just make this tweak in my io_uring 6.12 branch and get it upstream this > week, that'll take it out of your hands. > > What do you think? Like the below. You can update yours if you want, or I can shove this into 6.12, whatever is the easiest for you. diff --git a/io_uring/xattr.c b/io_uring/xattr.c index 6cf41c3bc369..4b68c282c91a 100644 --- a/io_uring/xattr.c +++ b/io_uring/xattr.c @@ -48,9 +48,6 @@ static int __io_getxattr_prep(struct io_kiocb *req, const char __user *name; int ret; - if (unlikely(req->flags & REQ_F_FIXED_FILE)) - return -EBADF; - ix->filename = NULL; ix->ctx.kvalue = NULL; name = u64_to_user_ptr(READ_ONCE(sqe->addr)); @@ -90,6 +87,9 @@ int io_getxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) const char __user *path; int ret; + if (unlikely(req->flags & REQ_F_FIXED_FILE)) + return -EBADF; + ret = __io_getxattr_prep(req, sqe); if (ret) return ret; @@ -152,9 +152,6 @@ static int __io_setxattr_prep(struct io_kiocb *req, const char __user *name; int ret; - if (unlikely(req->flags & REQ_F_FIXED_FILE)) - return -EBADF; - ix->filename = NULL; name = u64_to_user_ptr(READ_ONCE(sqe->addr)); ix->ctx.cvalue = u64_to_user_ptr(READ_ONCE(sqe->addr2)); @@ -183,6 +180,9 @@ int io_setxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) const char __user *path; int ret; + if (unlikely(req->flags & REQ_F_FIXED_FILE)) + return -EBADF; + ret = __io_setxattr_prep(req, sqe); if (ret) return ret;
On Mon, Oct 07, 2024 at 12:20:20PM -0600, Jens Axboe wrote: > On 10/7/24 12:09 PM, Jens Axboe wrote: > >>>> Questions on the io_uring side: > >>>> * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time. > >>>> Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case? > >>>> Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway... > >>>> Am I missing something subtle here? > >>> > >>> Right, it could be allowed for fgetxattr on the io_uring side. Anything > >>> that passes in a struct file would be fair game to enable it on. > >>> Anything that passes in a path (eg a non-fd value), it obviously > >>> wouldn't make sense anyway. > >> > >> OK, done and force-pushed into #work.xattr. > > > > I just checked, and while I think this is fine to do for the 'fd' taking > > {s,g}etxattr, I don't think the path taking ones should allow > > IOSQE_FIXED_FILE being set. It's nonsensical, as they don't take a file > > descriptor. So I'd prefer if we kept it to just the f* variants. I can > > just make this tweak in my io_uring 6.12 branch and get it upstream this > > week, that'll take it out of your hands. > > > > What do you think? > > Like the below. You can update yours if you want, or I can shove this > into 6.12, whatever is the easiest for you. Can I put your s-o-b on that, with e.g. io_uring: IORING_OP_F[GS]ETXATTR is fine with REQ_F_FIXED_FILE Rejection of IOSQE_FIXED_FILE combined with IORING_OP_[GS]ETXATTR is fine - these do not take a file descriptor, so such combination makes no sense. The checks are misplaced, though - as it is, they triggers on IORING_OP_F[GS]ETXATTR as well, and those do take a file reference, no matter the origin. as commit message?
On 10/7/24 3:20 PM, Al Viro wrote: > On Mon, Oct 07, 2024 at 12:20:20PM -0600, Jens Axboe wrote: >> On 10/7/24 12:09 PM, Jens Axboe wrote: >>>>>> Questions on the io_uring side: >>>>>> * you usually reject REQ_F_FIXED_FILE for ...at() at ->prep() time. >>>>>> Fine, but... what's the point of doing that in IORING_OP_FGETXATTR case? >>>>>> Or IORING_OP_GETXATTR, for that matter, since you pass AT_FDCWD anyway... >>>>>> Am I missing something subtle here? >>>>> >>>>> Right, it could be allowed for fgetxattr on the io_uring side. Anything >>>>> that passes in a struct file would be fair game to enable it on. >>>>> Anything that passes in a path (eg a non-fd value), it obviously >>>>> wouldn't make sense anyway. >>>> >>>> OK, done and force-pushed into #work.xattr. >>> >>> I just checked, and while I think this is fine to do for the 'fd' taking >>> {s,g}etxattr, I don't think the path taking ones should allow >>> IOSQE_FIXED_FILE being set. It's nonsensical, as they don't take a file >>> descriptor. So I'd prefer if we kept it to just the f* variants. I can >>> just make this tweak in my io_uring 6.12 branch and get it upstream this >>> week, that'll take it out of your hands. >>> >>> What do you think? >> >> Like the below. You can update yours if you want, or I can shove this >> into 6.12, whatever is the easiest for you. > > Can I put your s-o-b on that, with e.g. > > io_uring: IORING_OP_F[GS]ETXATTR is fine with REQ_F_FIXED_FILE > > Rejection of IOSQE_FIXED_FILE combined with IORING_OP_[GS]ETXATTR > is fine - these do not take a file descriptor, so such combination > makes no sense. The checks are misplaced, though - as it is, they > triggers on IORING_OP_F[GS]ETXATTR as well, and those do take > a file reference, no matter the origin. Yep that's perfect, officially: Signed-off-by: Jens Axboe <axboe@kernel.dk> Thanks Al!
On Mon, Oct 07, 2024 at 04:29:29PM -0600, Jens Axboe wrote: > > Can I put your s-o-b on that, with e.g. > > > > io_uring: IORING_OP_F[GS]ETXATTR is fine with REQ_F_FIXED_FILE > > > > Rejection of IOSQE_FIXED_FILE combined with IORING_OP_[GS]ETXATTR > > is fine - these do not take a file descriptor, so such combination > > makes no sense. The checks are misplaced, though - as it is, they > > triggers on IORING_OP_F[GS]ETXATTR as well, and those do take > > a file reference, no matter the origin. > > Yep that's perfect, officially: > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > > Thanks Al! OK, updated and force-pushed (with slight reordering). I can almost promise no-rebase mode for that thing from now on, as long as nobody on fsdevel objects to fs/xattr.c part of things after I repost the series in the current form. One possible exception: I'm not sure that fs/internal.h is a good place for those primitives. OTOH, any bikeshedding in that direction can be delayed until the next cycle... To expand the circle of potential bikeshedders: s/do_mkdirat/filename_mkdirat/ is a reasonable idea for this series, innit? How about turning e.g. int __init init_mkdir(const char *pathname, umode_t mode) { struct dentry *dentry; struct path path; int error; dentry = kern_path_create(AT_FDCWD, pathname, &path, LOOKUP_DIRECTORY); if (IS_ERR(dentry)) return PTR_ERR(dentry); mode = mode_strip_umask(d_inode(path.dentry), mode); error = security_path_mkdir(&path, dentry, mode); if (!error) error = vfs_mkdir(mnt_idmap(path.mnt), path.dentry->d_inode, dentry, mode); done_path_create(&path, dentry); return error; } into int __init init_mkdir(const char *pathname, umode_t mode) { return filename_mkdirat(AT_FDCWD, getname_kernel(pathname), mode); } reducing the duplication? It really should not be accessible to random places in the kernel, but syscalls in core VFS + io_uring interface for the same + possibly init/*.c... OTOH, I'm afraid to let the "but our driver is sooo special!" crowd play with the full set of syscalls... init_syscalls.h is already bad enough. Hell knows, fs/internal.h just might be a bit of deterrent...
On 10/7/24 5:58 PM, Al Viro wrote: > On Mon, Oct 07, 2024 at 04:29:29PM -0600, Jens Axboe wrote: >>> Can I put your s-o-b on that, with e.g. >>> >>> io_uring: IORING_OP_F[GS]ETXATTR is fine with REQ_F_FIXED_FILE >>> >>> Rejection of IOSQE_FIXED_FILE combined with IORING_OP_[GS]ETXATTR >>> is fine - these do not take a file descriptor, so such combination >>> makes no sense. The checks are misplaced, though - as it is, they >>> triggers on IORING_OP_F[GS]ETXATTR as well, and those do take >>> a file reference, no matter the origin. >> >> Yep that's perfect, officially: >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> >> Thanks Al! > > OK, updated and force-pushed (with slight reordering). I can almost > promise no-rebase mode for that thing from now on, as long as nobody > on fsdevel objects to fs/xattr.c part of things after I repost the > series in the current form. No worries on my end in terms of rebasing, I have no plans to touch xattr.c for the coming series. Risk of conflict should be very low, so I don't even need to pull that in. > One possible exception: I'm not sure that fs/internal.h is a good > place for those primitives. OTOH, any bikeshedding in that direction > can be delayed until the next cycle... It ended up just being the defacto place to shove declarations for things like that. But it always felt a bit dirty, particularly needing to include that from the io_uring side as it moved out of fs/ as well. Would indeed be nice to get that cleaned up a bit. > To expand the circle of potential bikeshedders: s/do_mkdirat/filename_mkdirat/ > is a reasonable idea for this series, innit? How about turning e.g. > > int __init init_mkdir(const char *pathname, umode_t mode) > { > struct dentry *dentry; > struct path path; > int error; > > dentry = kern_path_create(AT_FDCWD, pathname, &path, LOOKUP_DIRECTORY); > if (IS_ERR(dentry)) > return PTR_ERR(dentry); > mode = mode_strip_umask(d_inode(path.dentry), mode); > error = security_path_mkdir(&path, dentry, mode); > if (!error) > error = vfs_mkdir(mnt_idmap(path.mnt), path.dentry->d_inode, > dentry, mode); > done_path_create(&path, dentry); > return error; > } > > into > > int __init init_mkdir(const char *pathname, umode_t mode) > { > return filename_mkdirat(AT_FDCWD, getname_kernel(pathname), mode); > } > > reducing the duplication? It really should not be accessible to random > places in the kernel, but syscalls in core VFS + io_uring interface for > the same + possibly init/*.c... > > OTOH, I'm afraid to let the "but our driver is sooo special!" crowd play > with the full set of syscalls... init_syscalls.h is already bad enough. > Hell knows, fs/internal.h just might be a bit of deterrent... Deduping it is a good thing, suggestion looks good to me. For random drivers, very much agree. But are there any of these symbols we end up exporting? That tends to put a damper on the enthusiasm...
On Mon, Oct 07, 2024 at 07:58:15PM -0600, Jens Axboe wrote: > > OTOH, I'm afraid to let the "but our driver is sooo special!" crowd play > > with the full set of syscalls... init_syscalls.h is already bad enough. > > Hell knows, fs/internal.h just might be a bit of deterrent... > > Deduping it is a good thing, suggestion looks good to me. For random > drivers, very much agree. But are there any of these symbols we end up > exporting? That tends to put a damper on the enthusiasm... You wish... init_unlink() et.al. are not just not exported, they are freed once the system is booted. Doesn't stop that kind of magic being attempted.
diff --git a/fs/internal.h b/fs/internal.h index b9f5ac4d39fc..be7c0da3bcec 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -285,10 +285,10 @@ ssize_t do_getxattr(struct mnt_idmap *idmap, struct dentry *d, struct kernel_xattr_ctx *ctx); +int file_setxattr(struct file *file, struct kernel_xattr_ctx *ctx); +int filename_setxattr(int dfd, struct filename *filename, + unsigned int lookup_flags, struct kernel_xattr_ctx *ctx); int setxattr_copy(const char __user *name, struct kernel_xattr_ctx *ctx); -int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, - struct kernel_xattr_ctx *ctx); - int import_xattr_name(struct xattr_name *kname, const char __user *name); int may_write_xattr(struct mnt_idmap *idmap, struct inode *inode); diff --git a/fs/xattr.c b/fs/xattr.c index d8f7c766f28a..6326a1ea28e9 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -626,7 +626,7 @@ int setxattr_copy(const char __user *name, struct kernel_xattr_ctx *ctx) return error; } -int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, +static int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, struct kernel_xattr_ctx *ctx) { if (is_posix_acl_xattr(ctx->kname->name)) @@ -637,32 +637,31 @@ int do_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, ctx->kvalue, ctx->size, ctx->flags); } -static int path_setxattr(const char __user *pathname, - const char __user *name, const void __user *value, - size_t size, int flags, unsigned int lookup_flags) +int file_setxattr(struct file *f, struct kernel_xattr_ctx *ctx) +{ + int error = mnt_want_write_file(f); + + if (!error) { + audit_file(f); + error = do_setxattr(file_mnt_idmap(f), f->f_path.dentry, ctx); + mnt_drop_write_file(f); + } + return error; +} + +int filename_setxattr(int dfd, struct filename *filename, + unsigned int lookup_flags, struct kernel_xattr_ctx *ctx) { - struct xattr_name kname; - struct kernel_xattr_ctx ctx = { - .cvalue = value, - .kvalue = NULL, - .size = size, - .kname = &kname, - .flags = flags, - }; struct path path; int error; - error = setxattr_copy(name, &ctx); - if (error) - return error; - retry: - error = user_path_at(AT_FDCWD, pathname, lookup_flags, &path); + error = filename_lookup(dfd, filename, lookup_flags, &path, NULL); if (error) goto out; error = mnt_want_write(path.mnt); if (!error) { - error = do_setxattr(mnt_idmap(path.mnt), path.dentry, &ctx); + error = do_setxattr(mnt_idmap(path.mnt), path.dentry, ctx); mnt_drop_write(path.mnt); } path_put(&path); @@ -672,6 +671,30 @@ static int path_setxattr(const char __user *pathname, } out: + putname(filename); + return error; +} + +static int path_setxattr(const char __user *pathname, + const char __user *name, const void __user *value, + size_t size, int flags, unsigned int lookup_flags) +{ + struct xattr_name kname; + struct kernel_xattr_ctx ctx = { + .cvalue = value, + .kvalue = NULL, + .size = size, + .kname = &kname, + .flags = flags, + }; + int error; + + error = setxattr_copy(name, &ctx); + if (error) + return error; + + error = filename_setxattr(AT_FDCWD, getname(pathname), lookup_flags, + &ctx); kvfree(ctx.kvalue); return error; } @@ -707,17 +730,12 @@ SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name, if (fd_empty(f)) return -EBADF; - audit_file(fd_file(f)); + error = setxattr_copy(name, &ctx); if (error) return error; - error = mnt_want_write_file(fd_file(f)); - if (!error) { - error = do_setxattr(file_mnt_idmap(fd_file(f)), - fd_file(f)->f_path.dentry, &ctx); - mnt_drop_write_file(fd_file(f)); - } + error = file_setxattr(fd_file(f), &ctx); kvfree(ctx.kvalue); return error; } diff --git a/io_uring/xattr.c b/io_uring/xattr.c index 71d9e2569a2f..7f6bbfd846b9 100644 --- a/io_uring/xattr.c +++ b/io_uring/xattr.c @@ -200,28 +200,14 @@ int io_fsetxattr_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe) return __io_setxattr_prep(req, sqe); } -static int __io_setxattr(struct io_kiocb *req, unsigned int issue_flags, - const struct path *path) -{ - struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); - int ret; - - ret = mnt_want_write(path->mnt); - if (!ret) { - ret = do_setxattr(mnt_idmap(path->mnt), path->dentry, &ix->ctx); - mnt_drop_write(path->mnt); - } - - return ret; -} - int io_fsetxattr(struct io_kiocb *req, unsigned int issue_flags) { + struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); int ret; WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); - ret = __io_setxattr(req, issue_flags, &req->file->f_path); + ret = file_setxattr(req->file, &ix->ctx); io_xattr_finish(req, ret); return IOU_OK; } @@ -229,23 +215,11 @@ int io_fsetxattr(struct io_kiocb *req, unsigned int issue_flags) int io_setxattr(struct io_kiocb *req, unsigned int issue_flags) { struct io_xattr *ix = io_kiocb_to_cmd(req, struct io_xattr); - unsigned int lookup_flags = LOOKUP_FOLLOW; - struct path path; int ret; WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); -retry: - ret = filename_lookup(AT_FDCWD, ix->filename, lookup_flags, &path, NULL); - if (!ret) { - ret = __io_setxattr(req, issue_flags, &path); - path_put(&path); - if (retry_estale(ret, lookup_flags)) { - lookup_flags |= LOOKUP_REVAL; - goto retry; - } - } - + ret = filename_setxattr(AT_FDCWD, ix->filename, LOOKUP_FOLLOW, &ix->ctx); io_xattr_finish(req, ret); return IOU_OK; }
io_uring setxattr logics duplicates stuff from fs/xattr.c; provide saner helpers (filename_setxattr() and file_setxattr() resp.) and use them. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/internal.h | 6 ++--- fs/xattr.c | 68 ++++++++++++++++++++++++++++++------------------ io_uring/xattr.c | 32 +++-------------------- 3 files changed, 49 insertions(+), 57 deletions(-)