[PATCH v1 1/1] migration: Avoid false-positive on non-supported scenarios for zero-copy-send

[PATCH v1 1/1] migration: Avoid false-positive on non-supported scenarios for zero-copy-send

[Leonardo Bras]

Migration with zero-copy-send currently has it's limitations, as it can't
be used with TLS nor any kind of compression. In such scenarios, it should
output errors during parameter / capability setting.

But currently there are some ways of setting this not-supported scenarios
without printing the error message:

!) For 'compression' capability, it works by enabling it together with
zero-copy-send. This happens because the validity test for zero-copy uses
the helper unction migrate_use_compression(), which check for compression
presence in s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS].

The point here is: the validity test happens before the capability gets
enabled. If all of them get enabled together, this test will not return
error.

In order to fix that, replace migrate_use_compression() by directly testing
the cap_list parameter migrate_caps_check().

2) For features enabled by parameters such as TLS & 'multifd_compression',
there was also a possibility of setting non-supported scenarios: setting
zero-copy-send first, then setting the unsupported parameter.

In order to fix that, also add a check for parameters conflicting with
zero-copy-send on migrate_params_check().

3) XBZRLE is also a compression capability, so it makes sense to also add
it to the list of capabilities which are not supported with zero-copy-send.

Fixes: 1abaec9a1b2c ("migration: Change zero_copy_send from migration parameter to migration capability")
Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 migration/migration.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 78f5057373..c6260e54bf 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1274,7 +1274,9 @@ static bool migrate_caps_check(bool *cap_list,
 #ifdef CONFIG_LINUX
     if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] &&
         (!cap_list[MIGRATION_CAPABILITY_MULTIFD] ||
-         migrate_use_compression() ||
+         cap_list[MIGRATION_CAPABILITY_COMPRESS] ||
+         cap_list[MIGRATION_CAPABILITY_XBZRLE] ||
+         migrate_multifd_compression() ||
          migrate_use_tls())) {
         error_setg(errp,
                    "Zero copy only available for non-compressed non-TLS multifd migration");
@@ -1511,6 +1513,17 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
         error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
         return false;
     }
+
+#ifdef CONFIG_LINUX
+    if (migrate_use_zero_copy_send() &&
+        ((params->has_multifd_compression && params->multifd_compression) ||
+         (params->has_tls_creds && params->tls_creds && *params->tls_creds))) {
+        error_setg(errp,
+                   "Zero copy only available for non-compressed non-TLS multifd migration");
+        return false;
+    }
+#endif
+
     return true;
 }
 
-- 
2.37.1

Re: [PATCH v1 1/1] migration: Avoid false-positive on non-supported scenarios for zero-copy-send

[Dr. David Alan Gilbert]

* Leonardo Bras (leobras@redhat.com) wrote:
> Migration with zero-copy-send currently has it's limitations, as it can't
> be used with TLS nor any kind of compression. In such scenarios, it should
> output errors during parameter / capability setting.
> 
> But currently there are some ways of setting this not-supported scenarios
> without printing the error message:
> 
> !) For 'compression' capability, it works by enabling it together with
> zero-copy-send. This happens because the validity test for zero-copy uses
> the helper unction migrate_use_compression(), which check for compression
> presence in s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS].
> 
> The point here is: the validity test happens before the capability gets
> enabled. If all of them get enabled together, this test will not return
> error.
> 
> In order to fix that, replace migrate_use_compression() by directly testing
> the cap_list parameter migrate_caps_check().
> 
> 2) For features enabled by parameters such as TLS & 'multifd_compression',
> there was also a possibility of setting non-supported scenarios: setting
> zero-copy-send first, then setting the unsupported parameter.
> 
> In order to fix that, also add a check for parameters conflicting with
> zero-copy-send on migrate_params_check().
> 
> 3) XBZRLE is also a compression capability, so it makes sense to also add
> it to the list of capabilities which are not supported with zero-copy-send.
> 
> Fixes: 1abaec9a1b2c ("migration: Change zero_copy_send from migration parameter to migration capability")
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Yeh, it's unusual in that you need to check both the capabilities and
parameters; where as we have the inidividual 'caps_check' and
'params_check'.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 78f5057373..c6260e54bf 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1274,7 +1274,9 @@ static bool migrate_caps_check(bool *cap_list,
>  #ifdef CONFIG_LINUX
>      if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] &&
>          (!cap_list[MIGRATION_CAPABILITY_MULTIFD] ||
> -         migrate_use_compression() ||
> +         cap_list[MIGRATION_CAPABILITY_COMPRESS] ||
> +         cap_list[MIGRATION_CAPABILITY_XBZRLE] ||
> +         migrate_multifd_compression() ||
>           migrate_use_tls())) {
>          error_setg(errp,
>                     "Zero copy only available for non-compressed non-TLS multifd migration");
> @@ -1511,6 +1513,17 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>          error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
>          return false;
>      }
> +
> +#ifdef CONFIG_LINUX
> +    if (migrate_use_zero_copy_send() &&
> +        ((params->has_multifd_compression && params->multifd_compression) ||
> +         (params->has_tls_creds && params->tls_creds && *params->tls_creds))) {
> +        error_setg(errp,
> +                   "Zero copy only available for non-compressed non-TLS multifd migration");
> +        return false;
> +    }
> +#endif
> +
>      return true;
>  }
>  
> -- 
> 2.37.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v1 1/1] migration: Avoid false-positive on non-supported scenarios for zero-copy-send

[Peter Xu]

On Tue, Jul 19, 2022 at 09:23:45AM -0300, Leonardo Bras wrote:
> Migration with zero-copy-send currently has it's limitations, as it can't
> be used with TLS nor any kind of compression. In such scenarios, it should
> output errors during parameter / capability setting.
> 
> But currently there are some ways of setting this not-supported scenarios
> without printing the error message:
> 
> !) For 'compression' capability, it works by enabling it together with
> zero-copy-send. This happens because the validity test for zero-copy uses
> the helper unction migrate_use_compression(), which check for compression
> presence in s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS].
> 
> The point here is: the validity test happens before the capability gets
> enabled. If all of them get enabled together, this test will not return
> error.
> 
> In order to fix that, replace migrate_use_compression() by directly testing
> the cap_list parameter migrate_caps_check().
> 
> 2) For features enabled by parameters such as TLS & 'multifd_compression',
> there was also a possibility of setting non-supported scenarios: setting
> zero-copy-send first, then setting the unsupported parameter.
> 
> In order to fix that, also add a check for parameters conflicting with
> zero-copy-send on migrate_params_check().
> 
> 3) XBZRLE is also a compression capability, so it makes sense to also add
> it to the list of capabilities which are not supported with zero-copy-send.
> 
> Fixes: 1abaec9a1b2c ("migration: Change zero_copy_send from migration parameter to migration capability")
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  migration/migration.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 78f5057373..c6260e54bf 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1274,7 +1274,9 @@ static bool migrate_caps_check(bool *cap_list,
>  #ifdef CONFIG_LINUX
>      if (cap_list[MIGRATION_CAPABILITY_ZERO_COPY_SEND] &&
>          (!cap_list[MIGRATION_CAPABILITY_MULTIFD] ||
> -         migrate_use_compression() ||
> +         cap_list[MIGRATION_CAPABILITY_COMPRESS] ||
> +         cap_list[MIGRATION_CAPABILITY_XBZRLE] ||
> +         migrate_multifd_compression() ||
>           migrate_use_tls())) {
>          error_setg(errp,
>                     "Zero copy only available for non-compressed non-TLS multifd migration");
> @@ -1511,6 +1513,17 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>          error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
>          return false;
>      }
> +
> +#ifdef CONFIG_LINUX

A trivial nit: we don't need this since migrate_use_zero_copy_send() will
be defined unconditionally, and it's returning false with !CONFIG_LINUX.
So feel free to drop this if there's a new version.

> +    if (migrate_use_zero_copy_send() &&
> +        ((params->has_multifd_compression && params->multifd_compression) ||
> +         (params->has_tls_creds && params->tls_creds && *params->tls_creds))) {
> +        error_setg(errp,
> +                   "Zero copy only available for non-compressed non-TLS multifd migration");
> +        return false;
> +    }
> +#endif

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu