[PATCH nbdkit] xz: Do not pass can_write through to the plugin.
I'm not sure that this fix is really correct. An alternate way I can think to fix this would be for the core server to maintain a readonly flag for each layer (instead of just per- server). You could also argue that our readonly test in server/connections.c:compute_eflags is wrong and/or that the implementations of server/plugins.c:plugin_can_write and server/filters.c:filter_can_write have the wrong default - they should consult the readonly flag which was passed to that layer's open call. However the fix only affects one filter, and we're not maintaining an API for filters, so maybe we can defer the problem. Rich.
Using ‘nbdkit --filter=xz file disk.xz’ and loop mounting eventually results in this error: nbdkit: file[1]: debug: xz: pwrite count=4096 offset=1048576 flags=0x0 nbdkit: file[1]: debug: pwrite count=4096 offset=1048576 fua=0 nbdkit: file[1]: error: pwrite: Bad file descriptor nbdkit: file[1]: debug: sending error reply: Bad file descriptor Since the filter did not intercept can_write, it was passed through to the layer below (the file plugin). The filter opens the file plugin passing readonly=1, but in the case where there are no filters, nbdkit core would never call can_write() (assuming that because we opened the server readonly, it would return false). However with the filter, can_write is called and returns the default (true). This caused writes to be generated which were also passed through to the plugin which generates "Bad file descriptor" because: EBADF fd is not a valid file descriptor or is not open for writing. This change also disables zero and trim because the server logic disables those when the top backend->can_write returns false. --- filters/xz/xz.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/filters/xz/xz.c b/filters/xz/xz.c index 28a6a81..5ff791e 100644 --- a/filters/xz/xz.c +++ b/filters/xz/xz.c @@ -173,6 +173,13 @@ xz_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) return xzfile_get_size (h->xz); } +static int +xz_can_write (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + return 0; +} + /* Read data from the file. */ static int xz_pread (struct nbdkit_next_ops *next_ops, void *nxdata, @@ -225,6 +232,7 @@ static struct nbdkit_filter filter = { .close = xz_close, .prepare = xz_prepare, .get_size = xz_get_size, + .can_write = xz_can_write, .pread = xz_pread, }; -- 2.20.1
On 1/30/19 8:36 AM, Richard W.M. Jones wrote:
Using ‘nbdkit --filter=xz file disk.xz’ and loop mounting eventually results in this error:
nbdkit: file[1]: debug: xz: pwrite count=4096 offset=1048576 flags=0x0 nbdkit: file[1]: debug: pwrite count=4096 offset=1048576 fua=0 nbdkit: file[1]: error: pwrite: Bad file descriptor nbdkit: file[1]: debug: sending error reply: Bad file descriptor
Since the filter did not intercept can_write, it was passed through to the layer below (the file plugin). The filter opens the file plugin passing readonly=1, but in the case where there are no filters, nbdkit core would never call can_write() (assuming that because we opened the server readonly, it would return false). However with the filter, can_write is called and returns the default (true). This caused writes to be generated which were also passed through to the plugin which generates "Bad file descriptor" because:
EBADF fd is not a valid file descriptor or is not open for writing.
This change also disables zero and trim because the server logic disables those when the top backend->can_write returns false.
ACK.
--- filters/xz/xz.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/filters/xz/xz.c b/filters/xz/xz.c index 28a6a81..5ff791e 100644 --- a/filters/xz/xz.c +++ b/filters/xz/xz.c @@ -173,6 +173,13 @@ xz_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) return xzfile_get_size (h->xz); }
+static int +xz_can_write (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle)
Perhaps worth a comment in the code, similar to cow.c's comment about the override returning a constant because the filter enforces a particular behavior that might be different from the backend's default behavior.
+{ + return 0; +} + /* Read data from the file. */ static int xz_pread (struct nbdkit_next_ops *next_ops, void *nxdata, @@ -225,6 +232,7 @@ static struct nbdkit_filter filter = { .close = xz_close, .prepare = xz_prepare, .get_size = xz_get_size, + .can_write = xz_can_write, .pread = xz_pread, };
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 1/30/19 8:36 AM, Richard W.M. Jones wrote:
I'm not sure that this fix is really correct.
The fix is correct in the sense that yes, the xz filter cannot allow writes, and must therefore override write support from leaking through from the plugin.
An alternate way I can think to fix this would be for the core server to maintain a readonly flag for each layer (instead of just per- server).
You could also argue that our readonly test in server/connections.c:compute_eflags is wrong and/or that the implementations of server/plugins.c:plugin_can_write and server/filters.c:filter_can_write have the wrong default - they should consult the readonly flag which was passed to that layer's open call.
Arguing that filters.c picks the wrong defaults makes a bit of intuitive sense, but it trickier to implement correctly. We have several examples of filters that override .pwrite but not .can_write (e.g. truncate), some that override .can_write but not .pwrite (xz), some that override both (cow), some that override neither (nozero). When .can_write is overridden, it is obviously correct. But when .can_write is absent, whether or not .pwrite is also absent, I can see your idea of defaulting to "if I passed readonly=true to the plugin's open(), synthesize .can_write of false no matter what; if I passed readonly=false to the plugin's open(), then call the plugin's .can_write() which may return true or false based on other aspects up to the plugin". And we can track what the filter passes to the plugin's .open, since a filter's .open MUST call the next(nxdata) parameters it was given. Since next(nxdata) is implemented in filters.c, THAT could be the place where we stash off whether the filter is passing true/false to the underling plugin's .open (even if that value differs from the value of readonly passed to the filter's .open), such that we can then have filters.c in charge of remembering the per-connection state of whether the underlying plugin is in forced-readonly mode and therefore the filter must synthesize a .can_read that forces false.
However the fix only affects one filter, and we're not maintaining an API for filters, so maybe we can defer the problem.
I agree that our filter API is not stable, so I'm okay with your patch going in now, regardless of whether we also change filters.c to have saner defaults. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones