Re: [Devel] [PATCH hci-8.1 0/3] drivers/vhost/blk: fix virtio-blk request header and payload parsing

[Konstantin Khorenko <khorenko@virtuozzo.com>]

Hi Alexander,

The code in all three patches is fine, I have nothing to change there.
Two commit messages describe the old behaviour inaccurately, please fix
them in place for v2.

Patch 2, "take virtio_blk_outhdr only from the readable part":

> The request header was moved from the whole descriptor chain, so a
> chain with no readable part, or one shorter than the header, had header
> bytes read from buffers the guest marked device-writable.

The "shorter than the header" case was already rejected before the
patch: VHOST_MAX_METADATA_IOV is 1, so move_iovec() fills one target
segment and returns -1 when the header does not fit into the first
readable iovec, and the kick handler drops the chain as "too split".
Only a chain with no readable part at all had the header taken from a
writable buffer.

> Those bytes
> were also trimmed in place from the writable segments, shifting the
> status byte and the READ/GET_ID payload taken from them.

The status byte did not move: it was taken as the last byte of the
chain after req->len was computed. What shifted was the start of the
READ/GET_ID payload, by sizeof(hdr).

Suggested paragraph:

  The request header was moved from the whole descriptor chain, so a
  chain with no readable part had header bytes read from buffers the
  guest marked device-writable. Those bytes were also trimmed in place
  from the writable segments, shifting the start of the READ/GET_ID
  payload taken from them.

Patch 3, "take req->len from one side of the descriptor chain":

> Complete a read or write with VIRTIO_BLK_S_IOERR if its payload is
> empty, above ALIGN_DOWN(INT_MAX, SECTOR_SIZE) or, for a write, not a
> multiple of SECTOR_SIZE. Such requests used to fail in bio setup or
> hit a WARN_ONCE() and leave the guest request hanging. Drop the now
> unreachable WARN_ONCE().

Three things here.

"Such requests used to fail" is true only for the empty payload and the
unaligned write. A write above ALIGN_DOWN(INT_MAX, SECTOR_SIZE) was
never rejected before: req->len is size_t, the bios are split per
BIO_MAX_VECS and used_len for a write is one byte, so it was submitted
as is. The bound is a new restriction for writes and the message should
say so. For reads, the in_len > INT_MAX check still runs first, so a
read above INT_MAX keeps getting -EINVAL; the new bound only covers the
range below it.

"Leave the guest request hanging" understates it. On -EINVAL the kick
handler breaks out of the loop with the descriptor consumed and
notifications still disabled, so with EVENT_IDX the guest never kicks
again and the whole virtqueue stalls until QEMU resets it through
error_ctx.

The WARN_ONCE() was reachable from the guest: any write with an
unaligned length triggered it, and with panic_on_warn that panics the
host. That is the reason for the Fixes: 02ce37f62707 trailer and worth
one sentence, "now unreachable" is a side effect.

Suggested paragraphs:

  Complete a read or write with VIRTIO_BLK_S_IOERR if its payload is
  empty, above ALIGN_DOWN(INT_MAX, SECTOR_SIZE) or, for a write, not a
  multiple of SECTOR_SIZE. An empty payload used to fail in bio setup
  and an unaligned write hit a WARN_ONCE() reachable from the guest.
  In both cases the kick handler broke out with the descriptor consumed
  and notifications disabled, so the virtqueue stalled. Drop the
  WARN_ONCE().

  The size bound is new for writes: the readable side had no limit and
  a write of any size was submitted. Reads above INT_MAX still get
  -EINVAL from the in_len check, the bound only covers the range below
  it.

With the two messages fixed the series is good to go, no code changes
needed.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team


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