[PATCH v4 0/3] block/rbd: Add support for layered encryption

[PATCH v4 0/3] block/rbd: Add support for layered encryption

[Or Ozeri]

v4: split to multiple commits
    add support for more than just luks-any in layered encryption
    nit fixes
v3: further nit fixes suggested by @idryomov
v2: nit fixes suggested by @idryomov


Or Ozeri (3):
  block/rbd: encryption nit fixes
  block/rbd: Add luks-any encryption opening option
  block/rbd: Add support for layered encryption

 block/rbd.c          | 204 +++++++++++++++++++++++++++++++++++++++----
 qapi/block-core.json |  35 +++++++-
 2 files changed, 221 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH v4 1/3] block/rbd: encryption nit fixes

[Or Ozeri]

Add const modifier to passphrases,
and remove redundant stack variable passphrase_len.

Signed-off-by: Or Ozeri <oro@il.ibm.com>
---
 block/rbd.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index f826410f40..e575105e6d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
 #ifdef LIBRBD_SUPPORTS_ENCRYPTION
 static int qemu_rbd_convert_luks_options(
         RbdEncryptionOptionsLUKSBase *luks_opts,
-        char **passphrase,
+        const char **passphrase,
         size_t *passphrase_len,
         Error **errp)
 {
@@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
 static int qemu_rbd_convert_luks_create_options(
         RbdEncryptionCreateOptionsLUKSBase *luks_opts,
         rbd_encryption_algorithm_t *alg,
-        char **passphrase,
+        const char **passphrase,
         size_t *passphrase_len,
         Error **errp)
 {
@@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
                                       Error **errp)
 {
     int r = 0;
-    g_autofree char *passphrase = NULL;
-    size_t passphrase_len;
+    g_autofree const char *passphrase = NULL;
     rbd_encryption_format_t format;
     rbd_encryption_options_t opts;
     rbd_encryption_luks1_format_options_t luks_opts;
@@ -407,12 +406,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
             opts_size = sizeof(luks_opts);
             r = qemu_rbd_convert_luks_create_options(
                     qapi_RbdEncryptionCreateOptionsLUKS_base(&encrypt->u.luks),
-                    &luks_opts.alg, &passphrase, &passphrase_len, errp);
+                    &luks_opts.alg, &passphrase, &luks_opts.passphrase_size,
+                    errp);
             if (r < 0) {
                 return r;
             }
             luks_opts.passphrase = passphrase;
-            luks_opts.passphrase_size = passphrase_len;
             break;
         }
         case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
@@ -423,12 +422,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
             r = qemu_rbd_convert_luks_create_options(
                     qapi_RbdEncryptionCreateOptionsLUKS2_base(
                             &encrypt->u.luks2),
-                    &luks2_opts.alg, &passphrase, &passphrase_len, errp);
+                    &luks2_opts.alg, &passphrase, &luks2_opts.passphrase_size,
+                    errp);
             if (r < 0) {
                 return r;
             }
             luks2_opts.passphrase = passphrase;
-            luks2_opts.passphrase_size = passphrase_len;
             break;
         }
         default: {
@@ -466,8 +465,7 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
                                     Error **errp)
 {
     int r = 0;
-    g_autofree char *passphrase = NULL;
-    size_t passphrase_len;
+    g_autofree const char *passphrase = NULL;
     rbd_encryption_luks1_format_options_t luks_opts;
     rbd_encryption_luks2_format_options_t luks2_opts;
     rbd_encryption_format_t format;
@@ -482,12 +480,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
             opts_size = sizeof(luks_opts);
             r = qemu_rbd_convert_luks_options(
                     qapi_RbdEncryptionOptionsLUKS_base(&encrypt->u.luks),
-                    &passphrase, &passphrase_len, errp);
+                    &passphrase, &luks_opts.passphrase_size, errp);
             if (r < 0) {
                 return r;
             }
             luks_opts.passphrase = passphrase;
-            luks_opts.passphrase_size = passphrase_len;
             break;
         }
         case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
@@ -497,12 +494,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
             opts_size = sizeof(luks2_opts);
             r = qemu_rbd_convert_luks_options(
                     qapi_RbdEncryptionOptionsLUKS2_base(&encrypt->u.luks2),
-                    &passphrase, &passphrase_len, errp);
+                    &passphrase, &luks2_opts.passphrase_size, errp);
             if (r < 0) {
                 return r;
             }
             luks2_opts.passphrase = passphrase;
-            luks2_opts.passphrase_size = passphrase_len;
             break;
         }
         default: {
-- 
2.25.1

[PATCH v4 2/3] block/rbd: Add luks-any encryption opening option

[Or Ozeri]

Ceph RBD encryption API required specifying the encryption format
for loading encryption. The supported formats were LUKS (v1) and LUKS2.

Starting from Reef release, RBD also supports loading with "luks-any" format,
which works for both versions of LUKS.

This commit extends the qemu rbd driver API to enable qemu users to use
this luks-any wildcard format.

Signed-off-by: Or Ozeri <oro@il.ibm.com>
---
 block/rbd.c          | 19 +++++++++++++++++++
 qapi/block-core.json | 20 ++++++++++++++++++--
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index e575105e6d..7feae45e82 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -468,6 +468,9 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
     g_autofree const char *passphrase = NULL;
     rbd_encryption_luks1_format_options_t luks_opts;
     rbd_encryption_luks2_format_options_t luks2_opts;
+#ifdef LIBRBD_SUPPORTS_ENCRYPTION_LOAD2
+    rbd_encryption_luks_format_options_t luks_any_opts;
+#endif
     rbd_encryption_format_t format;
     rbd_encryption_options_t opts;
     size_t opts_size;
@@ -501,6 +504,22 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
             luks2_opts.passphrase = passphrase;
             break;
         }
+#ifdef LIBRBD_SUPPORTS_ENCRYPTION_LOAD2
+        case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS_ANY: {
+            memset(&luks_any_opts, 0, sizeof(luks_any_opts));
+            format = RBD_ENCRYPTION_FORMAT_LUKS;
+            opts = &luks_any_opts;
+            opts_size = sizeof(luks_any_opts);
+            r = qemu_rbd_convert_luks_options(
+                    qapi_RbdEncryptionOptionsLUKSAny_base(&encrypt->u.luks_any),
+                    &passphrase, &luks_any_opts.passphrase_size, errp);
+            if (r < 0) {
+                return r;
+            }
+            luks_any_opts.passphrase = passphrase;
+            break;
+        }
+#endif
         default: {
             r = -ENOTSUP;
             error_setg_errno(
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 882b266532..d064847d85 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3753,10 +3753,16 @@
 ##
 # @RbdImageEncryptionFormat:
 #
+# luks
+#
+# luks2
+#
+# luks-any: Used for opening either luks or luks2. (Since 8.0)
+#
 # Since: 6.1
 ##
 { 'enum': 'RbdImageEncryptionFormat',
-  'data': [ 'luks', 'luks2' ] }
+  'data': [ 'luks', 'luks2', 'luks-any' ] }
 
 ##
 # @RbdEncryptionOptionsLUKSBase:
@@ -3798,6 +3804,15 @@
   'base': 'RbdEncryptionOptionsLUKSBase',
   'data': { } }
 
+##
+# @RbdEncryptionOptionsLUKSAny:
+#
+# Since: 8.0
+##
+{ 'struct': 'RbdEncryptionOptionsLUKSAny',
+  'base': 'RbdEncryptionOptionsLUKSBase',
+  'data': { } }
+
 ##
 # @RbdEncryptionCreateOptionsLUKS:
 #
@@ -3825,7 +3840,8 @@
   'base': { 'format': 'RbdImageEncryptionFormat' },
   'discriminator': 'format',
   'data': { 'luks': 'RbdEncryptionOptionsLUKS',
-            'luks2': 'RbdEncryptionOptionsLUKS2' } }
+            'luks2': 'RbdEncryptionOptionsLUKS2',
+            'luks-any': 'RbdEncryptionOptionsLUKSAny'} }
 
 ##
 # @RbdEncryptionCreateOptions:
-- 
2.25.1

[PATCH v4 3/3] block/rbd: Add support for layered encryption

[Or Ozeri]

Starting from ceph Reef, RBD has built-in support for layered encryption,
where each ancestor image (in a cloned image setting) can be possibly
encrypted using a unique passphrase.

A new function, rbd_encryption_load2, was added to librbd API.
This new function supports an array of passphrases (via "spec" structs).

This commit extends the qemu rbd driver API to use this new librbd API,
in order to support this new layered encryption feature.

Signed-off-by: Or Ozeri <oro@il.ibm.com>
---
 block/rbd.c          | 161 ++++++++++++++++++++++++++++++++++++++++++-
 qapi/block-core.json |  17 ++++-
 2 files changed, 175 insertions(+), 3 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 7feae45e82..157922e23a 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -71,6 +71,16 @@ static const char rbd_luks2_header_verification[
     'L', 'U', 'K', 'S', 0xBA, 0xBE, 0, 2
 };
 
+static const char rbd_layered_luks_header_verification[
+        RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = {
+    'R', 'B', 'D', 'L', 0xBA, 0xBE, 0, 1
+};
+
+static const char rbd_layered_luks2_header_verification[
+        RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = {
+    'R', 'B', 'D', 'L', 0xBA, 0xBE, 0, 2
+};
+
 typedef enum {
     RBD_AIO_READ,
     RBD_AIO_WRITE,
@@ -537,6 +547,136 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
 
     return 0;
 }
+
+#ifdef LIBRBD_SUPPORTS_ENCRYPTION_LOAD2
+static int qemu_rbd_encryption_load2(rbd_image_t image,
+                                     RbdEncryptionOptions *encrypt,
+                                     Error **errp)
+{
+    int r = 0;
+    int encrypt_count = 1;
+    int i;
+    RbdEncryptionOptions *curr_encrypt;
+    rbd_encryption_spec_t *specs;
+    rbd_encryption_luks1_format_options_t* luks_opts;
+    rbd_encryption_luks2_format_options_t* luks2_opts;
+    rbd_encryption_luks_format_options_t* luks_any_opts;
+
+    /* count encryption options */
+    for (curr_encrypt = encrypt; curr_encrypt->has_parent;
+         curr_encrypt = curr_encrypt->parent) {
+        ++encrypt_count;
+    }
+
+    specs = g_new0(rbd_encryption_spec_t, encrypt_count);
+
+    curr_encrypt = encrypt;
+    for (i = 0; i < encrypt_count; ++i) {
+        switch (curr_encrypt->format) {
+            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS: {
+                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS1;
+                specs[i].opts_size =
+                        sizeof(rbd_encryption_luks1_format_options_t);
+
+                luks_opts = g_new0(rbd_encryption_luks1_format_options_t, 1);
+                specs[i].opts = luks_opts;
+
+                r = qemu_rbd_convert_luks_options(
+                        qapi_RbdEncryptionOptionsLUKS_base(
+                                &curr_encrypt->u.luks),
+                        &luks_opts->passphrase,
+                        &luks_opts->passphrase_size,
+                        errp);
+                break;
+            }
+
+            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
+                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS2;
+                specs[i].opts_size =
+                        sizeof(rbd_encryption_luks2_format_options_t);
+
+                luks2_opts = g_new0(rbd_encryption_luks2_format_options_t, 1);
+                specs[i].opts = luks2_opts;
+
+                r = qemu_rbd_convert_luks_options(
+                        qapi_RbdEncryptionOptionsLUKS2_base(
+                                &curr_encrypt->u.luks2),
+                        &luks2_opts->passphrase,
+                        &luks2_opts->passphrase_size,
+                        errp);
+                break;
+            }
+
+            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS_ANY: {
+                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS;
+                specs[i].opts_size =
+                        sizeof(rbd_encryption_luks_format_options_t);
+
+                luks_any_opts = g_new0(rbd_encryption_luks_format_options_t, 1);
+                specs[i].opts = luks_any_opts;
+
+                r = qemu_rbd_convert_luks_options(
+                        qapi_RbdEncryptionOptionsLUKSAny_base(
+                                &curr_encrypt->u.luks_any),
+                        &luks_any_opts->passphrase,
+                        &luks_any_opts->passphrase_size,
+                        errp);
+                break;
+            }
+
+            default: {
+                r = -ENOTSUP;
+                error_setg_errno(
+                        errp, -r, "unknown image encryption format: %u",
+                        curr_encrypt->format);
+            }
+        }
+
+        if (r < 0) {
+            goto exit;
+        }
+
+        curr_encrypt = curr_encrypt->parent;
+    }
+
+    r = rbd_encryption_load2(image, specs, encrypt_count);
+    if (r < 0) {
+        error_setg_errno(errp, -r, "layered encryption load fail");
+        goto exit;
+    }
+
+exit:
+    for (i = 0; i < encrypt_count; ++i) {
+        if (!specs[i].opts) {
+            break;
+        }
+
+        switch (specs[i].format) {
+            case RBD_ENCRYPTION_FORMAT_LUKS1: {
+                luks_opts = specs[i].opts;
+                g_free((void*)luks_opts->passphrase);
+                break;
+            }
+
+            case RBD_ENCRYPTION_FORMAT_LUKS2: {
+                luks2_opts = specs[i].opts;
+                g_free((void*)luks2_opts->passphrase);
+                break;
+            }
+
+            case RBD_ENCRYPTION_FORMAT_LUKS: {
+                luks_any_opts = specs[i].opts;
+                g_free((void*)luks_any_opts->passphrase);
+                break;
+            }
+        }
+
+        g_free(specs[i].opts);
+    }
+    g_free(specs);
+    return r;
+}
+#endif
 #endif
 
 /* FIXME Deprecate and remove keypairs or make it available in QMP. */
@@ -1008,7 +1148,16 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (opts->has_encrypt) {
 #ifdef LIBRBD_SUPPORTS_ENCRYPTION
-        r = qemu_rbd_encryption_load(s->image, opts->encrypt, errp);
+        if (opts->encrypt->has_parent) {
+#ifdef LIBRBD_SUPPORTS_ENCRYPTION_LOAD2
+            r = qemu_rbd_encryption_load2(s->image, opts->encrypt, errp);
+#else
+            r = -ENOTSUP;
+            error_setg(errp, "RBD library does not support layered encryption");
+#endif
+        } else {
+            r = qemu_rbd_encryption_load(s->image, opts->encrypt, errp);
+        }
         if (r < 0) {
             goto failed_post_open;
         }
@@ -1299,6 +1448,16 @@ static ImageInfoSpecific *qemu_rbd_get_specific_info(BlockDriverState *bs,
         spec_info->u.rbd.data->encryption_format =
                 RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2;
         spec_info->u.rbd.data->has_encryption_format = true;
+    } else if (memcmp(buf, rbd_layered_luks_header_verification,
+               RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN) == 0) {
+        spec_info->u.rbd.data->encryption_format =
+                RBD_IMAGE_ENCRYPTION_FORMAT_LUKS_LAYERED;
+        spec_info->u.rbd.data->has_encryption_format = true;
+    } else if (memcmp(buf, rbd_layered_luks2_header_verification,
+               RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN) == 0) {
+        spec_info->u.rbd.data->encryption_format =
+                RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2_LAYERED;
+        spec_info->u.rbd.data->has_encryption_format = true;
     } else {
         spec_info->u.rbd.data->has_encryption_format = false;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d064847d85..68f8c7c203 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3759,10 +3759,14 @@
 #
 # luks-any: Used for opening either luks or luks2. (Since 8.0)
 #
+# luks-layered: Layered encryption. Only used for info. (Since 8.0)
+#
+# luks2-layered: Layered encryption. Only used for info. (Since 8.0)
+#
 # Since: 6.1
 ##
 { 'enum': 'RbdImageEncryptionFormat',
-  'data': [ 'luks', 'luks2', 'luks-any' ] }
+  'data': [ 'luks', 'luks2', 'luks-any', 'luks-layered', 'luks2-layered' ] }
 
 ##
 # @RbdEncryptionOptionsLUKSBase:
@@ -3834,10 +3838,19 @@
 ##
 # @RbdEncryptionOptions:
 #
+# @format: Encryption format.
+#
+# @parent: Parent image encryption options (for cloned images).
+#          Can be left unspecified if this cloned image is encrypted
+#          using the same format and secret as its parent image (i.e.
+#          not explicitly formatted) or if its parent image is not
+#          encrypted. (Since 8.0)
+#
 # Since: 6.1
 ##
 { 'union': 'RbdEncryptionOptions',
-  'base': { 'format': 'RbdImageEncryptionFormat' },
+  'base': { 'format': 'RbdImageEncryptionFormat',
+            '*parent': 'RbdEncryptionOptions' },
   'discriminator': 'format',
   'data': { 'luks': 'RbdEncryptionOptionsLUKS',
             'luks2': 'RbdEncryptionOptionsLUKS2',
-- 
2.25.1

Re: [PATCH v4 3/3] block/rbd: Add support for layered encryption

[Ilya Dryomov]

On Sun, Nov 20, 2022 at 11:28 AM Or Ozeri <oro@il.ibm.com> wrote:
>
> Starting from ceph Reef, RBD has built-in support for layered encryption,
> where each ancestor image (in a cloned image setting) can be possibly
> encrypted using a unique passphrase.
>
> A new function, rbd_encryption_load2, was added to librbd API.
> This new function supports an array of passphrases (via "spec" structs).
>
> This commit extends the qemu rbd driver API to use this new librbd API,
> in order to support this new layered encryption feature.
>
> Signed-off-by: Or Ozeri <oro@il.ibm.com>
> ---
>  block/rbd.c          | 161 ++++++++++++++++++++++++++++++++++++++++++-
>  qapi/block-core.json |  17 ++++-
>  2 files changed, 175 insertions(+), 3 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 7feae45e82..157922e23a 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -71,6 +71,16 @@ static const char rbd_luks2_header_verification[
>      'L', 'U', 'K', 'S', 0xBA, 0xBE, 0, 2
>  };
>
> +static const char rbd_layered_luks_header_verification[
> +        RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = {
> +    'R', 'B', 'D', 'L', 0xBA, 0xBE, 0, 1
> +};
> +
> +static const char rbd_layered_luks2_header_verification[
> +        RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = {
> +    'R', 'B', 'D', 'L', 0xBA, 0xBE, 0, 2
> +};
> +
>  typedef enum {
>      RBD_AIO_READ,
>      RBD_AIO_WRITE,
> @@ -537,6 +547,136 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
>
>      return 0;
>  }
> +
> +#ifdef LIBRBD_SUPPORTS_ENCRYPTION_LOAD2
> +static int qemu_rbd_encryption_load2(rbd_image_t image,
> +                                     RbdEncryptionOptions *encrypt,
> +                                     Error **errp)
> +{
> +    int r = 0;
> +    int encrypt_count = 1;
> +    int i;
> +    RbdEncryptionOptions *curr_encrypt;
> +    rbd_encryption_spec_t *specs;
> +    rbd_encryption_luks1_format_options_t* luks_opts;
> +    rbd_encryption_luks2_format_options_t* luks2_opts;
> +    rbd_encryption_luks_format_options_t* luks_any_opts;

Hi Or,

Stick to the pointer alignment style used in this file:

    rbd_encryption_luks1_format_options_t *luks_opts;
    rbd_encryption_luks2_format_options_t *luks2_opts;
    rbd_encryption_luks_format_options_t *luks_any_opts;

> +
> +    /* count encryption options */
> +    for (curr_encrypt = encrypt; curr_encrypt->has_parent;

I think this needs to be rebased on top of 54fde4ff0621 ("qapi block:
Elide redundant has_FOO in generated C").  has_parent is probably not
a thing anymore.

> +         curr_encrypt = curr_encrypt->parent) {
> +        ++encrypt_count;
> +    }
> +
> +    specs = g_new0(rbd_encryption_spec_t, encrypt_count);
> +
> +    curr_encrypt = encrypt;
> +    for (i = 0; i < encrypt_count; ++i) {
> +        switch (curr_encrypt->format) {
> +            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS: {
> +                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS1;
> +                specs[i].opts_size =
> +                        sizeof(rbd_encryption_luks1_format_options_t);
> +
> +                luks_opts = g_new0(rbd_encryption_luks1_format_options_t, 1);
> +                specs[i].opts = luks_opts;

I would move opts_size assignment here and avoid repeating the type (and
similar for LUKS2 and LUKS cases):

    specs[i].opts_size = sizeof(*luks_opts);

> +
> +                r = qemu_rbd_convert_luks_options(
> +                        qapi_RbdEncryptionOptionsLUKS_base(
> +                                &curr_encrypt->u.luks),
> +                        &luks_opts->passphrase,
> +                        &luks_opts->passphrase_size,
> +                        errp);
> +                break;
> +            }
> +

No need to leave a blank line between case statements.

> +            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
> +                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS2;
> +                specs[i].opts_size =
> +                        sizeof(rbd_encryption_luks2_format_options_t);
> +
> +                luks2_opts = g_new0(rbd_encryption_luks2_format_options_t, 1);
> +                specs[i].opts = luks2_opts;
> +
> +                r = qemu_rbd_convert_luks_options(
> +                        qapi_RbdEncryptionOptionsLUKS2_base(
> +                                &curr_encrypt->u.luks2),
> +                        &luks2_opts->passphrase,
> +                        &luks2_opts->passphrase_size,
> +                        errp);
> +                break;
> +            }
> +
> +            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS_ANY: {
> +                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS;
> +                specs[i].opts_size =
> +                        sizeof(rbd_encryption_luks_format_options_t);
> +
> +                luks_any_opts = g_new0(rbd_encryption_luks_format_options_t, 1);
> +                specs[i].opts = luks_any_opts;
> +
> +                r = qemu_rbd_convert_luks_options(
> +                        qapi_RbdEncryptionOptionsLUKSAny_base(
> +                                &curr_encrypt->u.luks_any),
> +                        &luks_any_opts->passphrase,
> +                        &luks_any_opts->passphrase_size,
> +                        errp);
> +                break;
> +            }
> +
> +            default: {
> +                r = -ENOTSUP;
> +                error_setg_errno(
> +                        errp, -r, "unknown image encryption format: %u",
> +                        curr_encrypt->format);
> +            }
> +        }
> +
> +        if (r < 0) {
> +            goto exit;
> +        }
> +
> +        curr_encrypt = curr_encrypt->parent;
> +    }
> +
> +    r = rbd_encryption_load2(image, specs, encrypt_count);
> +    if (r < 0) {
> +        error_setg_errno(errp, -r, "layered encryption load fail");
> +        goto exit;
> +    }
> +
> +exit:
> +    for (i = 0; i < encrypt_count; ++i) {
> +        if (!specs[i].opts) {
> +            break;
> +        }
> +
> +        switch (specs[i].format) {
> +            case RBD_ENCRYPTION_FORMAT_LUKS1: {
> +                luks_opts = specs[i].opts;
> +                g_free((void*)luks_opts->passphrase);

Pointer alignment style:

    g_free((void *)luks_opts->passphrase);

> +                break;
> +            }
> +

No need to leave a blank line between case statements.

Thanks,

                Ilya

Re: [PATCH v4 1/3] block/rbd: encryption nit fixes

[Daniel P. Berrangé]

On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> Add const modifier to passphrases,
> and remove redundant stack variable passphrase_len.
> 
> Signed-off-by: Or Ozeri <oro@il.ibm.com>
> ---
>  block/rbd.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index f826410f40..e575105e6d 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
>  #ifdef LIBRBD_SUPPORTS_ENCRYPTION
>  static int qemu_rbd_convert_luks_options(
>          RbdEncryptionOptionsLUKSBase *luks_opts,
> -        char **passphrase,
> +        const char **passphrase,
>          size_t *passphrase_len,
>          Error **errp)
>  {
> @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
>  static int qemu_rbd_convert_luks_create_options(
>          RbdEncryptionCreateOptionsLUKSBase *luks_opts,
>          rbd_encryption_algorithm_t *alg,
> -        char **passphrase,
> +        const char **passphrase,
>          size_t *passphrase_len,
>          Error **errp)
>  {
> @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
>                                        Error **errp)
>  {
>      int r = 0;
> -    g_autofree char *passphrase = NULL;
> -    size_t passphrase_len;
> +    g_autofree const char *passphrase = NULL;

This looks wierd.  If it is as const string, why are
we free'ing it ?  Either want g_autofree, or const,
but not both.

>      rbd_encryption_format_t format;
>      rbd_encryption_options_t opts;
>      rbd_encryption_luks1_format_options_t luks_opts;
> @@ -407,12 +406,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
>              opts_size = sizeof(luks_opts);
>              r = qemu_rbd_convert_luks_create_options(
>                      qapi_RbdEncryptionCreateOptionsLUKS_base(&encrypt->u.luks),
> -                    &luks_opts.alg, &passphrase, &passphrase_len, errp);
> +                    &luks_opts.alg, &passphrase, &luks_opts.passphrase_size,
> +                    errp);
>              if (r < 0) {
>                  return r;
>              }
>              luks_opts.passphrase = passphrase;
> -            luks_opts.passphrase_size = passphrase_len;
>              break;
>          }
>          case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
> @@ -423,12 +422,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
>              r = qemu_rbd_convert_luks_create_options(
>                      qapi_RbdEncryptionCreateOptionsLUKS2_base(
>                              &encrypt->u.luks2),
> -                    &luks2_opts.alg, &passphrase, &passphrase_len, errp);
> +                    &luks2_opts.alg, &passphrase, &luks2_opts.passphrase_size,
> +                    errp);
>              if (r < 0) {
>                  return r;
>              }
>              luks2_opts.passphrase = passphrase;
> -            luks2_opts.passphrase_size = passphrase_len;
>              break;
>          }
>          default: {
> @@ -466,8 +465,7 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
>                                      Error **errp)
>  {
>      int r = 0;
> -    g_autofree char *passphrase = NULL;
> -    size_t passphrase_len;
> +    g_autofree const char *passphrase = NULL;
>      rbd_encryption_luks1_format_options_t luks_opts;
>      rbd_encryption_luks2_format_options_t luks2_opts;
>      rbd_encryption_format_t format;
> @@ -482,12 +480,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
>              opts_size = sizeof(luks_opts);
>              r = qemu_rbd_convert_luks_options(
>                      qapi_RbdEncryptionOptionsLUKS_base(&encrypt->u.luks),
> -                    &passphrase, &passphrase_len, errp);
> +                    &passphrase, &luks_opts.passphrase_size, errp);
>              if (r < 0) {
>                  return r;
>              }
>              luks_opts.passphrase = passphrase;
> -            luks_opts.passphrase_size = passphrase_len;
>              break;
>          }
>          case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
> @@ -497,12 +494,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
>              opts_size = sizeof(luks2_opts);
>              r = qemu_rbd_convert_luks_options(
>                      qapi_RbdEncryptionOptionsLUKS2_base(&encrypt->u.luks2),
> -                    &passphrase, &passphrase_len, errp);
> +                    &passphrase, &luks2_opts.passphrase_size, errp);
>              if (r < 0) {
>                  return r;
>              }
>              luks2_opts.passphrase = passphrase;
> -            luks2_opts.passphrase_size = passphrase_len;
>              break;
>          }
>          default: {
> -- 
> 2.25.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 2/3] block/rbd: Add luks-any encryption opening option

[Daniel P. Berrangé]

On Sun, Nov 20, 2022 at 04:28:35AM -0600, Or Ozeri wrote:
> Ceph RBD encryption API required specifying the encryption format
> for loading encryption. The supported formats were LUKS (v1) and LUKS2.
> 
> Starting from Reef release, RBD also supports loading with "luks-any" format,
> which works for both versions of LUKS.
> 
> This commit extends the qemu rbd driver API to enable qemu users to use
> this luks-any wildcard format.
> 
> Signed-off-by: Or Ozeri <oro@il.ibm.com>
> ---
>  block/rbd.c          | 19 +++++++++++++++++++
>  qapi/block-core.json | 20 ++++++++++++++++++--
>  2 files changed, 37 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 3/3] block/rbd: Add support for layered encryption

[Daniel P. Berrangé]

On Sun, Nov 20, 2022 at 04:28:36AM -0600, Or Ozeri wrote:
> Starting from ceph Reef, RBD has built-in support for layered encryption,
> where each ancestor image (in a cloned image setting) can be possibly
> encrypted using a unique passphrase.
> 
> A new function, rbd_encryption_load2, was added to librbd API.
> This new function supports an array of passphrases (via "spec" structs).
> 
> This commit extends the qemu rbd driver API to use this new librbd API,
> in order to support this new layered encryption feature.
> 
> Signed-off-by: Or Ozeri <oro@il.ibm.com>
> ---
>  block/rbd.c          | 161 ++++++++++++++++++++++++++++++++++++++++++-
>  qapi/block-core.json |  17 ++++-
>  2 files changed, 175 insertions(+), 3 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 7feae45e82..157922e23a 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -71,6 +71,16 @@ static const char rbd_luks2_header_verification[
>      'L', 'U', 'K', 'S', 0xBA, 0xBE, 0, 2
>  };
>  
> +static const char rbd_layered_luks_header_verification[
> +        RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = {
> +    'R', 'B', 'D', 'L', 0xBA, 0xBE, 0, 1
> +};
> +
> +static const char rbd_layered_luks2_header_verification[
> +        RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN] = {
> +    'R', 'B', 'D', 'L', 0xBA, 0xBE, 0, 2
> +};
> +
>  typedef enum {
>      RBD_AIO_READ,
>      RBD_AIO_WRITE,
> @@ -537,6 +547,136 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
>  
>      return 0;
>  }
> +
> +#ifdef LIBRBD_SUPPORTS_ENCRYPTION_LOAD2
> +static int qemu_rbd_encryption_load2(rbd_image_t image,
> +                                     RbdEncryptionOptions *encrypt,
> +                                     Error **errp)
> +{
> +    int r = 0;
> +    int encrypt_count = 1;
> +    int i;
> +    RbdEncryptionOptions *curr_encrypt;
> +    rbd_encryption_spec_t *specs;
> +    rbd_encryption_luks1_format_options_t* luks_opts;
> +    rbd_encryption_luks2_format_options_t* luks2_opts;
> +    rbd_encryption_luks_format_options_t* luks_any_opts;
> +
> +    /* count encryption options */
> +    for (curr_encrypt = encrypt; curr_encrypt->has_parent;
> +         curr_encrypt = curr_encrypt->parent) {
> +        ++encrypt_count;
> +    }
> +
> +    specs = g_new0(rbd_encryption_spec_t, encrypt_count);
> +
> +    curr_encrypt = encrypt;
> +    for (i = 0; i < encrypt_count; ++i) {
> +        switch (curr_encrypt->format) {
> +            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS: {
> +                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS1;
> +                specs[i].opts_size =
> +                        sizeof(rbd_encryption_luks1_format_options_t);
> +
> +                luks_opts = g_new0(rbd_encryption_luks1_format_options_t, 1);
> +                specs[i].opts = luks_opts;
> +
> +                r = qemu_rbd_convert_luks_options(
> +                        qapi_RbdEncryptionOptionsLUKS_base(
> +                                &curr_encrypt->u.luks),
> +                        &luks_opts->passphrase,
> +                        &luks_opts->passphrase_size,
> +                        errp);
> +                break;
> +            }
> +
> +            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
> +                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS2;
> +                specs[i].opts_size =
> +                        sizeof(rbd_encryption_luks2_format_options_t);
> +
> +                luks2_opts = g_new0(rbd_encryption_luks2_format_options_t, 1);
> +                specs[i].opts = luks2_opts;
> +
> +                r = qemu_rbd_convert_luks_options(
> +                        qapi_RbdEncryptionOptionsLUKS2_base(
> +                                &curr_encrypt->u.luks2),
> +                        &luks2_opts->passphrase,
> +                        &luks2_opts->passphrase_size,
> +                        errp);
> +                break;
> +            }
> +
> +            case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS_ANY: {
> +                specs[i].format = RBD_ENCRYPTION_FORMAT_LUKS;
> +                specs[i].opts_size =
> +                        sizeof(rbd_encryption_luks_format_options_t);
> +
> +                luks_any_opts = g_new0(rbd_encryption_luks_format_options_t, 1);
> +                specs[i].opts = luks_any_opts;
> +
> +                r = qemu_rbd_convert_luks_options(
> +                        qapi_RbdEncryptionOptionsLUKSAny_base(
> +                                &curr_encrypt->u.luks_any),
> +                        &luks_any_opts->passphrase,
> +                        &luks_any_opts->passphrase_size,
> +                        errp);
> +                break;
> +            }
> +
> +            default: {
> +                r = -ENOTSUP;
> +                error_setg_errno(
> +                        errp, -r, "unknown image encryption format: %u",
> +                        curr_encrypt->format);
> +            }
> +        }
> +
> +        if (r < 0) {
> +            goto exit;
> +        }
> +
> +        curr_encrypt = curr_encrypt->parent;
> +    }
> +
> +    r = rbd_encryption_load2(image, specs, encrypt_count);
> +    if (r < 0) {
> +        error_setg_errno(errp, -r, "layered encryption load fail");
> +        goto exit;
> +    }
> +
> +exit:
> +    for (i = 0; i < encrypt_count; ++i) {
> +        if (!specs[i].opts) {
> +            break;
> +        }
> +
> +        switch (specs[i].format) {
> +            case RBD_ENCRYPTION_FORMAT_LUKS1: {
> +                luks_opts = specs[i].opts;
> +                g_free((void*)luks_opts->passphrase);
> +                break;
> +            }
> +
> +            case RBD_ENCRYPTION_FORMAT_LUKS2: {
> +                luks2_opts = specs[i].opts;
> +                g_free((void*)luks2_opts->passphrase);
> +                break;
> +            }
> +
> +            case RBD_ENCRYPTION_FORMAT_LUKS: {
> +                luks_any_opts = specs[i].opts;
> +                g_free((void*)luks_any_opts->passphrase);
> +                break;
> +            }
> +        }
> +
> +        g_free(specs[i].opts);
> +    }
> +    g_free(specs);
> +    return r;
> +}
> +#endif
>  #endif
>  
>  /* FIXME Deprecate and remove keypairs or make it available in QMP. */
> @@ -1008,7 +1148,16 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      if (opts->has_encrypt) {
>  #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> -        r = qemu_rbd_encryption_load(s->image, opts->encrypt, errp);
> +        if (opts->encrypt->has_parent) {
> +#ifdef LIBRBD_SUPPORTS_ENCRYPTION_LOAD2
> +            r = qemu_rbd_encryption_load2(s->image, opts->encrypt, errp);
> +#else
> +            r = -ENOTSUP;
> +            error_setg(errp, "RBD library does not support layered encryption");
> +#endif
> +        } else {
> +            r = qemu_rbd_encryption_load(s->image, opts->encrypt, errp);
> +        }
>          if (r < 0) {
>              goto failed_post_open;
>          }
> @@ -1299,6 +1448,16 @@ static ImageInfoSpecific *qemu_rbd_get_specific_info(BlockDriverState *bs,
>          spec_info->u.rbd.data->encryption_format =
>                  RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2;
>          spec_info->u.rbd.data->has_encryption_format = true;
> +    } else if (memcmp(buf, rbd_layered_luks_header_verification,
> +               RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN) == 0) {
> +        spec_info->u.rbd.data->encryption_format =
> +                RBD_IMAGE_ENCRYPTION_FORMAT_LUKS_LAYERED;
> +        spec_info->u.rbd.data->has_encryption_format = true;
> +    } else if (memcmp(buf, rbd_layered_luks2_header_verification,
> +               RBD_ENCRYPTION_LUKS_HEADER_VERIFICATION_LEN) == 0) {
> +        spec_info->u.rbd.data->encryption_format =
> +                RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2_LAYERED;
> +        spec_info->u.rbd.data->has_encryption_format = true;
>      } else {
>          spec_info->u.rbd.data->has_encryption_format = false;
>      }
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index d064847d85..68f8c7c203 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3759,10 +3759,14 @@
>  #
>  # luks-any: Used for opening either luks or luks2. (Since 8.0)
>  #
> +# luks-layered: Layered encryption. Only used for info. (Since 8.0)
> +#
> +# luks2-layered: Layered encryption. Only used for info. (Since 8.0)

I don't think we should be reporting this differently.

The layering is not a different encryption format. It is
a configuration convenience to avoid repeating the same
passphrase for a stack of images when opening an image.

In terms of encryption format it is still either using
'luks1' or 'luks2'.

If we want to report the fact that all parent images
use the same key, then we should introduce a new field
for that  in ImageInfoSpecificRbd eg perhaps

{ 'struct': 'ImageInfoSpecificRbd',
  'data': {
      '*encryption-format': 'RbdImageEncryptionFormat'
      '*encryption-layered': 'bool',
  } }


> +#
>  # Since: 6.1
>  ##
>  { 'enum': 'RbdImageEncryptionFormat',
> -  'data': [ 'luks', 'luks2', 'luks-any' ] }
> +  'data': [ 'luks', 'luks2', 'luks-any', 'luks-layered', 'luks2-layered' ] }
>  
>  ##
>  # @RbdEncryptionOptionsLUKSBase:
> @@ -3834,10 +3838,19 @@
>  ##
>  # @RbdEncryptionOptions:
>  #
> +# @format: Encryption format.
> +#
> +# @parent: Parent image encryption options (for cloned images).
> +#          Can be left unspecified if this cloned image is encrypted
> +#          using the same format and secret as its parent image (i.e.
> +#          not explicitly formatted) or if its parent image is not
> +#          encrypted. (Since 8.0)
> +#
>  # Since: 6.1
>  ##
>  { 'union': 'RbdEncryptionOptions',
> -  'base': { 'format': 'RbdImageEncryptionFormat' },
> +  'base': { 'format': 'RbdImageEncryptionFormat',
> +            '*parent': 'RbdEncryptionOptions' },
>    'discriminator': 'format',
>    'data': { 'luks': 'RbdEncryptionOptionsLUKS',
>              'luks2': 'RbdEncryptionOptionsLUKS2',
> -- 
> 2.25.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

RE: [PATCH v4 3/3] block/rbd: Add support for layered encryption

[Or Ozeri]

> -----Original Message-----
> From: Daniel P. Berrangé <berrange@redhat.com>
> Sent: Thursday, 12 January 2023 14:50
> To: Or Ozeri <ORO@il.ibm.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; Danny Harnik
> <DANNYH@il.ibm.com>; idryomov@gmail.com
> Subject: [EXTERNAL] Re: [PATCH v4 3/3] block/rbd: Add support for layered
> encryption
> 
> I don't think we should be reporting this differently.
> 
> The layering is not a different encryption format. It is a configuration
> convenience to avoid repeating the same passphrase for a stack of images
> when opening an image.
> 
> In terms of encryption format it is still either using 'luks1' or 'luks2'.
> 

I don’t think that's right.
The simplest argument is that the magic for RBD layered-luks is not "LUKS".
So, it's a different format, which cannot be opened by dm-crypt for example.
I think this is important for the user to know that, and thus it is useful to point it out
in the output of qemu-img info.

Re: [PATCH v4 3/3] block/rbd: Add support for layered encryption

[Daniel P. Berrangé]

On Thu, Jan 12, 2023 at 01:06:51PM +0000, Or Ozeri wrote:
> > -----Original Message-----
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > Sent: Thursday, 12 January 2023 14:50
> > To: Or Ozeri <ORO@il.ibm.com>
> > Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; Danny Harnik
> > <DANNYH@il.ibm.com>; idryomov@gmail.com
> > Subject: [EXTERNAL] Re: [PATCH v4 3/3] block/rbd: Add support for layered
> > encryption
> > 
> > I don't think we should be reporting this differently.
> > 
> > The layering is not a different encryption format. It is a configuration
> > convenience to avoid repeating the same passphrase for a stack of images
> > when opening an image.
> > 
> > In terms of encryption format it is still either using 'luks1' or 'luks2'.
> > 
> 
> I don’t think that's right.
> The simplest argument is that the magic for RBD layered-luks is not "LUKS".
> So, it's a different format, which cannot be opened by dm-crypt for example.
> I think this is important for the user to know that, and thus it is useful to point it out
> in the output of qemu-img info.

This different magic is an internal implementation detail of RBD. The
on-disk encryption is still following either the luks1 or luks2 format
spec. On the QEMU side we're only needing to know what the on disk format
spec is, and whether or not the parents use a common key, so that apps
know what they need to provide to QEMU for disk config. 

Opening a volume  with dm-crypt is not relevant to QEMU's usage, and
if users are doing that, they should be using the RBD tools directly
and qemu-img info is unrelated to that.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 1/3] block/rbd: encryption nit fixes

[Ilya Dryomov]

On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > Add const modifier to passphrases,
> > and remove redundant stack variable passphrase_len.
> >
> > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > ---
> >  block/rbd.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/block/rbd.c b/block/rbd.c
> > index f826410f40..e575105e6d 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> >  #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> >  static int qemu_rbd_convert_luks_options(
> >          RbdEncryptionOptionsLUKSBase *luks_opts,
> > -        char **passphrase,
> > +        const char **passphrase,
> >          size_t *passphrase_len,
> >          Error **errp)
> >  {
> > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> >  static int qemu_rbd_convert_luks_create_options(
> >          RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> >          rbd_encryption_algorithm_t *alg,
> > -        char **passphrase,
> > +        const char **passphrase,
> >          size_t *passphrase_len,
> >          Error **errp)
> >  {
> > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> >                                        Error **errp)
> >  {
> >      int r = 0;
> > -    g_autofree char *passphrase = NULL;
> > -    size_t passphrase_len;
> > +    g_autofree const char *passphrase = NULL;
>
> This looks wierd.  If it is as const string, why are
> we free'ing it ?  Either want g_autofree, or const,
> but not both.

Just curious, is it a requirement imposed by g_autofree?  Otherwise
pointer constness and pointee lifetime are completely orthogonal and
freeing (or, in this case, wanting to auto-free) an object referred to
by a const pointer seems perfectly fine to me.

Thanks,

                Ilya

Re: [PATCH v4 1/3] block/rbd: encryption nit fixes

[Daniel P. Berrangé]

On Thu, Jan 12, 2023 at 03:26:56PM +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > > Add const modifier to passphrases,
> > > and remove redundant stack variable passphrase_len.
> > >
> > > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > > ---
> > >  block/rbd.c | 24 ++++++++++--------------
> > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/block/rbd.c b/block/rbd.c
> > > index f826410f40..e575105e6d 100644
> > > --- a/block/rbd.c
> > > +++ b/block/rbd.c
> > > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > >  #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > >  static int qemu_rbd_convert_luks_options(
> > >          RbdEncryptionOptionsLUKSBase *luks_opts,
> > > -        char **passphrase,
> > > +        const char **passphrase,
> > >          size_t *passphrase_len,
> > >          Error **errp)
> > >  {
> > > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > >  static int qemu_rbd_convert_luks_create_options(
> > >          RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > >          rbd_encryption_algorithm_t *alg,
> > > -        char **passphrase,
> > > +        const char **passphrase,
> > >          size_t *passphrase_len,
> > >          Error **errp)
> > >  {
> > > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > >                                        Error **errp)
> > >  {
> > >      int r = 0;
> > > -    g_autofree char *passphrase = NULL;
> > > -    size_t passphrase_len;
> > > +    g_autofree const char *passphrase = NULL;
> >
> > This looks wierd.  If it is as const string, why are
> > we free'ing it ?  Either want g_autofree, or const,
> > but not both.
> 
> Just curious, is it a requirement imposed by g_autofree?  Otherwise
> pointer constness and pointee lifetime are completely orthogonal and
> freeing (or, in this case, wanting to auto-free) an object referred to
> by a const pointer seems perfectly fine to me.

Free'ing a const point is not OK

$ cat c.c
#include <stdlib.h>
void bar(const char *foo) {
    free(foo);
}

$ gcc -Wall -c c.c
c.c: In function ‘bar’:
c.c:5:10: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    5 |     free(foo);
      |          ^~~
In file included from c.c:2:
/usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of type ‘const char *’
  568 | extern void free (void *__ptr) __THROW;
      |                   ~~~~~~^~~~~


The g_autofree happens to end up hiding this warning, because the const
annotation isn't propagated to the registere callback, but that doesn't
mean we should do that.

When a programmer sees a variable annotated const, they expect that
either someone else is responsible for free'ing it, or that the data
is statically initialized or stack allocated and thus doesn't need
free'ing.  So g_autofree + const is just wrong.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 1/3] block/rbd: encryption nit fixes

[Ilya Dryomov]

On Thu, Jan 12, 2023 at 3:46 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jan 12, 2023 at 03:26:56PM +0100, Ilya Dryomov wrote:
> > On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > > > Add const modifier to passphrases,
> > > > and remove redundant stack variable passphrase_len.
> > > >
> > > > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > > > ---
> > > >  block/rbd.c | 24 ++++++++++--------------
> > > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/block/rbd.c b/block/rbd.c
> > > > index f826410f40..e575105e6d 100644
> > > > --- a/block/rbd.c
> > > > +++ b/block/rbd.c
> > > > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > > >  #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > > >  static int qemu_rbd_convert_luks_options(
> > > >          RbdEncryptionOptionsLUKSBase *luks_opts,
> > > > -        char **passphrase,
> > > > +        const char **passphrase,
> > > >          size_t *passphrase_len,
> > > >          Error **errp)
> > > >  {
> > > > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > > >  static int qemu_rbd_convert_luks_create_options(
> > > >          RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > > >          rbd_encryption_algorithm_t *alg,
> > > > -        char **passphrase,
> > > > +        const char **passphrase,
> > > >          size_t *passphrase_len,
> > > >          Error **errp)
> > > >  {
> > > > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > > >                                        Error **errp)
> > > >  {
> > > >      int r = 0;
> > > > -    g_autofree char *passphrase = NULL;
> > > > -    size_t passphrase_len;
> > > > +    g_autofree const char *passphrase = NULL;
> > >
> > > This looks wierd.  If it is as const string, why are
> > > we free'ing it ?  Either want g_autofree, or const,
> > > but not both.
> >
> > Just curious, is it a requirement imposed by g_autofree?  Otherwise
> > pointer constness and pointee lifetime are completely orthogonal and
> > freeing (or, in this case, wanting to auto-free) an object referred to
> > by a const pointer seems perfectly fine to me.
>
> Free'ing a const point is not OK
>
> $ cat c.c
> #include <stdlib.h>
> void bar(const char *foo) {
>     free(foo);
> }
>
> $ gcc -Wall -c c.c
> c.c: In function ‘bar’:
> c.c:5:10: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>     5 |     free(foo);
>       |          ^~~
> In file included from c.c:2:
> /usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of type ‘const char *’
>   568 | extern void free (void *__ptr) __THROW;
>       |                   ~~~~~~^~~~~
>
> The g_autofree happens to end up hiding this warning, because the const
> annotation isn't propagated to the registere callback, but that doesn't
> mean we should do that.
>
> When a programmer sees a variable annotated const, they expect that
> either someone else is responsible for free'ing it, or that the data
> is statically initialized or stack allocated and thus doesn't need
> free'ing.  So g_autofree + const is just wrong.

FWIW many believe that this specification of free() was a mistake and
that it should have been specified to take const void *.  Some projects
actually went ahead and fixed that: kfree() and friends in the Linux
kernel take const void *, for example.  C++ delete operator works on
const pointers as well -- because object creation and destruction is
fundamentally independent of modification.

But this is more of a philosophical thing...  I asked about g_autofree
because a quick grep revealed a bunch of g_autofree const char * locals
in the tree.  Or would probably prefer to just drop const here ;)

Thanks,

                Ilya

Re: [PATCH v4 1/3] block/rbd: encryption nit fixes

[Daniel P. Berrangé]

On Thu, Jan 12, 2023 at 06:07:58PM +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2023 at 3:46 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jan 12, 2023 at 03:26:56PM +0100, Ilya Dryomov wrote:
> > > On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > > > > Add const modifier to passphrases,
> > > > > and remove redundant stack variable passphrase_len.
> > > > >
> > > > > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > > > > ---
> > > > >  block/rbd.c | 24 ++++++++++--------------
> > > > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/block/rbd.c b/block/rbd.c
> > > > > index f826410f40..e575105e6d 100644
> > > > > --- a/block/rbd.c
> > > > > +++ b/block/rbd.c
> > > > > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > > > >  #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > > > >  static int qemu_rbd_convert_luks_options(
> > > > >          RbdEncryptionOptionsLUKSBase *luks_opts,
> > > > > -        char **passphrase,
> > > > > +        const char **passphrase,
> > > > >          size_t *passphrase_len,
> > > > >          Error **errp)
> > > > >  {
> > > > > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > > > >  static int qemu_rbd_convert_luks_create_options(
> > > > >          RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > > > >          rbd_encryption_algorithm_t *alg,
> > > > > -        char **passphrase,
> > > > > +        const char **passphrase,
> > > > >          size_t *passphrase_len,
> > > > >          Error **errp)
> > > > >  {
> > > > > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > > > >                                        Error **errp)
> > > > >  {
> > > > >      int r = 0;
> > > > > -    g_autofree char *passphrase = NULL;
> > > > > -    size_t passphrase_len;
> > > > > +    g_autofree const char *passphrase = NULL;
> > > >
> > > > This looks wierd.  If it is as const string, why are
> > > > we free'ing it ?  Either want g_autofree, or const,
> > > > but not both.
> > >
> > > Just curious, is it a requirement imposed by g_autofree?  Otherwise
> > > pointer constness and pointee lifetime are completely orthogonal and
> > > freeing (or, in this case, wanting to auto-free) an object referred to
> > > by a const pointer seems perfectly fine to me.
> >
> > Free'ing a const point is not OK
> >
> > $ cat c.c
> > #include <stdlib.h>
> > void bar(const char *foo) {
> >     free(foo);
> > }
> >
> > $ gcc -Wall -c c.c
> > c.c: In function ‘bar’:
> > c.c:5:10: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >     5 |     free(foo);
> >       |          ^~~
> > In file included from c.c:2:
> > /usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of type ‘const char *’
> >   568 | extern void free (void *__ptr) __THROW;
> >       |                   ~~~~~~^~~~~
> >
> > The g_autofree happens to end up hiding this warning, because the const
> > annotation isn't propagated to the registere callback, but that doesn't
> > mean we should do that.
> >
> > When a programmer sees a variable annotated const, they expect that
> > either someone else is responsible for free'ing it, or that the data
> > is statically initialized or stack allocated and thus doesn't need
> > free'ing.  So g_autofree + const is just wrong.
> 
> FWIW many believe that this specification of free() was a mistake and
> that it should have been specified to take const void *.  Some projects
> actually went ahead and fixed that: kfree() and friends in the Linux
> kernel take const void *, for example.  C++ delete operator works on
> const pointers as well -- because object creation and destruction is
> fundamentally independent of modification.

I'd really not like that as IMHO seeing the 'const' gives an important
hint to developers as to who is responsible for the releasing the pointer

> But this is more of a philosophical thing...  I asked about g_autofree
> because a quick grep revealed a bunch of g_autofree const char * locals
> in the tree.  Or would probably prefer to just drop const here ;)

IMHO those existing cases are all bugs that we should fix, along with
adding a rule to checkpatch.pl to detect this mistake.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|