4:37 p.m.
In implementing my proof-of-concept 64-bit headers, I found the
following spec changes to be independent enough to post for review
now.
Eric Blake (2):
spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
spec: Tweak description of maximum block size
doc/proto.md | 87 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 53 insertions(+), 34 deletions(-)
--
2.35.1
4:37 p.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
The spec was silent on how many extents a server could reply with.
However, both qemu and nbdkit (the two server implementations known to
have implemented the NBD_CMD_BLOCK_STATUS extension) implement a hard
cap, and will truncate the amount of extents in a reply to avoid
sending a client a reply larger than the maximum NBD_CMD_READ response
they are willing to tolerate:
When qemu first implemented NBD_CMD_BLOCK_STATUS for the
base:allocation context (qemu commit e7b1948d51, Mar 2018), it behaved
as if NBD_CMD_FLAG_REQ_ONE were always passed by the client, and never
responded with more than one extent. Later, when adding its
qemu:dirty-bitmap:XXX context extension (qemu commit 3d068aff16, Jun
2018), it added a cap to 128k extents (1M+4 bytes), and that cap was
applied to base:allocation once qemu started sending multiple extents
for that context as well (qemu commit fb7afc797e, Jul 2018). Qemu
extents are never smaller than 512 bytes (other than an exception at
the end of a file whose size is not aligned to 512), but even so, a
request for just under 4G of block status could produce 8M extents,
resulting in a reply of 64M if it were not capped smaller.
When nbdkit first implemented NBD_CMD_BLOCK_STATUS (nbdkit 4ca66f70a5,
Mar 2019), it did not impose any restriction on the number of extents
in the reply chunk. But because it allows extents as small as one
byte, it is easy to write a server that can amplify a client's request
of status over 1M of the image into a reply over 8M in size, and it
was very easy to demonstrate that a hard cap was needed to avoid
crashing clients or otherwise killing the connection (a bad server
impacting the client negatively); unique to nbdkit's situation is the
fact that because it is designed for plugin server implementations,
not capping the length of extent also posed a problem to nbdkit as the
server (a client requesting large block status could cause the server
to run out of memory depending on the plugin providing the server
callbacks). So nbdkit enforced a bound of 1M extents (8M+4 bytes,
nbdkit commit 6e0dc839ea, Jun 2019).
Since the limit chosen by these two implementations is different, and
since nbdkit has versions that were not limited, add this as a SHOULD
NOT instead of MUST NOT constraint on servers implementing block
status. It does not matter that qemu picked a smaller limit that it
truncates to, since we have already documented that the server may
truncate for other reasons (such as it being inefficient to collect
that many extents in the first place). But documenting the limit now
becomes even more important in the face of a future addition of 64-bit
requests, where a client's request is no longer bounded to 4G and
could thereby produce even more than 8M extents for the corner case
when every 512 bytes is a new extent, if it were not for this
recommendation.
---
doc/proto.md | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/doc/proto.md b/doc/proto.md
index bacccfa..c3c7cd9 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -1815,6 +1815,12 @@ MUST initiate a hard disconnect.
the different contexts need not have the same number of extents or
cumulative extent length.
+ Servers SHOULD NOT send more than 2^20 extents in a single reply
+ chunk; in other words, the maximum size of
+ `NBD_REPLY_TYPE_BLOCK_STATUS` should not be more than 4 + 8*2^20
+ (8,388,612 bytes), even if this requires that the server truncate
+ the response in relation to the *length* requested by the client.
+
Even if the client did not use the `NBD_CMD_FLAG_REQ_ONE` flag in
its request, the server MAY return fewer descriptors in the reply
than would be required to fully specify the whole range of requested
@@ -2181,10 +2187,13 @@ The following request types exist:
multiple descriptors, and the final descriptor MAY extend beyond
the original requested size if the server can determine a larger
length without additional effort. On the other hand, the server MAY
- return less data than requested. However the server MUST return at
- least one status descriptor (and since each status descriptor has
- a non-zero length, a client can always make progress on a
- successful return). The server SHOULD use different *status*
+ return less data than requested. In particular, a server SHOULD NOT
+ send more than 2^20 status descriptors in a single chunk.
+
+ However the server MUST return at least one status descriptor,
+ and since each status descriptor has a non-zero length, a client
+ can always make progress on a successful return. The server SHOULD
+ use different *status*
values between consecutive descriptors where feasible, although
the client SHOULD be prepared to handle consecutive descriptors
with the same *status* value. The server SHOULD use descriptor
--
2.35.1
5:14 p.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
08.04.2022 00:37, Eric Blake wrote:
The spec was silent on how many extents a server could reply with.
However, both qemu and nbdkit (the two server implementations known to
have implemented the NBD_CMD_BLOCK_STATUS extension) implement a hard
cap, and will truncate the amount of extents in a reply to avoid
sending a client a reply larger than the maximum NBD_CMD_READ response
they are willing to tolerate:
When qemu first implemented NBD_CMD_BLOCK_STATUS for the
base:allocation context (qemu commit e7b1948d51, Mar 2018), it behaved
as if NBD_CMD_FLAG_REQ_ONE were always passed by the client, and never
responded with more than one extent. Later, when adding its
qemu:dirty-bitmap:XXX context extension (qemu commit 3d068aff16, Jun
2018), it added a cap to 128k extents (1M+4 bytes), and that cap was
applied to base:allocation once qemu started sending multiple extents
for that context as well (qemu commit fb7afc797e, Jul 2018). Qemu
extents are never smaller than 512 bytes (other than an exception at
the end of a file whose size is not aligned to 512), but even so, a
request for just under 4G of block status could produce 8M extents,
resulting in a reply of 64M if it were not capped smaller.
When nbdkit first implemented NBD_CMD_BLOCK_STATUS (nbdkit 4ca66f70a5,
Mar 2019), it did not impose any restriction on the number of extents
in the reply chunk. But because it allows extents as small as one
byte, it is easy to write a server that can amplify a client's request
of status over 1M of the image into a reply over 8M in size, and it
was very easy to demonstrate that a hard cap was needed to avoid
crashing clients or otherwise killing the connection (a bad server
impacting the client negatively); unique to nbdkit's situation is the
fact that because it is designed for plugin server implementations,
not capping the length of extent also posed a problem to nbdkit as the
server (a client requesting large block status could cause the server
to run out of memory depending on the plugin providing the server
callbacks). So nbdkit enforced a bound of 1M extents (8M+4 bytes,
nbdkit commit 6e0dc839ea, Jun 2019).
Since the limit chosen by these two implementations is different, and
since nbdkit has versions that were not limited, add this as a SHOULD
NOT instead of MUST NOT constraint on servers implementing block
status. It does not matter that qemu picked a smaller limit that it
truncates to, since we have already documented that the server may
truncate for other reasons (such as it being inefficient to collect
that many extents in the first place). But documenting the limit now
becomes even more important in the face of a future addition of 64-bit
requests, where a client's request is no longer bounded to 4G and
could thereby produce even more than 8M extents for the corner case
when every 512 bytes is a new extent, if it were not for this
recommendation.
---
doc/proto.md | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/doc/proto.md b/doc/proto.md
index bacccfa..c3c7cd9 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -1815,6 +1815,12 @@ MUST initiate a hard disconnect.
the different contexts need not have the same number of extents or
cumulative extent length.
+ Servers SHOULD NOT send more than 2^20 extents in a single reply
+ chunk; in other words, the maximum size of
+ `NBD_REPLY_TYPE_BLOCK_STATUS` should not be more than 4 + 8*2^20
"the maximum size .. should not be more" sounds a bit strange. Maybe just
"the size .. should not be more" ?
+ (8,388,612 bytes), even if this requires that the server truncate
+ the response in relation to the *length* requested by the client.
+
Even if the client did not use the `NBD_CMD_FLAG_REQ_ONE` flag in
its request, the server MAY return fewer descriptors in the reply
than would be required to fully specify the whole range of requested
@@ -2181,10 +2187,13 @@ The following request types exist:
multiple descriptors, and the final descriptor MAY extend beyond
the original requested size if the server can determine a larger
length without additional effort. On the other hand, the server MAY
- return less data than requested. However the server MUST return at
- least one status descriptor (and since each status descriptor has
- a non-zero length, a client can always make progress on a
- successful return). The server SHOULD use different *status*
+ return less data than requested. In particular, a server SHOULD NOT
+ send more than 2^20 status descriptors in a single chunk.
+
+ However the server MUST return at least one status descriptor,
+ and since each status descriptor has a non-zero length, a client
+ can always make progress on a successful return. The server SHOULD
+ use different *status*
values between consecutive descriptors where feasible, although
the client SHOULD be prepared to handle consecutive descriptors
with the same *status* value. The server SHOULD use descriptor
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)openvz.org>
--
Best regards,
Vladimir
2:25 a.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
Hi Eric,
On Thu, Apr 07, 2022 at 04:37:19PM -0500, Eric Blake wrote:
The spec was silent on how many extents a server could reply with.
However, both qemu and nbdkit (the two server implementations known to
have implemented the NBD_CMD_BLOCK_STATUS extension) implement a hard
cap, and will truncate the amount of extents in a reply to avoid
sending a client a reply larger than the maximum NBD_CMD_READ response
they are willing to tolerate:
When qemu first implemented NBD_CMD_BLOCK_STATUS for the
base:allocation context (qemu commit e7b1948d51, Mar 2018), it behaved
as if NBD_CMD_FLAG_REQ_ONE were always passed by the client, and never
responded with more than one extent. Later, when adding its
qemu:dirty-bitmap:XXX context extension (qemu commit 3d068aff16, Jun
2018), it added a cap to 128k extents (1M+4 bytes), and that cap was
applied to base:allocation once qemu started sending multiple extents
for that context as well (qemu commit fb7afc797e, Jul 2018). Qemu
extents are never smaller than 512 bytes (other than an exception at
the end of a file whose size is not aligned to 512), but even so, a
request for just under 4G of block status could produce 8M extents,
resulting in a reply of 64M if it were not capped smaller.
When nbdkit first implemented NBD_CMD_BLOCK_STATUS (nbdkit 4ca66f70a5,
Mar 2019), it did not impose any restriction on the number of extents
in the reply chunk. But because it allows extents as small as one
byte, it is easy to write a server that can amplify a client's request
of status over 1M of the image into a reply over 8M in size, and it
was very easy to demonstrate that a hard cap was needed to avoid
crashing clients or otherwise killing the connection (a bad server
impacting the client negatively); unique to nbdkit's situation is the
fact that because it is designed for plugin server implementations,
not capping the length of extent also posed a problem to nbdkit as the
server (a client requesting large block status could cause the server
to run out of memory depending on the plugin providing the server
callbacks). So nbdkit enforced a bound of 1M extents (8M+4 bytes,
nbdkit commit 6e0dc839ea, Jun 2019).
Since the limit chosen by these two implementations is different, and
since nbdkit has versions that were not limited, add this as a SHOULD
NOT instead of MUST NOT constraint on servers implementing block
status. It does not matter that qemu picked a smaller limit that it
truncates to, since we have already documented that the server may
truncate for other reasons (such as it being inefficient to collect
that many extents in the first place). But documenting the limit now
becomes even more important in the face of a future addition of 64-bit
requests, where a client's request is no longer bounded to 4G and
could thereby produce even more than 8M extents for the corner case
when every 512 bytes is a new extent, if it were not for this
recommendation.
It feels backwards to me to make this a restriction on the server side.
You're saying there are server implementations that will be inefficient
if there are more than 2^20 extents, and therefore no server should send
more than those, even if it can do so efficiently.
Isn't it up to the server implementation to decide what can be done
efficiently?
Perhaps we can make the language about possibly reducing length of
extens a bit stronger; but I don't think adding explicit limits for a
server's own protection is necessary.
---
doc/proto.md | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/doc/proto.md b/doc/proto.md
index bacccfa..c3c7cd9 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -1815,6 +1815,12 @@ MUST initiate a hard disconnect.
the different contexts need not have the same number of extents or
cumulative extent length.
+ Servers SHOULD NOT send more than 2^20 extents in a single reply
+ chunk; in other words, the maximum size of
+ `NBD_REPLY_TYPE_BLOCK_STATUS` should not be more than 4 + 8*2^20
+ (8,388,612 bytes), even if this requires that the server truncate
+ the response in relation to the *length* requested by the client.
+
Even if the client did not use the `NBD_CMD_FLAG_REQ_ONE` flag in
its request, the server MAY return fewer descriptors in the reply
than would be required to fully specify the whole range of requested
@@ -2181,10 +2187,13 @@ The following request types exist:
multiple descriptors, and the final descriptor MAY extend beyond
the original requested size if the server can determine a larger
length without additional effort. On the other hand, the server MAY
- return less data than requested. However the server MUST return at
- least one status descriptor (and since each status descriptor has
- a non-zero length, a client can always make progress on a
- successful return). The server SHOULD use different *status*
+ return less data than requested. In particular, a server SHOULD NOT
+ send more than 2^20 status descriptors in a single chunk.
+
+ However the server MUST return at least one status descriptor,
+ and since each status descriptor has a non-zero length, a client
+ can always make progress on a successful return. The server SHOULD
+ use different *status*
values between consecutive descriptors where feasible, although
the client SHOULD be prepared to handle consecutive descriptors
with the same *status* value. The server SHOULD use descriptor
--
2.35.1
--
w(a)uter.{be,co.za}
wouter(a){grep.be,fosdem.org,debian.org}
6:52 a.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Apr 08, 2022 at 09:25:01AM +0200, Wouter Verhelst wrote:
Hi Eric,
On Thu, Apr 07, 2022 at 04:37:19PM -0500, Eric Blake wrote:
> The spec was silent on how many extents a server could reply with.
> However, both qemu and nbdkit (the two server implementations known to
> have implemented the NBD_CMD_BLOCK_STATUS extension) implement a hard
> cap, and will truncate the amount of extents in a reply to avoid
> sending a client a reply larger than the maximum NBD_CMD_READ response
> they are willing to tolerate:
>
> When qemu first implemented NBD_CMD_BLOCK_STATUS for the
> base:allocation context (qemu commit e7b1948d51, Mar 2018), it behaved
> as if NBD_CMD_FLAG_REQ_ONE were always passed by the client, and never
> responded with more than one extent. Later, when adding its
> qemu:dirty-bitmap:XXX context extension (qemu commit 3d068aff16, Jun
> 2018), it added a cap to 128k extents (1M+4 bytes), and that cap was
> applied to base:allocation once qemu started sending multiple extents
> for that context as well (qemu commit fb7afc797e, Jul 2018). Qemu
> extents are never smaller than 512 bytes (other than an exception at
> the end of a file whose size is not aligned to 512), but even so, a
> request for just under 4G of block status could produce 8M extents,
> resulting in a reply of 64M if it were not capped smaller.
>
> When nbdkit first implemented NBD_CMD_BLOCK_STATUS (nbdkit 4ca66f70a5,
> Mar 2019), it did not impose any restriction on the number of extents
> in the reply chunk. But because it allows extents as small as one
> byte, it is easy to write a server that can amplify a client's request
> of status over 1M of the image into a reply over 8M in size, and it
> was very easy to demonstrate that a hard cap was needed to avoid
> crashing clients or otherwise killing the connection (a bad server
> impacting the client negatively); unique to nbdkit's situation is the
> fact that because it is designed for plugin server implementations,
> not capping the length of extent also posed a problem to nbdkit as the
> server (a client requesting large block status could cause the server
> to run out of memory depending on the plugin providing the server
> callbacks). So nbdkit enforced a bound of 1M extents (8M+4 bytes,
> nbdkit commit 6e0dc839ea, Jun 2019).
>
> Since the limit chosen by these two implementations is different, and
> since nbdkit has versions that were not limited, add this as a SHOULD
> NOT instead of MUST NOT constraint on servers implementing block
> status. It does not matter that qemu picked a smaller limit that it
> truncates to, since we have already documented that the server may
> truncate for other reasons (such as it being inefficient to collect
> that many extents in the first place). But documenting the limit now
> becomes even more important in the face of a future addition of 64-bit
> requests, where a client's request is no longer bounded to 4G and
> could thereby produce even more than 8M extents for the corner case
> when every 512 bytes is a new extent, if it were not for this
> recommendation.
It feels backwards to me to make this a restriction on the server side.
You're saying there are server implementations that will be inefficient
if there are more than 2^20 extents, and therefore no server should send
more than those, even if it can do so efficiently.
Isn't it up to the server implementation to decide what can be done
efficiently?
Perhaps we can make the language about possibly reducing length of
extens a bit stronger; but I don't think adding explicit limits for a
server's own protection is necessary.
I agree, but for a different reason.
I think Eric should add language that servers can consider limiting
response sizes in order to prevent possible amplification issues
and/or simply overwhelming the client with work (bad server DoS
attacks against clients are a thing!), but I don't think it's
necessarily a "SHOULD" issue.
BTW attached is an nbdkit plugin that creates an NBD server that
responds with massive numbers of byte-granularity extents, in case
anyone wants to test how nbdkit and/or clients respond:
$ chmod +x /var/tmp/lots-of-extents.py
$ /var/tmp/lots-of-extents.py -f
$ nbdinfo --map nbd://localhost | head
0 1 3 hole,zero
1 1 0 data
2 1 3 hole,zero
3 1 0 data
4 1 3 hole,zero
5 1 0 data
6 1 3 hole,zero
7 1 0 data
8 1 3 hole,zero
9 1 0 data
$ nbdinfo --map --totals nbd://localhost
524288 50.0% 0 data
524288 50.0% 3 hole,zero
From experimentation I found this really hurts my laptop :-(
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
7:09 a.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Apr 08, 2022 at 12:52:44PM +0100, Richard W.M. Jones wrote:
From experimentation I found this really hurts my laptop :-(
... because of a memory leak in the nbdkit Python bindings as it
turned out. This function:
def extents(h, count, offset, flags):
e = [(i, 1, typ(i)) for i in range(offset, offset + count + 1)]
return e
returns tuples which never got dereferenced in the C part.
Fixed in:
https://gitlab.com/nbdkit/nbdkit/-/commit/4ede3d7b0778cc1fe661307d2289fad...
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
8:48 a.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Apr 8, 2022 at 2:52 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:
On Fri, Apr 08, 2022 at 09:25:01AM +0200, Wouter Verhelst wrote:
> Hi Eric,
>
> On Thu, Apr 07, 2022 at 04:37:19PM -0500, Eric Blake wrote:
> > The spec was silent on how many extents a server could reply with.
> > However, both qemu and nbdkit (the two server implementations known to
> > have implemented the NBD_CMD_BLOCK_STATUS extension) implement a hard
> > cap, and will truncate the amount of extents in a reply to avoid
> > sending a client a reply larger than the maximum NBD_CMD_READ response
> > they are willing to tolerate:
> >
> > When qemu first implemented NBD_CMD_BLOCK_STATUS for the
> > base:allocation context (qemu commit e7b1948d51, Mar 2018), it behaved
> > as if NBD_CMD_FLAG_REQ_ONE were always passed by the client, and never
> > responded with more than one extent. Later, when adding its
> > qemu:dirty-bitmap:XXX context extension (qemu commit 3d068aff16, Jun
> > 2018), it added a cap to 128k extents (1M+4 bytes), and that cap was
> > applied to base:allocation once qemu started sending multiple extents
> > for that context as well (qemu commit fb7afc797e, Jul 2018). Qemu
> > extents are never smaller than 512 bytes (other than an exception at
> > the end of a file whose size is not aligned to 512), but even so, a
> > request for just under 4G of block status could produce 8M extents,
> > resulting in a reply of 64M if it were not capped smaller.
> >
> > When nbdkit first implemented NBD_CMD_BLOCK_STATUS (nbdkit 4ca66f70a5,
> > Mar 2019), it did not impose any restriction on the number of extents
> > in the reply chunk. But because it allows extents as small as one
> > byte, it is easy to write a server that can amplify a client's request
> > of status over 1M of the image into a reply over 8M in size, and it
> > was very easy to demonstrate that a hard cap was needed to avoid
> > crashing clients or otherwise killing the connection (a bad server
> > impacting the client negatively); unique to nbdkit's situation is the
> > fact that because it is designed for plugin server implementations,
> > not capping the length of extent also posed a problem to nbdkit as the
> > server (a client requesting large block status could cause the server
> > to run out of memory depending on the plugin providing the server
> > callbacks). So nbdkit enforced a bound of 1M extents (8M+4 bytes,
> > nbdkit commit 6e0dc839ea, Jun 2019).
> >
> > Since the limit chosen by these two implementations is different, and
> > since nbdkit has versions that were not limited, add this as a SHOULD
> > NOT instead of MUST NOT constraint on servers implementing block
> > status. It does not matter that qemu picked a smaller limit that it
> > truncates to, since we have already documented that the server may
> > truncate for other reasons (such as it being inefficient to collect
> > that many extents in the first place). But documenting the limit now
> > becomes even more important in the face of a future addition of 64-bit
> > requests, where a client's request is no longer bounded to 4G and
> > could thereby produce even more than 8M extents for the corner case
> > when every 512 bytes is a new extent, if it were not for this
> > recommendation.
>
> It feels backwards to me to make this a restriction on the server side.
> You're saying there are server implementations that will be inefficient
> if there are more than 2^20 extents, and therefore no server should send
> more than those, even if it can do so efficiently.
>
> Isn't it up to the server implementation to decide what can be done
> efficiently?
>
> Perhaps we can make the language about possibly reducing length of
> extens a bit stronger; but I don't think adding explicit limits for a
> server's own protection is necessary.
I agree, but for a different reason.
I think Eric should add language that servers can consider limiting
response sizes in order to prevent possible amplification issues
and/or simply overwhelming the client with work (bad server DoS
attacks against clients are a thing!), but I don't think it's
necessarily a "SHOULD" issue.
BTW attached is an nbdkit plugin that creates an NBD server that
responds with massive numbers of byte-granularity extents, in case
anyone wants to test how nbdkit and/or clients respond:
$ chmod +x /var/tmp/lots-of-extents.py
$ /var/tmp/lots-of-extents.py -f
$ nbdinfo --map nbd://localhost | head
0 1 3 hole,zero
1 1 0 data
2 1 3 hole,zero
3 1 0 data
4 1 3 hole,zero
5 1 0 data
6 1 3 hole,zero
7 1 0 data
8 1 3 hole,zero
9 1 0 data
$ nbdinfo --map --totals nbd://localhost
524288 50.0% 0 data
524288 50.0% 3 hole,zero
This is a malicious server. A good client will drop the connection when
receiving the first 1 byte chunk.
The real issue here is not enforcing or suggesting a limit on the number of
extent the server returns, but enforcing a limit on the minimum size of
a chunk.
Since this is the network *block device* protocol it should not allow chunks
smaller than the device block size, so anything smaller than 512 bytes
should be invalid response from the server.
Even the last chunk should not be smaller than 512 bytes. The fact that you
can serve a file with size that is not aligned to 512 bytes does not mean that
the export size can be unaligned to the logical block size. There are no real
block devices that have such alignment so the protocol should not allow this.
A good server will round the file size down the logical block size to avoid this
issue.
How about letting the client set a minimum size of a chunk? This way we
avoid the issue of limiting the number of chunks. Merging small chunks
is best done on the server side instead of wasting bandwidth and doing
this on the client side.
Nir
9:01 a.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Apr 08, 2022 at 04:48:59PM +0300, Nir Soffer wrote:
This is a malicious server. A good client will drop the connection
when
receiving the first 1 byte chunk.
The real issue here is not enforcing or suggesting a limit on the number of
extent the server returns, but enforcing a limit on the minimum size of
a chunk.
Since this is the network *block device* protocol it should not allow chunks
smaller than the device block size, so anything smaller than 512 bytes
should be invalid response from the server.
Even the last chunk should not be smaller than 512 bytes. The fact that you
can serve a file with size that is not aligned to 512 bytes does not mean that
the export size can be unaligned to the logical block size. There are no real
block devices that have such alignment so the protocol should not allow this.
A good server will round the file size down the logical block size to avoid this
issue.
How about letting the client set a minimum size of a chunk? This way we
avoid the issue of limiting the number of chunks. Merging small chunks
is best done on the server side instead of wasting bandwidth and doing
this on the client side.
While it's interesting to know if chunks should obey the
(server-specified) minimum block size, I don't think we should force
operations to only work on sector boundaries. That's a step
backwards. We've spent a long time and effort making nbdkit & NBD
work well for < 512 byte images, byte granularity tails, and disk
operations.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
10:46 a.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Apr 08, 2022 at 04:48:59PM +0300, Nir Soffer wrote:
On Fri, Apr 8, 2022 at 2:52 PM Richard W.M. Jones
<rjones(a)redhat.com> wrote:
>
> On Fri, Apr 08, 2022 at 09:25:01AM +0200, Wouter Verhelst wrote:
> > Hi Eric,
> >
> > On Thu, Apr 07, 2022 at 04:37:19PM -0500, Eric Blake wrote:
> > > The spec was silent on how many extents a server could reply with.
> > > However, both qemu and nbdkit (the two server implementations known to
> > > have implemented the NBD_CMD_BLOCK_STATUS extension) implement a hard
> > > cap, and will truncate the amount of extents in a reply to avoid
> > > sending a client a reply larger than the maximum NBD_CMD_READ response
> > > they are willing to tolerate:
> > >
> > > When qemu first implemented NBD_CMD_BLOCK_STATUS for the
> > > base:allocation context (qemu commit e7b1948d51, Mar 2018), it behaved
> > > as if NBD_CMD_FLAG_REQ_ONE were always passed by the client, and never
> > > responded with more than one extent. Later, when adding its
> > > qemu:dirty-bitmap:XXX context extension (qemu commit 3d068aff16, Jun
> > > 2018), it added a cap to 128k extents (1M+4 bytes), and that cap was
> > > applied to base:allocation once qemu started sending multiple extents
> > > for that context as well (qemu commit fb7afc797e, Jul 2018). Qemu
> > > extents are never smaller than 512 bytes (other than an exception at
> > > the end of a file whose size is not aligned to 512), but even so, a
> > > request for just under 4G of block status could produce 8M extents,
> > > resulting in a reply of 64M if it were not capped smaller.
Qemu as server imposed the limit because sending a 64M reply to a
block status request for 4G (where every 512 bytes alternates status)
would kill qemu as client (which refuses to accept responses from the
server larger than 32M). Nothing in here about considerations for the
server.
> > >
> > > When nbdkit first implemented NBD_CMD_BLOCK_STATUS (nbdkit 4ca66f70a5,
> > > Mar 2019), it did not impose any restriction on the number of extents
> > > in the reply chunk. But because it allows extents as small as one
> > > byte, it is easy to write a server that can amplify a client's
request
> > > of status over 1M of the image into a reply over 8M in size, and it
> > > was very easy to demonstrate that a hard cap was needed to avoid
> > > crashing clients or otherwise killing the connection (a bad server
> > > impacting the client negatively);
Likewise, THIS part of the commit message is about nbdkit limiting things to avoid killing
the client.
> > > unique to nbdkit's situation is the
> > > fact that because it is designed for plugin server implementations,
> > > not capping the length of extent also posed a problem to nbdkit as the
> > > server (a client requesting large block status could cause the server
> > > to run out of memory depending on the plugin providing the server
> > > callbacks). So nbdkit enforced a bound of 1M extents (8M+4 bytes,
> > > nbdkit commit 6e0dc839ea, Jun 2019).
While this part is an incidental anecdote (we also had problems with
clients being able to kill the server, when the server was poorly
written to consume too much memory by heavily alternating status), but
does NOT change the content of the text change in the patch.
> > >
> > > Since the limit chosen by these two implementations is different, and
> > > since nbdkit has versions that were not limited, add this as a SHOULD
> > > NOT instead of MUST NOT constraint on servers implementing block
> > > status. It does not matter that qemu picked a smaller limit that it
> > > truncates to, since we have already documented that the server may
> > > truncate for other reasons (such as it being inefficient to collect
> > > that many extents in the first place). But documenting the limit now
> > > becomes even more important in the face of a future addition of 64-bit
> > > requests, where a client's request is no longer bounded to 4G and
> > > could thereby produce even more than 8M extents for the corner case
> > > when every 512 bytes is a new extent, if it were not for this
> > > recommendation.
> >
> > It feels backwards to me to make this a restriction on the server side.
> > You're saying there are server implementations that will be inefficient
> > if there are more than 2^20 extents, and therefore no server should send
> > more than those, even if it can do so efficiently.
The limitation is on the server to avoid killing clients. Whether or
not the server happens to be inefficient if it could otherwise
generate more than 2^20 extents (as nbdkit was temporarily able to do)
is not mentioned in the spec change in the patch itself, but only in
the commit message. I can do a better job of marking the server
effect as an interesting side effect. Servers can ALWAYS choose to
truncate early to avoid exhausting their own resources; the important
part of this patch is that servers MUST truncate early to avoid
killing the client, even if the server has the resources to provide a
larger response up to the bounds of the client's request.
> >
> > Isn't it up to the server implementation to decide what can be done
> > efficiently?
> >
> > Perhaps we can make the language about possibly reducing length of
> > extens a bit stronger; but I don't think adding explicit limits for a
> > server's own protection is necessary.
>
> I agree, but for a different reason.
>
> I think Eric should add language that servers can consider limiting
> response sizes in order to prevent possible amplification issues
> and/or simply overwhelming the client with work (bad server DoS
> attacks against clients are a thing!), but I don't think it's
> necessarily a "SHOULD" issue.
On the contrary, allowing a server to send anything larger than 32M in
a reply chunk is GOING to kill clients (since many existing clients
treat server packets larger than 32M as a denial of service attack).
But the client has no control over how much the server will choose to
reply with, other than the client can limit its initial size hint to
something small enough that even if the server causes an 8-fold
increase in size of the reply by alternating status for every byte,
the server cannot send more than the number of extents requested by
the client. But forcing the client to limit their extent hints is
counterproductive, since MOST images don't have that many alternating
extents, and it causes more traffic overhead to break extent queries
into smaller chunks just to worry about corner cases.
>
> BTW attached is an nbdkit plugin that creates an NBD server that
> responds with massive numbers of byte-granularity extents, in case
> anyone wants to test how nbdkit and/or clients respond:
>
> $ chmod +x /var/tmp/lots-of-extents.py
> $ /var/tmp/lots-of-extents.py -f
>
> $ nbdinfo --map nbd://localhost | head
> 0 1 3 hole,zero
> 1 1 0 data
> 2 1 3 hole,zero
> 3 1 0 data
> 4 1 3 hole,zero
> 5 1 0 data
> 6 1 3 hole,zero
> 7 1 0 data
> 8 1 3 hole,zero
> 9 1 0 data
> $ nbdinfo --map --totals nbd://localhost
> 524288 50.0% 0 data
> 524288 50.0% 3 hole,zero
This is a malicious server. A good client will drop the connection when
receiving the first 1 byte chunk.
Depends on the server. Most servers don't serve 1-byte extents, and
the NBD spec even recommends that extents be at least 512 bytes in
size, and requires that extents be a multiple of any minimum block
size if one was advertised by the server.
But even though most servers don't have 1-byte extents does not mean
that the NBD protocol must forbid them.
The real issue here is not enforcing or suggesting a limit on the number of
extent the server returns, but enforcing a limit on the minimum size of
a chunk.
Since this is the network *block device* protocol it should not allow chunks
smaller than the device block size, so anything smaller than 512 bytes
should be invalid response from the server.
No, not an invalid response, but merely a discouraged one - and that
text is already present in the wording of NBD_CMD_BLOCK_STATUS.
Even the last chunk should not be smaller than 512 bytes. The fact that you
can serve a file with size that is not aligned to 512 bytes does not mean that
the export size can be unaligned to the logical block size. There are no real
block devices that have such alignment so the protocol should not allow this.
A good server will round the file size down the logical block size to avoid this
issue.
How about letting the client set a minimum size of a chunk? This way we
avoid the issue of limiting the number of chunks. Merging small chunks
is best done on the server side instead of wasting bandwidth and doing
this on the client side.
The client can't set the minimum block size, but the server can
certainly advertise one, and must obey that advertisement. Or are you
asking for a new extension where the client mandates what the minimum
granularity must be from the server in responses to NBD_CMD_READ and
NBD_CMD_BLOCK_STATUS, when the client wants a larger granularity than
what the server advertises? That's a different extension than this
patch, but may be worth considering.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
12:45 p.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Apr 8, 2022 at 6:47 PM Eric Blake <eblake(a)redhat.com> wrote:
On Fri, Apr 08, 2022 at 04:48:59PM +0300, Nir Soffer wrote:
...
> > BTW attached is an nbdkit plugin that creates an NBD server
that
> > responds with massive numbers of byte-granularity extents, in case
> > anyone wants to test how nbdkit and/or clients respond:
> >
> > $ chmod +x /var/tmp/lots-of-extents.py
> > $ /var/tmp/lots-of-extents.py -f
> >
> > $ nbdinfo --map nbd://localhost | head
> > 0 1 3 hole,zero
> > 1 1 0 data
> > 2 1 3 hole,zero
> > 3 1 0 data
> > 4 1 3 hole,zero
> > 5 1 0 data
> > 6 1 3 hole,zero
> > 7 1 0 data
> > 8 1 3 hole,zero
> > 9 1 0 data
> > $ nbdinfo --map --totals nbd://localhost
> > 524288 50.0% 0 data
> > 524288 50.0% 3 hole,zero
>
> This is a malicious server. A good client will drop the connection when
> receiving the first 1 byte chunk.
Depends on the server. Most servers don't serve 1-byte extents, and
the NBD spec even recommends that extents be at least 512 bytes in
size, and requires that extents be a multiple of any minimum block
size if one was advertised by the server.
But even though most servers don't have 1-byte extents does not mean
that the NBD protocol must forbid them.
Forbidding this simplifies clients without limiting real world use cases.
What is a reason to allow this?
> The real issue here is not enforcing or suggesting a limit on
the number of
> extent the server returns, but enforcing a limit on the minimum size of
> a chunk.
>
> Since this is the network *block device* protocol it should not allow chunks
> smaller than the device block size, so anything smaller than 512 bytes
> should be invalid response from the server.
No, not an invalid response, but merely a discouraged one - and that
text is already present in the wording of NBD_CMD_BLOCK_STATUS.
My suggestion is to make it an invalid response, because there are no block
devices that can return such a response.
> Even the last chunk should not be smaller than 512 bytes. The
fact that you
> can serve a file with size that is not aligned to 512 bytes does not mean that
> the export size can be unaligned to the logical block size. There are no real
> block devices that have such alignment so the protocol should not allow this.
> A good server will round the file size down the logical block size to avoid this
> issue.
>
> How about letting the client set a minimum size of a chunk? This way we
> avoid the issue of limiting the number of chunks. Merging small chunks
> is best done on the server side instead of wasting bandwidth and doing
> this on the client side.
The client can't set the minimum block size, but the server can
certainly advertise one, and must obey that advertisement. Or are you
asking for a new extension where the client mandates what the minimum
granularity must be from the server in responses to NBD_CMD_READ and
NBD_CMD_BLOCK_STATUS, when the client wants a larger granularity than
what the server advertises? That's a different extension than this
patch, but may be worth considering.
Yes, this should really be discussed in another thread.
Nir
1:05 p.m.
New subject: [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length
On Fri, Apr 08, 2022 at 08:45:56PM +0300, Nir Soffer wrote:
On Fri, Apr 8, 2022 at 6:47 PM Eric Blake <eblake(a)redhat.com>
wrote:
>
> On Fri, Apr 08, 2022 at 04:48:59PM +0300, Nir Soffer wrote:
...
> > > BTW attached is an nbdkit plugin that creates an NBD server that
> > > responds with massive numbers of byte-granularity extents, in case
> > > anyone wants to test how nbdkit and/or clients respond:
> > >
> > > $ chmod +x /var/tmp/lots-of-extents.py
> > > $ /var/tmp/lots-of-extents.py -f
> > >
> > > $ nbdinfo --map nbd://localhost | head
> > > 0 1 3 hole,zero
> > > 1 1 0 data
> > > 2 1 3 hole,zero
> > > 3 1 0 data
> > > 4 1 3 hole,zero
> > > 5 1 0 data
> > > 6 1 3 hole,zero
> > > 7 1 0 data
> > > 8 1 3 hole,zero
> > > 9 1 0 data
> > > $ nbdinfo --map --totals nbd://localhost
> > > 524288 50.0% 0 data
> > > 524288 50.0% 3 hole,zero
> >
> > This is a malicious server. A good client will drop the connection when
> > receiving the first 1 byte chunk.
>
> Depends on the server. Most servers don't serve 1-byte extents, and
> the NBD spec even recommends that extents be at least 512 bytes in
> size, and requires that extents be a multiple of any minimum block
> size if one was advertised by the server.
>
> But even though most servers don't have 1-byte extents does not mean
> that the NBD protocol must forbid them.
Forbidding this simplifies clients without limiting real world use cases.
I'm not even sure this is true. Clients are quite free to only make
requests on 512 byte block boundaries if they want, and refuse to deal
with servers which offer non-aligned disk sizes or extents.
If clients are doing this and still have problems they ought to be
fixed, although I don't know what those would be.
What is a reason to allow this?
Because you don't always want to serve things which are exactly disk
images, or which make assumptions related to modern PCs (256 byte
sectors were used into the 90s).
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
4:37 p.m.
New subject: [PATCH 2/2] spec: Tweak description of maximum block size
Commit 9f30fedb improved the spec to allow non-payload requests that
exceed any advertised maximum block size. Take this one step further
by permitting the server to use NBD_EOVERFLOW as a hint to the client
when a request is oversize (while permitting NBD_EINVAL for
back-compat), and by rewording the text to explicitly call out that
what is being advertised is the maximum payload length, not maximum
block size. This becomes more important when we add 64-bit
extensions, where it becomes possible to extend `NBD_CMD_BLOCK_STATUS`
to have both an effect length (how much of the image does the client
want status on) and a payload length (filtering the response to a
subset of negotiated metadata contexts).
---
doc/proto.md | 70 ++++++++++++++++++++++++++++++----------------------
1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/doc/proto.md b/doc/proto.md
index c3c7cd9..e332e21 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -769,8 +769,8 @@ learn the server's constraints without committing to them.
If block size constraints have not been advertised or agreed on
externally, then a server SHOULD support a default minimum block size
-of 1, a preferred block size of 2^12 (4,096), and a maximum block size
-that is effectively unlimited (0xffffffff, or the export size if that
+of 1, a preferred block size of 2^12 (4,096), and a maximum block payload
+size that is at least 2^25 (33,554,432) (or the export size if that
is smaller), while a client desiring maximum interoperability SHOULD
constrain its requests to a minimum block size of 2^9 (512), and limit
`NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum block size of
@@ -815,8 +815,8 @@ the preferred block size for that export. The server MAY advertise
an
export size that is not an integer multiple of the preferred block
size.
-The maximum block size represents the maximum length that the server
-is willing to handle in one request. If advertised, it MAY be
+The maximum block size represents the maximum payload length that the
+server is willing to handle in one request. If advertised, it MAY be
something other than a power of 2, but MUST be either an integer
multiple of the minimum block size or the value 0xffffffff for no
inherent limit, MUST be at least as large as the smaller of the
@@ -825,7 +825,20 @@ preferred block size or export size, and SHOULD be at least 2^20
MAY advertise a maximum block size that is larger than the export
size, although in that case, the client MUST treat the export size as
the effective maximum block size (as further constrained by a nonzero
-offset).
+offset). For commands that require a payload in either direction and
+where the client controls the payload length (`NBD_CMD_WRITE` or
+`NBD_CMD_READ`), the client MUST NOT use a length larger than the
+maximum block size. For replies where the payload length is controlled
+by the server (such as `NBD_CMD_BLOCK_STATUS` without the flag
+`NBD_CMD_FLAG_REQ_ONE`), the server MUST NOT send a reply larger than
+the maximum block size. For commands that do not require a payload
+(such as `NBD_CMD_TRIM`), the client MAY request a length larger than
+the maximum; the server SHOULD NOT disconnect, but MAY reply with an
+`NBD_EOVERFLOW` or `NBD_EINVAL` error if the oversize request would
+require more server resources than the same command operating on only
+the maximum block size (such as some implementations of
+`NBD_CMD_WRITE_ZEROES` without the flag `NBD_CMD_FLAG_FAST_ZERO`, or
+`NBD_CMD_CACHE`).
Where a transmission request can have a nonzero *offset* and/or
*length* (such as `NBD_CMD_READ`, `NBD_CMD_WRITE`, or `NBD_CMD_TRIM`),
@@ -833,24 +846,17 @@ the client MUST ensure that *offset* and *length* are integer
multiples of any advertised minimum block size, and SHOULD use integer
multiples of any advertised preferred block size where possible. For
those requests, the client MUST NOT use a *length* which, when added to
-*offset*, would exceed the export size. Also for NBD_CMD_READ,
-NBD_CMD_WRITE, NBD_CMD_CACHE and NBD_CMD_WRITE_ZEROES (except for
-when NBD_CMD_FLAG_FAST_ZERO is set), the client MUST NOT use a *length*
-larger than any advertised maximum block size.
-The server SHOULD report an `NBD_EINVAL` error if
-the client's request is not aligned to advertised minimum block size
-boundaries, or is larger than the advertised maximum block size.
+*offset*, would exceed the export size. The server SHOULD report an
+`NBD_EINVAL` error if the client's request is not aligned to advertised
+minimum block size boundaries or would exceed the export size.
+
Notwithstanding any maximum block size advertised, either the server
-or the client MAY initiate a hard disconnect if the payload of an
-`NBD_CMD_WRITE` request or `NBD_CMD_READ` reply would be large enough
-to be deemed a denial of service attack; however, for maximum
-portability, any *length* less than 2^25 (33,554,432) bytes SHOULD NOT
-be considered a denial of service attack (even if the advertised
-maximum block size is smaller). For all other commands, where the
-*length* is not reflected in the payload (such as `NBD_CMD_TRIM` or
-`NBD_CMD_WRITE_ZEROES`), a server SHOULD merely fail the command with
-an `NBD_EINVAL` error for a client that exceeds the maximum block size,
-rather than initiating a hard disconnect.
+or the client MAY initiate a hard disconnect if a payload length of
+either a request or a reply would be large enough to be deemed a
+denial of service attack; however, for maximum portability, any
+*length* less than 2^25 (33,554,432) bytes SHOULD NOT be considered a
+denial of service attack (even if the advertised maximum block size is
+smaller).
## Metadata querying
@@ -1592,7 +1598,7 @@ during option haggling in the fixed newstyle negotiation.
- 16 bits, `NBD_INFO_BLOCK_SIZE`
- 32 bits, minimum block size
- 32 bits, preferred block size
- - 32 bits, maximum block size
+ - 32 bits, maximum block payload size
* `NBD_REP_META_CONTEXT` (4)
@@ -1740,7 +1746,8 @@ Some chunk types can additionally be categorized by role, such as
*error chunks* or *content chunks*. Each type determines how to
interpret the "length" bytes of payload. If the client receives
an unknown or unexpected type, other than an *error chunk*, it
-MUST initiate a hard disconnect.
+MUST initiate a hard disconnect. A server MUST NOT send a chunk
+larger than any advertised maximum block payload size.
* `NBD_REPLY_TYPE_NONE` (0)
@@ -2111,11 +2118,12 @@ The following request types exist:
If the server advertised `NBD_FLAG_SEND_FAST_ZERO` but
`NBD_CMD_FLAG_FAST_ZERO` is not set, then the server MUST NOT fail
with `NBD_ENOTSUP`, even if the operation is no faster than a
- corresponding `NBD_CMD_WRITE`. Conversely, if
- `NBD_CMD_FLAG_FAST_ZERO` is set, the server MUST fail quickly with
- `NBD_ENOTSUP` unless the request can be serviced in less time than
- a corresponding `NBD_CMD_WRITE`, and SHOULD NOT alter the contents
- of the export when returning this failure. The server's
+ corresponding `NBD_CMD_WRITE`. Conversely, if `NBD_CMD_FLAG_FAST_ZERO`
+ is set, the server SHOULD NOT fail with `NBD_EOVERFLOW` regardless of
+ the client length, MUST fail quickly with `NBD_ENOTSUP` unless the
+ request can be serviced in less time than a corresponding
+ `NBD_CMD_WRITE`, and SHOULD NOT alter the contents of the export when
+ returning an `NBD_ENOTSUP` failure. The server's
determination on whether to fail a fast request MAY depend on a
number of factors, such as whether the request was suitably
aligned, on whether the `NBD_CMD_FLAG_NO_HOLE` flag was present,
@@ -2266,7 +2274,9 @@ SHOULD return `NBD_EPERM` if it receives a write or trim request on
a
read-only export.
The server SHOULD NOT return `NBD_EOVERFLOW` except as documented in
-response to `NBD_CMD_READ` when `NBD_CMD_FLAG_DF` is supported.
+response to `NBD_CMD_READ` when `NBD_CMD_FLAG_DF` is supported, or when
+a command without payload requests a length larger than an advertised
+maximum block payload length.
The server SHOULD NOT return `NBD_ENOTSUP` except as documented in
response to `NBD_CMD_WRITE_ZEROES` when `NBD_CMD_FLAG_FAST_ZERO` is
--
2.35.1
996
days inactive
997
days old
11 comments
5 participants
participants (5)
-
Eric Blake -
Nir Soffer -
Richard W.M. Jones -
Vladimir Sementsov-Ogievskiy -
Wouter Verhelst