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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

i have commited this patch, but this review looks valid, is you agree,
please consider sending an incremental patch like

  diff --git a/fs/fuse/kio/pcs/pcs_sock_io.c b/fs/fuse/kio/pcs/pcs_sock_io.c
  index 2da6d51c898b0..3a2228088540a 100644
  --- a/fs/fuse/kio/pcs/pcs_sock_io.c
  +++ b/fs/fuse/kio/pcs/pcs_sock_io.c
  @@ -307,6 +307,12 @@ static int do_sockio_sendmsg(struct pcs_sockio *sio, struct pcs_msg *msg)
                if (!iov_iter_count(it)) {
                        /* Current iter is exhausted, init new one */
                        msg->get_iter(msg, sio->write_offset, it, WRITE);
  +
  +                     /* An empty refill would spin this loop forever */
  +                     if (WARN_ON(!iov_iter_count(it))) {
  +                             clear_and_wake_up_bit(PCS_MSG_BUSY, &msg->flags);
  +                             return -PCS_ERR_NET_ABORT;
  +                     }
                }

                BUG_ON(iov_iter_count(it) > left);

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 9/30/26 09:42, no-reply@sashiko.virtuozzo.com wrote:
> 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