[Qemu-devel] [PATCH 1/2] nbd: Don't export a block device with no medium.

[Qemu-devel] [PATCH 1/2] nbd: Don't export a block device with no medium.

[Hani Benhabiles]

The device is exported with erroneous values and can't be read.

Before the patch:
$ sudo nbd-client localhost -p 10809 /dev/nbd0 -name floppy0
Negotiation: ..size = 17592186044415MB
bs=1024, sz=18446744073709547520 bytes

$ sudo mount /dev/nbd0 /mnt/tmp/
mount: block device /dev/nbd0 is write-protected, mounting read-only
mount: /dev/nbd0: can't read superblock

After the patch:
(qemu) nbd_server_add ide0-hd0
(qemu) nbd_server_add floppy0
Device 'floppy0' has no medium

Signed-off-by: Hani Benhabiles <hani@linux.com>
---
 blockdev-nbd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 922cf56..a700d52 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -91,6 +91,10 @@ void qmp_nbd_server_add(const char *device, bool has_writable, bool writable,
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return;
+    }
 
     if (!has_writable) {
         writable = false;
-- 
1.8.3.2

[Qemu-devel] [PATCH 2/2] nbd: Don't validate from and len in NBD_CMD_DISC.

[Hani Benhabiles]

These values aren't used in this case.

Currently, the from field in the request sent by the nbd kernel module leading
to a false error message when ending the connection with the client.

$ qemu-nbd some.img -v
// After nbd-client -d /dev/nbd0
nbd.c:nbd_trip():L1031: From: 18446744073709551104, Len: 0, Size: 20971520,
Offset: 0
nbd.c:nbd_trip():L1032: requested operation past EOF--bad client?
nbd.c:nbd_receive_request():L638: read failed

Signed-off-by: Hani Benhabiles <hani@linux.com>
---
 nbd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/nbd.c b/nbd.c
index e5084b6..dc076d7 100644
--- a/nbd.c
+++ b/nbd.c
@@ -1001,6 +1001,7 @@ static void nbd_trip(void *opaque)
     struct nbd_request request;
     struct nbd_reply reply;
     ssize_t ret;
+    uint32_t type;
 
     TRACE("Reading request.");
     if (client->closing) {
@@ -1023,8 +1024,8 @@ static void nbd_trip(void *opaque)
         reply.error = -ret;
         goto error_reply;
     }
-
-    if ((request.from + request.len) > exp->size) {
+    type = request.type & NBD_CMD_MASK_COMMAND;
+    if (type != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
             LOG("From: %" PRIu64 ", Len: %u, Size: %" PRIu64
             ", Offset: %" PRIu64 "\n",
                     request.from, request.len,
@@ -1033,7 +1034,7 @@ static void nbd_trip(void *opaque)
         goto invalid_request;
     }
 
-    switch (request.type & NBD_CMD_MASK_COMMAND) {
+    switch (type) {
     case NBD_CMD_READ:
         TRACE("Request type is READ");
 
-- 
1.8.3.2

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/2] nbd: Don't export a block device with no medium.

[Michael Tokarev]

18.05.2014 14:50, Hani Benhabiles wrote:
> The device is exported with erroneous values and can't be read.
> 
> Before the patch:
> $ sudo nbd-client localhost -p 10809 /dev/nbd0 -name floppy0
> Negotiation: ..size = 17592186044415MB
> bs=1024, sz=18446744073709547520 bytes
> 
> $ sudo mount /dev/nbd0 /mnt/tmp/
> mount: block device /dev/nbd0 is write-protected, mounting read-only
> mount: /dev/nbd0: can't read superblock
> 
> After the patch:
> (qemu) nbd_server_add ide0-hd0
> (qemu) nbd_server_add floppy0
> Device 'floppy0' has no medium

I'm not sure this is the right direction.  Maybe we should actually
allow exporting devices with no medium to be able to insert medium
later without re-starting the server?  Especially with -t (persistent).
I dunno.  At least this is the decision which should not be left to
a -trivial tree.

Thank you!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/2] nbd: Don't export a block device with no medium.

[Paolo Bonzini]

Il 19/05/2014 10:00, Michael Tokarev ha scritto:
> 18.05.2014 14:50, Hani Benhabiles wrote:
>> The device is exported with erroneous values and can't be read.
>>
>> Before the patch:
>> $ sudo nbd-client localhost -p 10809 /dev/nbd0 -name floppy0
>> Negotiation: ..size = 17592186044415MB
>> bs=1024, sz=18446744073709547520 bytes
>>
>> $ sudo mount /dev/nbd0 /mnt/tmp/
>> mount: block device /dev/nbd0 is write-protected, mounting read-only
>> mount: /dev/nbd0: can't read superblock
>>
>> After the patch:
>> (qemu) nbd_server_add ide0-hd0
>> (qemu) nbd_server_add floppy0
>> Device 'floppy0' has no medium
>
> I'm not sure this is the right direction.  Maybe we should actually
> allow exporting devices with no medium to be able to insert medium
> later without re-starting the server?  Especially with -t (persistent).
> I dunno.  At least this is the decision which should not be left to
> a -trivial tree.

The protocol has no support for marking a device as no-medium, or for 
reporting a medium change, so there's nothing sane that we can do. 
(Upon a medium change, for example, the embedded NBD server will shut 
down all connections).

In particular, for qemu-nbd there is no support for named exports, so 
even with "-t" it makes sense to just exit with an error condition and 
ask the user to reinvoke qemu-nbd later.

I'll revive the nbd-next branch for these two patches.  Thanks Michael 
for carrying the other two.

Paolo

Re: [Qemu-devel] [PATCH 2/2] nbd: Don't validate from and len in NBD_CMD_DISC.

[Paolo Bonzini]

Il 18/05/2014 12:50, Hani Benhabiles ha scritto:
> These values aren't used in this case.
>
> Currently, the from field in the request sent by the nbd kernel module leading
> to a false error message when ending the connection with the client.
>
> $ qemu-nbd some.img -v
> // After nbd-client -d /dev/nbd0
> nbd.c:nbd_trip():L1031: From: 18446744073709551104, Len: 0, Size: 20971520,
> Offset: 0
> nbd.c:nbd_trip():L1032: requested operation past EOF--bad client?
> nbd.c:nbd_receive_request():L638: read failed
>
> Signed-off-by: Hani Benhabiles <hani@linux.com>
> ---
>  nbd.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/nbd.c b/nbd.c
> index e5084b6..dc076d7 100644
> --- a/nbd.c
> +++ b/nbd.c
> @@ -1001,6 +1001,7 @@ static void nbd_trip(void *opaque)
>      struct nbd_request request;
>      struct nbd_reply reply;
>      ssize_t ret;
> +    uint32_t type;
>
>      TRACE("Reading request.");
>      if (client->closing) {
> @@ -1023,8 +1024,8 @@ static void nbd_trip(void *opaque)
>          reply.error = -ret;
>          goto error_reply;
>      }
> -
> -    if ((request.from + request.len) > exp->size) {
> +    type = request.type & NBD_CMD_MASK_COMMAND;
> +    if (type != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
>              LOG("From: %" PRIu64 ", Len: %u, Size: %" PRIu64
>              ", Offset: %" PRIu64 "\n",
>                      request.from, request.len,
> @@ -1033,7 +1034,7 @@ static void nbd_trip(void *opaque)
>          goto invalid_request;
>      }
>
> -    switch (request.type & NBD_CMD_MASK_COMMAND) {
> +    switch (type) {
>      case NBD_CMD_READ:
>          TRACE("Request type is READ");
>
>

Applied after renaming the variable from type to command (for 
consistency with e.g. nbd_co_receive_request).

Paolo

Re: [Qemu-devel] [PATCH 2/2] nbd: Don't validate from and len in NBD_CMD_DISC.

[Hani Benhabiles]

On Mon, May 19, 2014 at 01:05:12PM +0200, Paolo Bonzini wrote:
> Il 18/05/2014 12:50, Hani Benhabiles ha scritto:
> >These values aren't used in this case.
> >
> >Currently, the from field in the request sent by the nbd kernel module leading
> >to a false error message when ending the connection with the client.
> >
> >$ qemu-nbd some.img -v
> >// After nbd-client -d /dev/nbd0
> >nbd.c:nbd_trip():L1031: From: 18446744073709551104, Len: 0, Size: 20971520,
> >Offset: 0
> >nbd.c:nbd_trip():L1032: requested operation past EOF--bad client?
> >nbd.c:nbd_receive_request():L638: read failed
> >
> >Signed-off-by: Hani Benhabiles <hani@linux.com>
> >---
> > nbd.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> >diff --git a/nbd.c b/nbd.c
> >index e5084b6..dc076d7 100644
> >--- a/nbd.c
> >+++ b/nbd.c
> >@@ -1001,6 +1001,7 @@ static void nbd_trip(void *opaque)
> >     struct nbd_request request;
> >     struct nbd_reply reply;
> >     ssize_t ret;
> >+    uint32_t type;
> >
> >     TRACE("Reading request.");
> >     if (client->closing) {
> >@@ -1023,8 +1024,8 @@ static void nbd_trip(void *opaque)
> >         reply.error = -ret;
> >         goto error_reply;
> >     }
> >-
> >-    if ((request.from + request.len) > exp->size) {
> >+    type = request.type & NBD_CMD_MASK_COMMAND;
> >+    if (type != NBD_CMD_DISC && (request.from + request.len) > exp->size) {
> >             LOG("From: %" PRIu64 ", Len: %u, Size: %" PRIu64
> >             ", Offset: %" PRIu64 "\n",
> >                     request.from, request.len,
> >@@ -1033,7 +1034,7 @@ static void nbd_trip(void *opaque)
> >         goto invalid_request;
> >     }
> >
> >-    switch (request.type & NBD_CMD_MASK_COMMAND) {
> >+    switch (type) {
> >     case NBD_CMD_READ:
> >         TRACE("Request type is READ");
> >
> >
> 
> Applied after renaming the variable from type to command (for consistency
> with e.g. nbd_co_receive_request).

No issue. Thanks!