[Qemu-commits] [qemu/qemu] 08ace1: nbd: Don't crash when server reports NBD_CMD_READ ...

[GitHub]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 08ace1d75372b57c5ab56aea02c71cdda4b58fdf
      
https://github.com/qemu/qemu/commit/08ace1d75372b57c5ab56aea02c71cdda4b58fdf
  Author: Eric Blake <address@hidden>
  Date:   2017-11-17 (Fri, 17 Nov 2017)

  Changed paths:
    M block/nbd-client.c

  Log Message:
  -----------
  nbd: Don't crash when server reports NBD_CMD_READ failure

If a server fails a read, for example with EIO, but the connection
is still live, then we would crash trying to print a non-existent
error message in nbd_client_co_preadv().  For consistency, also
change the error printout in nbd_read_reply_entry(), although that
instance does not crash.  Bug introduced in commit f140e300.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: cb6b1a3fc30c52ffd94ed0b69ca5991b19651724
      
https://github.com/qemu/qemu/commit/cb6b1a3fc30c52ffd94ed0b69ca5991b19651724
  Author: Eric Blake <address@hidden>
  Date:   2017-11-17 (Fri, 17 Nov 2017)

  Changed paths:
    M nbd/client.c

  Log Message:
  -----------
  nbd/client: Use error_prepend() correctly

When using error prepend(), it is necessary to end with a space
in the format string; otherwise, messages come out incorrectly,
such as when connecting to a socket that hangs up immediately:

can't open device nbd://localhost:10809/: Failed to read dataUnexpected 
end-of-file before all bytes were read

Originally botched in commit e44ed99d, then several more instances
added in the meantime.

Pre-existing and not fixed here: we are inconsistent on capitalization;
some of our messages start with lower case, and others start with upper,
although the use of error_prepend() is much nicer to read when all
fragments consistently start with lower.

CC: address@hidden
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Reviewed-by: Markus Armbruster <address@hidden>


  Commit: 01b05c66a3616d5a4adc39fc90962e9efaf791d1
      
https://github.com/qemu/qemu/commit/01b05c66a3616d5a4adc39fc90962e9efaf791d1
  Author: Eric Blake <address@hidden>
  Date:   2017-11-17 (Fri, 17 Nov 2017)

  Changed paths:
    M nbd/client.c

  Log Message:
  -----------
  nbd/client: Don't hard-disconnect on ESHUTDOWN from server

The NBD spec says that a server may fail any transmission request
with ESHUTDOWN when it is apparent that no further request from
the client can be successfully honored.  The client is supposed
to then initiate a soft shutdown (wait for all remaining in-flight
requests to be answered, then send NBD_CMD_DISC).  However, since
qemu's server never uses ESHUTDOWN errors, this code was mostly
untested since its introduction in commit b6f5d3b5.

More recently, I learned that nbdkit as the NBD server is able to
send ESHUTDOWN errors, so I finally tested this code, and noticed
that our client was special-casing ESHUTDOWN to cause a hard
shutdown (immediate disconnect, with no NBD_CMD_DISC), but only
if the server sends this error as a simple reply.  Further
investigation found that commit d2febedb introduced a regression
where structured replies behave differently than simple replies -
but that the structured reply behavior is more in line with the
spec (even if we still lack code in nbd-client.c to properly quit
sending further requests).  So this patch reverts the portion of
b6f5d3b5 that introduced an improper hard-disconnect special-case
at the lower level, and leaves the future enhancement of a nicer
soft-disconnect at the higher level for another day.

CC: address@hidden
Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: fed5f8f82056c9f222433c41aeb9fca50c89f297
      
https://github.com/qemu/qemu/commit/fed5f8f82056c9f222433c41aeb9fca50c89f297
  Author: Eric Blake <address@hidden>
  Date:   2017-11-17 (Fri, 17 Nov 2017)

  Changed paths:
    M nbd/server.c

  Log Message:
  -----------
  nbd/server: Fix error reporting for bad requests

The NBD spec says an attempt to NBD_CMD_TRIM on a read-only
export should fail with EPERM, as a trim has the potential
to change disk contents, but we were relying on the block
layer to catch that for us, which might not always give the
right error (and even if it does, it does not let us pass
back a sane message for structured replies).

The NBD spec says an attempt to NBD_CMD_WRITE_ZEROES out of
bounds should fail with ENOSPC, not EINVAL.

Our check for u64 offset + u32 length wraparound up front is
pointless; nothing uses offset until after the second round
of sanity checks, and we can just as easily ensure there is
no wraparound by checking whether offset is in bounds (since
a disk size cannot exceed off_t which is 63 bits, adding a
32-bit number for a valid offset can't overflow).  Bonus:
dropping the up-front check lets us keep the connection alive
after NBD_CMD_WRITE, whereas before we would drop the
connection (of course, any client sending a packet that would
trigger the failure is already buggy, so it's also okay to
drop the connection, but better quality-of-implementation
never hurts).

Solve all of these issues by some code motion and improved
request validation.

Signed-off-by: Eric Blake <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


  Commit: 085ee6d282d38b430c850900c051e6b9e8c1681f
      
https://github.com/qemu/qemu/commit/085ee6d282d38b430c850900c051e6b9e8c1681f
  Author: Peter Maydell <address@hidden>
  Date:   2017-11-17 (Fri, 17 Nov 2017)

  Changed paths:
    M block/nbd-client.c
    M nbd/client.c
    M nbd/server.c

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2017-11-17' into 
staging

nbd patches for 2017-11-17

Eric Blake - nbd: Don't crash when server reports NBD_CMD_READ failure
Eric Blake - nbd/client: Use error_prepend() correctly
Eric Blake - nbd/client: Don't hard-disconnect on ESHUTDOWN from server
Eric Blake - nbd/server: Fix error reporting for bad requests

# gpg: Signature made Fri 17 Nov 2017 14:53:30 GMT
# gpg:                using RSA key 0xA7A16B4A2527436A
# gpg: Good signature from "Eric Blake <address@hidden>"
# gpg:                 aka "Eric Blake (Free Software Programmer) 
<address@hidden>"
# gpg:                 aka "[jpeg image of size 6874]"
# Primary key fingerprint: 71C2 CC22 B1C4 6029 27D2  F3AA A7A1 6B4A 2527 436A

* remotes/ericb/tags/pull-nbd-2017-11-17:
  nbd/server: Fix error reporting for bad requests
  nbd/client: Don't hard-disconnect on ESHUTDOWN from server
  nbd/client: Use error_prepend() correctly
  nbd: Don't crash when server reports NBD_CMD_READ failure

Signed-off-by: Peter Maydell <address@hidden>


Compare: https://github.com/qemu/qemu/compare/fec035a53fa1...085ee6d282d3