Re: [Devel] [PATCH hci-8.1 v2 6/6] dm/push_backup: add event file descriptor interface

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dm-push-backup: push_backup_get_fd() installs fd before result is delivered
- [Low] dm-push-backup: event fd has no .poll method, so it cannot be multiplexed
--

commit 586e161f2dd08e2697bfd3552aef44d435e3d52e
Author: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>

dm/push_backup: add event file descriptor interface

This adds a "push_backup_get_fd" target message that returns an anon inode
fd. The backup tool can read() the next range to back up and write() to
acknowledge it, without sleeping inside a dm message. A kref on struct
push_backup is tied to the number of open fds.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-152912

> diff --git a/drivers/md/dm-push-backup.c b/drivers/md/dm-push-backup.c
> --- a/drivers/md/dm-push-backup.c
> +++ b/drivers/md/dm-push-backup.c

[ ... ]

> +static const struct file_operations pb_fops = {
> +	.owner		= THIS_MODULE,
> +	.read		= pb_fd_read,
> +	.write		= pb_fd_write,
> +	.release	= pb_fd_release,
> +};

[Severity: Low]
This isn't a bug, but pb_fops provides no .poll method, although
pb->waitq and pb_has_event() already exist and could back one.

Without .poll, poll() and select() report the fd as always ready, and
epoll_ctl() rejects it.

A consumer that wants to multiplex the fd must dedicate a thread to a
blocking read(), or retry read() on -EAGAIN with O_NONBLOCK. The UAPI
header documents the O_NONBLOCK -EAGAIN mode, but without .poll it is hard
to use.

Would it make sense to add .poll before the UAPI is finalized?

There is also no .llseek or stream_open(), so the fd is seekable even though
f_pos is meaningless. Should this be a stream-like file?

> +static int push_backup_get_fd(struct push_backup *pb,
> +			      char *result, unsigned int maxlen)
> +{
> +	unsigned int sz = 0;
> +	int fd;
> +
> +	kref_get(&pb->ref);
> +	fd = anon_inode_getfd("[push_backup]", &pb_fops, pb,
> +			      O_RDWR | O_CLOEXEC);
> +	if (fd < 0) {
> +		pb_put(pb);
> +		return fd;
> +	}
> +
> +	DMEMIT("%d", fd);
> +	return 1;
> +}

[Severity: Medium]
Can this leak a file descriptor in the calling process?

anon_inode_getfd() calls fd_install() before the result is known to be
deliverable. After that, DMEMIT("%d", fd) only formats into the kernel
result buffer. It silently truncates if maxlen is too small, and
push_backup_get_fd() still returns 1.

If the DMEMIT output is truncated, or the dm ioctl core later fails to copy
the result out to userspace, the caller never learns the fd number:

pb_message()
  push_backup_get_fd()
    kref_get(&pb->ref);
    anon_inode_getfd()      <- fd_install() already done
    DMEMIT("%d", fd);       <- may truncate
    return 1;

The caller cannot close that fd. The fd keeps its kref on struct push_backup
and a module reference, so the target memory and the module stay pinned until
the process exits.

Would it be better to reserve the fd with get_unused_fd_flags(), check that
the result fits, and call fd_install() last?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261103003246.712239-1-andrey.zhadchenko@virtuozzo.com?part=6

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel