1:26 p.m.
On 31.05.23 20:40, Eric Blake wrote:
On Wed, May 31, 2023 at 08:00:43PM +0300, Vladimir
Sementsov-Ogievskiy wrote:
> On 15.05.23 22:53, Eric Blake wrote:
>> Because we use NBD_CMD_FLAG_REQ_ONE with NBD_CMD_BLOCK_STATUS, a
>> client in narrow mode should not be able to provoke a server into
>> sending a block status result larger than the client's 32-bit request.
>> But in extended mode, a 64-bit status request must be able to handle a
>> 64-bit status result, once a future patch enables the client
>> requesting extended mode. We can also tolerate a non-compliant server
>> sending the new chunk even when it should not.
>>
>> @@ -672,7 +681,8 @@ static int nbd_parse_blockstatus_payload(BDRVNBDState *s,
>> * connection; just ignore trailing extents, and clamp things to
>> * the length of our request.
>> */
>> - if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
>> + if (count != wide ||
>
> hard to read for me. Could it be simply "count > 1 ||" ?
For existing commands (compact), count is initialized to 0 as it is
not transferred over the wire. For extended commands, count is
transferred over the wire, but we expect it to be 1 (and not 0).
Comparing count != wide is more precise than checking count > 0 (which
should never happen for compact, but would be a bug for extended).
The only case you add to the check is when count = 0 for extended. But in this case
"more than one extent" message is counterintuitive.
>
>> + chunk->length > sizeof(context_id) + wide * sizeof(count) + len)
{
>> trace_nbd_parse_blockstatus_compliance("more than one
extent");
>> }
>> if (extent->length > orig_length) {
>> @@ -1117,7 +1127,7 @@ static int coroutine_fn
nbd_co_receive_cmdread_reply(BDRVNBDState *s, uint64_t h
>>
>> static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>> uint64_t handle,
uint64_t length,
>> - NBDExtent *extent,
>> + NBDExtentExt *extent,
>> int *request_ret,
Error **errp)
>> {
>> NBDReplyChunkIter iter;
>> @@ -1125,6 +1135,7 @@ static int coroutine_fn
nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>> void *payload = NULL;
>> Error *local_err = NULL;
>> bool received = false;
>> + bool wide = false;
>>
>> assert(!extent->length);
>> NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply,
&payload) {
>> @@ -1134,7 +1145,13 @@ static int coroutine_fn
nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>> assert(nbd_reply_is_structured(&reply));
>>
>> switch (chunk->type) {
>> + case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
>> + wide = true;
>> + /* fallthrough */
>> case NBD_REPLY_TYPE_BLOCK_STATUS:
>> + if (s->info.extended_headers != wide) {
>> +
trace_nbd_extended_headers_compliance("block_status");
>
> You set wide to true once, on first "NBD_REPLY_TYPE_BLOCK_STATUS_EXT", and
then parse following "NBD_REPLY_TYPE_BLOCK_STATUS" if the come with wide=true.
>
> Should it be:
>
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1135,7 +1135,7 @@ static int coroutine_fn
nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
> void *payload = NULL;
> Error *local_err = NULL;
> bool received = false;
> - bool wide = false;
> + bool wide;
> assert(!extent->length);
> NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload)
{
> @@ -1146,9 +1146,8 @@ static int coroutine_fn
nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
> switch (chunk->type) {
> case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
> - wide = true;
> - /* fallthrough */
> case NBD_REPLY_TYPE_BLOCK_STATUS:
> + wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT;
> if (s->info.extended_headers != wide) {
Good observation, since we reach this multiple times in a loop. I'm
squashing that in.
--
Best regards,
Vladimir