Re: [Libguestfs] [PATCH 1/2] spec: Recommend cap on NBD_REPLY_TYPE_BLOCK_STATUS length

[Nir Soffer]

On Fri, Apr 8, 2022 at 6:47 PM Eric Blake <eblake@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