Re: [Libguestfs] [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:

> > 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.