Re: [PATCH 5/9] replace do_setxattr() with saner helpers.

[Jens Axboe <axboe@kernel.dk>]

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

-- 
Jens Axboe