Re: [Qemu-devel] [PATCH 2/7] nbd/server: Trace server noncompliance on unaligned requests

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5970 bytes --]

On 4/5/19 9:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.04.2019 6:05, Eric Blake wrote:
>> We've recently added traces for clients to flag server non-compliance;
>> let's do the same for servers to flag client non-compliance. According

Thus, s/Trace server/Trace client/ in the subject line.

>>
>> Qemu as client used to have one spot where it sent non-compliant
>> requests: if the server sends an unaligned reply to
>> NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire
>> disk, the next request would start at that unaligned point; this was
>> fixed in commit a39286dd when the client was taught to work around
>> server non-compliance; but is equally fixed if the server is patched
>> to not send unaligned replies in the first place (the next few patches
>> will address that).

I'll have to reword this now that we know 4.0 will still have some of
those server bugs in place (as the tail of this series is deferred to 4.1).


>> @@ -660,6 +662,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
>>
>>       if (client->opt == NBD_OPT_GO) {
>>           client->exp = exp;
>> +        client->check_align = blocksize ?
>> +            blk_get_request_alignment(exp->blk) : 0;
> 
> I think better set in same place, where sizes[0] set, or use sizes[0] here or add separate local
> varibale for it, so that, if "sizes[0] =" changes somehow, we will not forget to fix this place too.

I don't want to set client->check_align for NBD_OPT_INFO; but I can
indeed use a temporary variable or hoist the computation so that it is
all in one spot.


>> +++ b/nbd/trace-events
>> @@ -71,4 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t
>>   nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
>>   nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
>>   nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
>> +nbd_co_receive_align_compliance(const char *op) "client sent non-compliant unaligned %s request"
> 
> Don't you want print request->from, request->len and client->check_align as well?

Wouldn't hurt.

> 
>>   nbd_trip(void) "Reading request"
>>
> 
> Patch seems correct anyway, so if you are in a hurry, it's OK as is:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

Here's what I'll squash in; I think it is obvious enough to still keep
your R-b, if I send the pull request before you reply back.

diff --git i/nbd/server.c w/nbd/server.c
index 3040ceb5606..cb38dfe6902 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -535,6 +535,7 @@ static int nbd_negotiate_handle_info(NBDClient
*client, uint16_t myflags,
     bool blocksize = false;
     uint32_t sizes[3];
     char buf[sizeof(uint64_t) + sizeof(uint16_t)];
+    uint32_t check_align = 0;

     /* Client sends:
         4 bytes: L, name length (can be 0)
@@ -611,7 +612,7 @@ static int nbd_negotiate_handle_info(NBDClient
*client, uint16_t myflags,
      * whether this is OPT_INFO or OPT_GO. */
     /* minimum - 1 for back-compat, or actual if client will obey it. */
     if (client->opt == NBD_OPT_INFO || blocksize) {
-        sizes[0] = blk_get_request_alignment(exp->blk);
+        check_align = sizes[0] = blk_get_request_alignment(exp->blk);
     } else {
         sizes[0] = 1;
     }
@@ -662,8 +663,7 @@ static int nbd_negotiate_handle_info(NBDClient
*client, uint16_t myflags,

     if (client->opt == NBD_OPT_GO) {
         client->exp = exp;
-        client->check_align = blocksize ?
-            blk_get_request_alignment(exp->blk) : 0;
+        client->check_align = check_align;
         QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
         nbd_export_get(client->exp);
         nbd_check_meta_export(client);
@@ -2136,7 +2136,10 @@ static int nbd_co_receive_request(NBDRequestData
*req, NBDRequest *request,
          * The block layer gracefully handles unaligned requests, but
          * it's still worth tracing client non-compliance
          */
-
trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type));
+        trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type,
+                                                             request->from,
+                                                             request->len,
+
client->check_align));
     }
     valid_flags = NBD_CMD_FLAG_FUA;
     if (request->type == NBD_CMD_READ && client->structured_reply) {
diff --git i/nbd/trace-events w/nbd/trace-events
index 87a2b896c35..ec2d46c9247 100644
--- i/nbd/trace-events
+++ w/nbd/trace-events
@@ -71,5 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int
extents, uint32_t id, uint64_t
 nbd_co_send_structured_error(uint64_t handle, int err, const char
*errname, const char *msg) "Send structured error reply: handle = %"
PRIu64 ", error = %d (%s), msg = '%s'"
 nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type,
const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16
" (%s)"
 nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len)
"Payload received: handle = %" PRIu64 ", len = %" PRIu32
-nbd_co_receive_align_compliance(const char *op) "client sent
non-compliant unaligned %s request"
+nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t
len, uint32_t align) "client sent non-compliant unaligned %s request:
from=0x%" PRIx64 ", len=0x%x, align=0x%x"
 nbd_trip(void) "Reading request"



-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]