Re: [Devel] [PATCH hci-8.1 2/3] fs/fuse kio: refactor transmit path in pcs_sockio_send()

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] fs/fuse kio: do_sockio_sendmsg() can spin forever on an empty get_iter() refill
- [Low] fs/fuse kio: iov_iter_sendpages_ok() under-checks pages of unaligned multi-page bvecs
--

commit 2b6fc40ff00b50732f7cf6408214e55943ba4135
Author: Liu Kui <kui.liu@virtuozzo.com>

fs/fuse kio: refactor transmit path in pcs_sockio_send()

This patch replaces do_send_one_seg() with do_sockio_sendmsg(). The new
function submits the whole iov_iter in one sock_sendmsg() call, and uses
MSG_SPLICE_PAGES when all pages qualify. It also clears PCS_MSG_BUSY in
the transport whenever IO on the msg ends.

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

> diff --git a/fs/fuse/kio/pcs/pcs_sock_io.c b/fs/fuse/kio/pcs/pcs_sock_io.c
> --- a/fs/fuse/kio/pcs/pcs_sock_io.c
> +++ b/fs/fuse/kio/pcs/pcs_sock_io.c

[ ... ]

> +static bool iov_iter_sendpages_ok(struct iov_iter *it)
> +{
> +	const struct bio_vec *bvec = it->bvec;
> +
> +	if (!iov_iter_is_bvec(it))
> +		return false;
> +
> +	while (bvec < (it->bvec + it->nr_segs)) {
> +		if (!sendpages_ok(bvec->bv_page, bvec->bv_len, bvec->bv_offset))
> +			return false;
> +		bvec++;
> +	}
> +
> +	return true;
> +}

[Severity: Low]
Can iov_iter_sendpages_ok() skip the last page of a multi-page bvec that
has an unaligned bv_offset?

sendpages_ok() starts at page + (offset >> PAGE_SHIFT) and steps
PAGE_SIZE for len bytes.  For example, with bv_offset=2048 and
bv_len=4096 the range covers two pages, but only one is checked with
sendpage_ok().

The bvec builder in pcs_krpc.c (umem branch) sets bv_offset to a
non-page-aligned value and bv_len up to the contiguous length, so such
bvecs look possible:

    bvec->bv_offset = offset;
    ...
    bvec->bv_len = len < contig ? len : contig;

If that happens, MSG_SPLICE_PAGES could be set in do_sockio_sendmsg()
while the unchecked page is a slab page or has a zero refcount.  That
page would then be spliced into the socket by reference instead of being
copied.  Is the page check meant to cover the pages that the byte range
actually touches?

> +static int do_sockio_sendmsg(struct pcs_sockio *sio, struct pcs_msg *msg)
> +{

[ ... ]

> +	while (sio->write_offset < msg->size) {
> +		size_t left = msg->size - sio->write_offset;
> +		struct msghdr sockmsg = {.msg_flags = flags};
> +
> +		TRACE("sio(%px) offset:%d msg:%px left:%zu, it->len:%zu\n", sio, sio->write_offset, msg, left, iov_iter_count(it));
> +
> +		if (!iov_iter_count(it)) {
> +			/* Current iter is exhausted, init new one */
> +			msg->get_iter(msg, sio->write_offset, it, WRITE);
> +		}
> +
> +		BUG_ON(iov_iter_count(it) > left);

[ ... ]

> +		while (iov_iter_count(it)) {

[Severity: Medium]
Can do_sockio_sendmsg() spin forever here if msg->get_iter() returns an
iterator with a zero count while sio->write_offset < msg->size?

In that case the BUG_ON(iov_iter_count(it) > left) check passes, the
inner while (iov_iter_count(it)) loop is skipped, and sock_sendmsg() is
never called.  WARN_ON(msg_data_left(&sockmsg)) sees zero and does not
fire.  write_offset does not change, so the outer loop repeats with the
same state and no exit, which would be a soft lockup.

The old do_send_one_seg() returned 0 for an empty segment, and
pcs_sockio_send() turned that into a WARN_ON and sio_abort(). I did not
find a get_iter() callback that returns an empty iterator for an
in-range offset (krpc_msg_get_data() and pcs_get_iter_inline() look
fine), so this looks latent.  Would it make sense to add a check that
fails the message when get_iter() returns an empty iterator?

> +			int ret;

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20260930-sockio-refactor-8-1-v1-0-806348b9eb26@virtuozzo.com?part=2

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