Re: [RFC PATCH 2/3] io_uring: add support for utimensat

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] io_uring: missing flags check and wrong NULL path handling in io_utimensat()
--

commit 8d82d4a2e30073e0345719074be6f5140b1c8829
Author: Méven Car <meven29@gmail.com>

io_uring: add support for utimensat

This commit adds the IORING_OP_UTIMENSAT opcode to io_uring to support
updating file timestamps asynchronously via io-wq. It copies the path and
timespec arguments during submission prep and invokes filename_utimensat()
when issued.

> diff --git a/io_uring/fs.c b/io_uring/fs.c
> index 26ea841a22e70..85e0a1cb6b479 100644
> --- a/io_uring/fs.c
> +++ b/io_uring/fs.c
> @@ -304,3 +304,102 @@ void io_link_cleanup(struct io_kiocb *req)
>  	dismiss_delayed_filename(&sl->oldpath);
>  	dismiss_delayed_filename(&sl->newpath);
>  }
> +
> +/* Internal flags, above the AT_ flags of the SQE */
> +#define IO_UTIMENSAT_NOOP	BIT(30)	/* both times UTIME_OMIT */
> +#define IO_UTIMENSAT_NO_PATH	BIT(31)	/* NULL path: the dfd itself */
> +
> +struct io_utimensat {
> +	struct file			*file;
> +	int				dfd;
> +	unsigned int			flags;
> +	struct delayed_filename		filename;
> +	/* NULL sets both times to the current time */
> +	struct timespec64		*times;
> +	struct timespec64		ts[2];
> +};
> +
> +int io_utimensat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> +{
> +	struct io_utimensat *ut = io_kiocb_to_cmd(req, struct io_utimensat);
> +	struct __kernel_timespec __user *uts;
> +	const char __user *fname;
> +	int err;
> +
> +	if (sqe->len || sqe->buf_index || sqe->splice_fd_in || sqe->addr3 ||
> +	    sqe->__pad2[0])
> +		return -EINVAL;
> +	if (unlikely(req->flags & REQ_F_FIXED_FILE))
> +		return -EBADF;
> +
> +	ut->dfd = READ_ONCE(sqe->fd);
> +	ut->flags = READ_ONCE(sqe->utimes_flags);
> +	if (ut->flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH))
> +		return -EINVAL;
> +
> +	uts = u64_to_user_ptr(READ_ONCE(sqe->addr2));
> +	ut->times = NULL;
> +	if (uts) {
> +		if (get_timespec64(&ut->ts[0], &uts[0]) ||
> +		    get_timespec64(&ut->ts[1], &uts[1]))
> +			return -EFAULT;
> +		ut->times = ut->ts;
> +		if (ut->ts[0].tv_nsec == UTIME_OMIT &&
> +		    ut->ts[1].tv_nsec == UTIME_OMIT) {
> +			ut->flags |= IO_UTIMENSAT_NOOP;
> +			return 0;
> +		}
> +	}
> +
> +	fname = u64_to_user_ptr(READ_ONCE(sqe->addr));
> +	if (!fname) {
> +		if (ut->dfd == AT_FDCWD)
> +			return -EFAULT;
> +		ut->flags |= IO_UTIMENSAT_NO_PATH;
> +		goto out;
> +	}
> +	err = delayed_getname_uflags(&ut->filename, fname, ut->flags);
> +	if (unlikely(err))
> +		return err;
> +	req->flags |= REQ_F_NEED_CLEANUP;
> +out:
> +	req->flags |= REQ_F_FORCE_ASYNC;
> +	return 0;
> +}
> +
> +static int __io_utimensat(struct io_utimensat *ut, struct filename *name)
> +{
> +	int flags = ut->flags & (AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH);
> +
> +	return filename_utimensat(ut->dfd, name, ut->times, flags);
> +}
> +
> +int io_utimensat(struct io_kiocb *req, unsigned int issue_flags)
> +{
> +	struct io_utimensat *ut = io_kiocb_to_cmd(req, struct io_utimensat);
> +	int ret = 0;
> +
> +	WARN_ON_ONCE((issue_flags & IO_URING_F_NONBLOCK) &&
> +		     !(ut->flags & IO_UTIMENSAT_NOOP));
> +
> +	if (ut->flags & IO_UTIMENSAT_NO_PATH) {
> +		CLASS(filename_kernel, name)("");
> +		ret = __io_utimensat(ut, name);

[Severity: Medium]
When sqe->addr is NULL (the IO_UTIMENSAT_NO_PATH case), should
io_utimensat_prep() and io_utimensat() follow do_utimes_fd() instead of
allocating an empty filename and routing through filename_utimensat()?

In fs/utimes.c, do_utimes() dispatches a NULL filename (with dfd != AT_FDCWD)
to do_utimes_fd():

fs/utimes.c:do_utimes_fd() {
	if (flags)
		return -EINVAL;

	CLASS(fd, f)(fd);
	if (fd_empty(f))
		return -EBADF;
	return vfs_utimes(&fd_file(f)->f_path, times);
}

Routing IO_UTIMENSAT_NO_PATH through getname_kernel("") -> __io_utimensat() ->
filename_utimensat() -> filename_lookup() differs from utimensat(2) in several
ways:

1. In io_utimensat_prep(), non-zero sqe->utimes_flags (AT_SYMLINK_NOFOLLOW or
AT_EMPTY_PATH) are accepted when fname is NULL, whereas do_utimes_fd() rejects
any non-zero flags with -EINVAL.

2. In filename_lookup() -> path_lookupat(), an empty string returns from
link_path_walk() with nd->last_type == LAST_ROOT without calling step_into(),
so LOOKUP_FOLLOW is not applied to dfd itself. If sqe->fd is an O_PATH |
O_NOFOLLOW descriptor referring to a symlink and sqe->addr is NULL, this
modifies the symlink itself even when AT_SYMLINK_NOFOLLOW is not set.

3. In path_lookupat(), path_init() sets ND_JUMPED and complete_walk() invokes
d_op->d_weak_revalidate() on filesystems such as NFS and 9p. If sqe->fd refers
to an open file that has been unlinked, d_weak_revalidate() fails and
filename_utimensat() returns -ESTALE (after an extra server round-trip),
whereas do_utimes_fd() calls vfs_utimes() directly on fd_file(f)->f_path and
succeeds.

4. Every IO_UTIMENSAT_NO_PATH call allocates a 4KB struct filename from
names_cachep and records a synthetic empty filename in audit_getname().

Also, the commit message states:

"Unlike the syscall, a NULL path or AT_EMPTY_PATH also works on an O_PATH
descriptor."

Doesn't the utimensat(2) syscall already support AT_EMPTY_PATH with a non-NULL
empty path on an O_PATH descriptor via do_utimes_path() ->
filename_utimensat() -> filename_lookup(), while only rejecting O_PATH on a
NULL path in do_utimes_fd()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030123945.1704118-1-meven@kde.org?part=2