On 4/23/19 4:01 AM, Richard W.M. Jones wrote: Looks reasonable for ensuring there is no sensitive leak. And compared to my comments on patch 2, at least here you are only slowing down .pread, and leaving .pwrite unchanged. Although my other comments on patch 2 question whether we can still come up with something more efficient by reusing a buffer rather than having to reinitialize it on every single NBD_CMD_READ (leaking previously-seen plugin contents is not the same risk as leaking uninitialized data). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

On 4/23/19 4:01 AM, Richard W.M. Jones wrote: This patch does not document any guarantee. Would it be worth explicitly mentioning this behavior as part of the API guarantee of .pread, so that plugins like zero and null can rely on the incoming buffer being pre-zeroed rather than having to repeat the memset() themselves? Should this behavior have an opt-out switch for plugins concerned with speed? For safety, we want this behavior to be on by default (as we don't know whether out-of-tree plugins might otherwise leak our heap data). But if we were to add a new field to include/nbd-plugin.h that a plugin can set when they DON'T want to waste time on the pre-emptive clear because they promise to fill the entire buffer, then the bug shifts out of nbdkit for leaking data and over to the plugin for setting the flag incorrectly if it breaks the promise. The new field won't be set by any existing out-of-tree plugins, and newly written plugins will have the documentation of that particular aspect to make their decision on whether to opt-out. Definitely a safety fix for READ, but isn't this a speed penalty for WRITE? That is, there is a cost of zeroing the buffer (especially if it is a 32M request, where zeroing it may be large enough to also have performance impacts due to cache pressure), when we KNOW that we are going to immediately rewrite the buffer with incoming data before ever calling into the plugin's .pwrite, makes it seem like we should limit the calloc or memset to just the READ path. I don't know if the added time spent on calloc() is significant, or if it is in the noise compared to other overheads such as the socket traffic, but it may be worth benchmarking (I know you have your fio patches that might help us answer that question). Is there any smarter algorithm we could use? Right now, we are doing a malloc()/free() per READ/WRITE. And although libc malloc() should in general ty to be efficient on frequent malloc reuse, we can often still be faster by reusing allocations rather than doing frequent malloc()/free(). Would it be worth exploring a patch that creates a per-thread buffer (maybe initially sized at 64k, and widened only as the client sends larger requests), where we reuse the buffer between successive calls rather than reallocating? We'd still want to initially zero the buffer when we first allocate one for the thread or when we enlarge it, but we can argue that after that point, a plugin with a .pread that does a partial job will only leak data that has already been previously read from or written to that plugin (and no longer risk a leak of OUR heap data), which means we avoid both the malloc() and the memset() in the common case, for slightly faster behavior, and with no visible difference in output for a plugin that fully fills the buffer in .pread. [I still hope to someday improve the nbdkit threading to create helper threads on demand due to actual client parallel requests, rather than its current behavior of spawning all parallel threads up front when the client first connects - that becomes more important if we are also allocating up to 64M in per-thread buffers, when we don't need the allocation for a client that only ever sends serialized requests] And if we DO switch to per-thread reused buffers, it may STILL make sense to have nbdkit-plugin.h have a way for plugins to opt-in or opt-out of whether .pread is guaranteed a pre-zeroed buffer on all reads. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org