| Message ID | 20221114224655.2186173-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD spec changes for 64-bit extensions | expand |
On 11/15/22 01:46, 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 so large that the client would treat it as a > denial of service attack. Clients currently have no way during > negotiation to request such a limit of the server, so it is easier to > just document this as a restriction on viable server implementations > than to add yet another round of handshaking. Also, mentioning > amplification effects is worthwhile. > > 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:XYZ 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). So nbdkit enforced a bound of 1M > extents (8M+4 bytes, nbdkit commit 6e0dc839ea, Jun 2019). [Unrelated > to this patch, but worth noting for history: nbdkit's situation also > has to deal with the fact that it is designed for plugin server > implementations; and not capping the number of extents in a reply also > posed a problem to nbdkit as the server, where a plugin could exhaust > memory and kill the server, unrelated to any size constraints enforced > by a client.] > > 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. s-o-b line missed. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
On Fri, Dec 16, 2022 at 10:32:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 11/15/22 01:46, 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 so large that the client would treat it as a > > denial of service attack. Clients currently have no way during > > negotiation to request such a limit of the server, so it is easier to > > just document this as a restriction on viable server implementations > > than to add yet another round of handshaking. Also, mentioning > > amplification effects is worthwhile. > > > > 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:XYZ 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). So nbdkit enforced a bound of 1M > > extents (8M+4 bytes, nbdkit commit 6e0dc839ea, Jun 2019). [Unrelated > > to this patch, but worth noting for history: nbdkit's situation also > > has to deal with the fact that it is designed for plugin server > > implementations; and not capping the number of extents in a reply also > > posed a problem to nbdkit as the server, where a plugin could exhaust > > memory and kill the server, unrelated to any size constraints enforced > > by a client.] > > > > 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. > > s-o-b line missed. I'm not sure if the NBD project has a strict policy on including one, but I don't mind adding it. > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > -- > Best regards, > Vladimir >
On Fri, Mar 03, 2023 at 04:17:40PM -0600, Eric Blake wrote: > On Fri, Dec 16, 2022 at 10:32:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > s-o-b line missed. > > I'm not sure if the NBD project has a strict policy on including one, > but I don't mind adding it. I've never required it, mostly because it's something that I myself always forget, too, so, *shrug*. (if there were a way in git to make it add that automatically, that would help; I've looked but haven't found it)
On 3/5/23 09:41, Wouter Verhelst wrote: > On Fri, Mar 03, 2023 at 04:17:40PM -0600, Eric Blake wrote: >> On Fri, Dec 16, 2022 at 10:32:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: >>> s-o-b line missed. >> >> I'm not sure if the NBD project has a strict policy on including one, >> but I don't mind adding it. > > I've never required it, mostly because it's something that I myself > always forget, too, so, *shrug*. > > (if there were a way in git to make it add that automatically, that > would help; I've looked but haven't found it) > You can point the "commit.template" git-config knob to a particular file, and then include your S-o-b in that file. There is also the "-s" ("--signoff") option for git-commit, but I don't see a git-config knob to make that permanent. (You can always introduce a git.alias for it though.) Laszlo
On Sun, Mar 5, 2023 at 10:42 AM Wouter Verhelst <w@uter.be> wrote: > > On Fri, Mar 03, 2023 at 04:17:40PM -0600, Eric Blake wrote: > > On Fri, Dec 16, 2022 at 10:32:01PM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > s-o-b line missed. > > > > I'm not sure if the NBD project has a strict policy on including one, > > but I don't mind adding it. > > I've never required it, mostly because it's something that I myself > always forget, too, so, *shrug*. > > (if there were a way in git to make it add that automatically, that > would help; I've looked but haven't found it) What I'm using in all projects that require signed-off-by is: $ cat .git/hooks/commit-msg #!/bin/sh # Add Signed-off-by trailer. sob=$(git var GIT_AUTHOR_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p') git interpret-trailers --in-place --trailer "$sob" "$1" You can also use a pre-commit hook but the commit-msg hook is more convenient. And in github you can add the DCO application to the project: https://github.com/apps/dco Once installed it will check that all commits are signed off, and provide helpful error messages to contributors. Nir
diff --git a/doc/proto.md b/doc/proto.md index 3a96703..8f08583 100644 --- a/doc/proto.md +++ b/doc/proto.md @@ -1818,6 +1818,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 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 @@ -2180,26 +2186,31 @@ The following request types exist: `NBD_REPLY_TYPE_BLOCK_STATUS` chunk represent consecutive portions of the file starting from specified *offset*. If the client used the `NBD_CMD_FLAG_REQ_ONE` flag, each chunk contains exactly one - descriptor where the *length* of the descriptor MUST NOT be greater - than the *length* of the request; otherwise, a chunk MAY contain - 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* - 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 - lengths that are an integer multiple of 512 bytes where possible - (the first and last descriptor of an unaligned query being the - most obvious places for an exception), and MUST use descriptor - lengths that are an integer multiple of any advertised minimum - block size. The status flags are intentionally defined so that a - server MAY always safely report a status of 0 for any block, - although the server SHOULD return additional status values when - they can be easily detected. + descriptor where the *length* of the descriptor MUST NOT be + greater than the *length* of the request; otherwise, a chunk MAY + contain 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. 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 lengths that are + an integer multiple of 512 bytes where possible (the first and + last descriptor of an unaligned query being the most obvious + places for an exception), in part to avoid an amplification effect + where a series of smaller descriptors can cause the server's reply + to occupy more bytes than the *length* of the client's request. + The server MUST use descriptor lengths that are an integer + multiple of any advertised minimum block size. The status flags + are intentionally defined so that a server MAY always safely + report a status of 0 for any block, although the server SHOULD + return additional status values when they can be easily detected. If an error occurs, the server SHOULD set the appropriate error code in the error field of an error chunk. However, if the error