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@xxxxxxxxx>
>>
>> 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...
--
Jens Axboe
