On Tuesday, 13 February 2018 19:49:12 CET Max Reitz wrote: > On 2018-01-18 17:44, Pino Toscano wrote: > > Rewrite the implementation of the ssh block driver to use libssh instead > > of libssh2. The libssh library has various advantages over libssh2: > > - easier API for authentication (for example for using ssh-agent) > > - easier API for known_hosts handling > > - supports newer types of keys in known_hosts > > > > Kerberos authentication can be enabled once the libssh buxg for it [1] is > > fixed. > > > > The development version of libssh (i.e. the future 0.8.x) supports > > fsync, so reuse the build time check for this. > > > > [1] https://red.libssh.org/issues/242 > > > > Signed-off-by: Pino Toscano <ptosc...@redhat.com> > > ---
Sorry for the (very late) reply.
I fixed basically all the code issues noted with this review; follow
few replies/notes that are not just "fixed".
> > Changes from v3:
> > - fix socket cleanup in connect_to_ssh()
> > - add comments about the socket cleanup
> > - improve the error reporting (closer to what was with libssh2)
> > - improve EOF detection on sftp_read()
> >
> > Changes from v2:
> > - used again an own fd
> > - fixed co_yield() implementation
> >
> > Changes from v1:
> > - fixed jumbo packets writing
> > - fixed missing 'err' assignment
> > - fixed commit message
>
> One thing: The performance seems to have dropped hugely, from what I can
> tell.
>
> Before this patch, running the iotests 1-10 over ssh (raw/ssh) took
> 12.6 s. With this patch, they take 59.3 s. Perhaps the starkest
> contrast can be seen in test 1, which took 4 s before and 27 s after --
> this test simply reads and writes 128 MB of continuous data.
>
> I like having elliptic curves, but I think this patch needs optimization
> work before we can replace libssh2.
One thing that I discovered helping a lot is setting the TCP_NODELAY
option on the socket, to disable the Nagle algorithm; this drastically
reduced the overhead, which now does not seem to be more than 200% on
the very intensive tests (at least with my benchmarks).
Also using libssh from master shows more improvements too (and a bit
more of instability though, but that's a different story), and the
resulting overhead seems more acceptable to me now.
>
> > block/Makefile.objs | 6 +-
> > block/ssh.c | 522
> > ++++++++++++++++++++++++----------------------------
> > configure | 65 ++++---
> > 3 files changed, 278 insertions(+), 315 deletions(-)
>
> [...]
>
> > diff --git a/block/ssh.c b/block/ssh.c
> > index b049a16eb9..2975fc27d8 100644
> > --- a/block/ssh.c
> > +++ b/block/ssh.c
>
> [...]
>
> > @@ -87,27 +81,25 @@ static void ssh_state_init(BDRVSSHState *s)
> > {
> > memset(s, 0, sizeof *s);
> > s->sock = -1;
> > - s->offset = -1;
> > qemu_co_mutex_init(&s->lock);
> > }
> >
> > static void ssh_state_free(BDRVSSHState *s)
> > {
> > + if (s->attrs) {
> > + sftp_attributes_free(s->attrs);
> > + }
> > if (s->sftp_handle) {
> > - libssh2_sftp_close(s->sftp_handle);
> > + sftp_close(s->sftp_handle);
> > }
> > if (s->sftp) {
> > - libssh2_sftp_shutdown(s->sftp);
> > + sftp_free(s->sftp);
> > }
> > if (s->session) {
> > - libssh2_session_disconnect(s->session,
> > - "from qemu ssh client: "
> > - "user closed the connection");
> > - libssh2_session_free(s->session);
> > - }
> > - if (s->sock >= 0) {
> > - close(s->sock);
> > + ssh_disconnect(s->session);
> > + ssh_free(s->session);
> > }
> > + /* s->sock is owned by the ssh_session, which free's it. */
>
> s/free's/frees/
>
> > }
> >
> > static void GCC_FMT_ATTR(3, 4)
> > @@ -121,13 +113,13 @@ session_error_setg(Error **errp, BDRVSSHState *s,
> > const char *fs, ...)
> > va_end(args);
> >
> > if (s->session) {
> > - char *ssh_err;
> > + const char *ssh_err;
> > int ssh_err_code;
> >
> > - /* This is not an errno. See <libssh2.h>. */
> > - ssh_err_code = libssh2_session_last_error(s->session,
> > - &ssh_err, NULL, 0);
> > - error_setg(errp, "%s: %s (libssh2 error code: %d)",
> > + /* This is not an errno. See <libssh/libssh.h>. */
> > + ssh_err = ssh_get_error(s->session);
> > + ssh_err_code = ssh_get_error_code(s->session);
> > + error_setg(errp, "%s: %s (libssh error code: %d)",
> > msg, ssh_err, ssh_err_code);
>
> Maybe we should not append the error info if there is no error.
>
> (Example:
>
> $ ./qemu-img info ssh://localhost/tmp/foo
> qemu-img: Could not open 'ssh://localhost/tmp/foo': no host key was
> found in known_hosts: (libssh error code: 0)
The current libssh2 code has the same issue -- the solution is what you
mention later on, i.e. use error_setg() directly.
There were few more "mismatches" on the usage of the error functions,
which I fixed.
> > @@ -291,68 +283,41 @@ static void ssh_parse_filename(const char *filename,
> > QDict *options,
> > static int check_host_key_knownhosts(BDRVSSHState *s,
> > const char *host, int port, Error
> > **errp)
> > {
> > - const char *home;
> > - char *knh_file = NULL;
> > - LIBSSH2_KNOWNHOSTS *knh = NULL;
> > - struct libssh2_knownhost *found;
> > - int ret, r;
> > - const char *hostkey;
> > - size_t len;
> > - int type;
> > + int ret;
> > + int state;
> >
> > - hostkey = libssh2_session_hostkey(s->session, &len, &type);
> > - if (!hostkey) {
> > - ret = -EINVAL;
> > - session_error_setg(errp, s, "failed to read remote host key");
> > - goto out;
> > - }
> > + state = ssh_is_server_known(s->session);
> >
> > - knh = libssh2_knownhost_init(s->session);
> > - if (!knh) {
> > - ret = -EINVAL;
> > - session_error_setg(errp, s,
> > - "failed to initialize known hosts support");
> > - goto out;
> > - }
> > -
> > - home = getenv("HOME");
> > - if (home) {
> > - knh_file = g_strdup_printf("%s/.ssh/known_hosts", home);
> > - } else {
> > - knh_file = g_strdup_printf("/root/.ssh/known_hosts");
> > - }
> > -
> > - /* Read all known hosts from OpenSSH-style known_hosts file. */
> > - libssh2_knownhost_readfile(knh, knh_file,
> > LIBSSH2_KNOWNHOST_FILE_OPENSSH);
> > -
> > - r = libssh2_knownhost_checkp(knh, host, port, hostkey, len,
> > - LIBSSH2_KNOWNHOST_TYPE_PLAIN|
> > - LIBSSH2_KNOWNHOST_KEYENC_RAW,
> > - &found);
> > - switch (r) {
> > - case LIBSSH2_KNOWNHOST_CHECK_MATCH:
> > + switch (state) {
> > + case SSH_SERVER_KNOWN_OK:
> > /* OK */
> > - DPRINTF("host key OK: %s", found->key);
> > break;
> > - case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
> > + case SSH_SERVER_KNOWN_CHANGED:
> > ret = -EINVAL;
> > session_error_setg(errp, s,
> > - "host key does not match the one in known_hosts"
> > - " (found key %s)", found->key);
> > + "host key does not match the one in known_hosts");
>
> Technically, this is against the specification which requires us to warn
> the user about a possible attack. :-P
Sadly libssh up to 0.7 does not provide an API for this; there is work
ongoing upstream to expand the "knownhosts" API, and query for this
information. Once it is finalized (e.g. with a 0.8 release), I will
make this block device use it.
> > @@ -407,18 +368,31 @@ static int compare_fingerprint(const unsigned char
> > *fingerprint, size_t len,
> >
> > static int
> > check_host_key_hash(BDRVSSHState *s, const char *hash,
> > - int hash_type, size_t fingerprint_len, Error **errp)
> > + enum ssh_publickey_hash_type type, size_t
> > fingerprint_len,
> > + Error **errp)
> > {
> > - const char *fingerprint;
> > + int r;
> > + ssh_key pubkey;
> > + unsigned char *server_hash;
> > + size_t server_hash_len;
> >
> > - fingerprint = libssh2_hostkey_hash(s->session, hash_type);
> > - if (!fingerprint) {
> > + r = ssh_get_publickey(s->session, &pubkey);
>
> The documentation says this is deprecated and one should use
> ssh_get_server_publickey() instead (it just looks like a rename, though).
Only in current master, i.e the future 0.8.
> > @@ -628,6 +570,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict
> > *options,
> > Error *local_err = NULL;
> > const char *user, *path, *host_key_check;
> > long port = 0;
> > + unsigned long portU = 0;
>
> I was about to say: How about making port an unsigned long and swapping
> the qemu_strtol() for a qemu_strtoul()?
>
> But I think you'd rather want an unsigned int instead (and that won't
> work with qemu_strtoul()).
After Dan implemented qemu_strtoui(), this now can use unsigned int.
> > ret = -EINVAL;
> > goto err;
> > }
> >
> > + /* Make sure the SFTP file is handled in blocking mode. */
> > + sftp_file_set_blocking(s->sftp_handle);
> > +
>
> Hm, but ssh_file_open() makes the SSH session non-blocking immediately
> afterwards, and it doesn't seem like the old libssh2 cared about how
> SFTP behaved. Was that the same thing in libssh2? Should
> ssh_file_open() make the SFTP session non-blocking as well?
The implementation for blocking/non-blocking follows the behaviour
currently implemented using libssh2, and in libssh2 itself.
--
Pino Toscano
signature.asc
Description: This is a digitally signed message part.
