similar to: [nbdkit PATCH v2] nbd: Fix memory leak

Time to enhance the nbd plugin to request structured replies from the server. For now, deal only with structured reads. The server can now return sparse reads, even though we need nbdkit version 3 before we can in turn return sparse reads back to the client. In general, we have to assume the server is malicious, and so we must sanity check that it sends replies we expect. Thus, we have a choice

In preparation for allowing interleaved response, refactor the nbd forwarder so that writes are still done from the thread handling the original request from the client, but all reads are done by a dedicated reader thread. Control between the two flags is gated by a mutex for storing the transaction information, coupled with a pipe for the reader thread to send the final status back to the

When we first used libnbd (nbdkit 1.14), it was brand new, so we left in the ability to build a limited nbd plugin without the use of libnbd for comparison. But now that libnbd has had more time under its belt, and we have learned that our nbd-standalone code is not getting much testing, it's time to follow through with our plan to finish off the old code, and now build the nbd plugin only

This is a minimal implementation of an NBD forwarder; it lets us convert between old and newstyle connections (great if a client expects one style but the real server only provides the other), or add TLS safety on top of a server without having to rewrite that server. Right now, the real server is expected to live on a named Unix socket with no encryption, and the transactions are serialized

I ran some local testing on my Fedora 29 system via: $ ./nbdkit memory 1m $ for i in `seq 10`; do ./nbdkit -U - --filter=stats nbd \ statsappend=true statsfile=$file hostname=localhost port=10809 \ --run '~/libnbd/examples/threaded-reads-and-writes $unixsocket' done Pre-patch, the runs averaged 1.266s, 1.30E+08 bits/s Post-patch, the runs averaged 1.154s, 1.42E+08

Now that we properly clean up 'trans' in the reader thread, we must not dereference 'trans' from the write thread at any point after trans has been added to the list unless we have grabbed it back off the list ourselves, or we risk an occasional use-after-free or even double free that valgrind can detect. Reported-by: Richard W.M. Jones <rjones@redhat.com> Fixes:

On 3/8/19 4:04 AM, Richard W.M. Jones wrote: > This is about the simplest implementation of NBD Structured Replies > (SR) that we can do right now. > > It accepts NBD_OPT_STRUCTURED_REPLIES when negotiated by the client, > but only sends back the simplest possible SR when required to by > NBD_CMD_READ. The rest of the time it will send back simple replies > as before. We do

This is about the simplest implementation of NBD Structured Replies (SR) that we can do right now. It accepts NBD_OPT_STRUCTURED_REPLIES when negotiated by the client, but only sends back the simplest possible SR when required to by NBD_CMD_READ. The rest of the time it will send back simple replies as before. We do not modify the plugin API so plugins are unable to send complex SRs. Also we

With this, I am finally able to get the nbd plugin to do out-of-order responses to the client. Once this series goes in, we should be ready for Rich to cut a release. Eric Blake (4): nbd: Split reading into separate thread nbd: Protect writes with mutex nbd: Enable parallel handling tests: Test parallel nbd behavior plugins/nbd/nbd.c | 217

On 4/22/19 7:50 PM, Eric Blake wrote: > Time to enhance the nbd plugin to request structured replies from the > server. For now, deal only with structured reads. The server can now > return sparse reads, even though we need nbdkit version 3 before we > can in turn return sparse reads back to the client. > > In general, we have to assume the server is malicious, and so we must

I'm still working on the interleaving (and Rich reminded me on IRC that we still don't have THREAD_MODEL_PARALLEL working anywhere yet, anyways). Since nbdkit doesn't really have a parallel plugin yet, my testing on that front will have to use qemu-nbd as the original server, as well as qemu-io as the driver (qemu-io's aio_read and aio_write commands can be used to trigger

Use new 'name_of_*' functions to offer a bit more details about messages being forwarded on to the remote server. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/nbd/nbd.c | 8 +++++--- plugins/nbd/Makefile.am | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 672f35b..6bd8861 100644 ---

On Sat, Dec 02, 2017 at 11:52:31AM -0600, Eric Blake wrote: > When converting from a single transaction to a linked list, I > forgot to free the storage for each member of the list. > > Reported-by: Richard W.M. Jones <rjones at redhat.com> > Fixes: 7f5bb9bf13f041ea7702bda557d9dd668bc3423a > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > > I'm

Updated based on other changes that have happened in the meantime: - rely more on cleanup.h (throughout) - split structured read for easier review (patch 2 and 3 were combined in v1) - rely on nbdkit not leaking a server's partial answer (patch 3) - add tests (patch 5) - other bug fixes I found while testing it - drop EOVERFLOW patch for now; it will be separate once upstream NBD protocol

On Fri, Dec 07, 2018 at 10:00:42AM -0600, Eric Blake wrote: > Use new 'name_of_*' functions to offer a bit more details about > messages being forwarded on to the remote server. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > plugins/nbd/nbd.c | 8 +++++--- > plugins/nbd/Makefile.am | 3 ++- > 2 files changed, 7 insertions(+), 4 deletions(-)

I'm hoping to implement .extents for the nbd plugin; this is a prerequisite. I'm not sure about patch 3 - if we like it, I'll squash it to 2, if we don't, I think we are okay just dropping it. I'm also wondering if we have to worry about malicious plugins that don't populate the entire .pread buffer in an effort to get nbdkit to expose portions of the heap; my patch 7 loses

libnbd-0.1.2-1 is now available in Fedora 29/30 updates-testing, although it was not compiled against libxml2 so it lacks uri support (I ended up testing patch 4 with a self-built libnbd). Diffs since v1 - rebase to master, bump from libnbd 0.1 to 0.1.2, add URI support, better timing results Still not done - patch 5 needs associated tests Eric Blake (5): nbd: Check for libnbd nbd:

When converting from a single transaction to a linked list, I forgot to free the storage for each member of the list. Reported-by: Richard W.M. Jones <rjones at redhat.com> Fixes: 7f5bb9bf13f041ea7702bda557d9dd668bc3423a Signed-off-by: Eric Blake <eblake at redhat.com> --- I'm still not sure why 'make check' passes while 'make check-valgrind' fails for

libnbd-0.1.4-1 is now available in Fedora 29/30 updates testing. Diffs since v2 - rebase to master, bump from libnbd 0.1.2 to 0.1.3+, add tests to TLS usage which flushed out the need to turn relative pathnames into absolute, doc tweaks Now that the testsuite covers TLS and libnbd has been fixed to provide the things I found lacking when developing v2, I'm leaning towards pushing this on

This is a minimal implementation of an NBD forwarder; it lets us convert between old and newstyle connections (great if a client expects one style but the real server only provides the other), or add TLS safety on top of a server without having to rewrite that server. Right now, the real server is expected to live on a named Unix socket, and the transactions are serialized rather than