[RFC PATCH 0/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[RFC PATCH 0/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[Philippe Mathieu-Daudé]

Use g_autoptr to free rdma_cm_event to avoid missing to use
the not so obvious rdma_ack_cm_event() instead:
https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg00511.html

Li Zhijian (1):
  migration/rdma: Fix cm event use after free

Philippe Mathieu-Daudé (1):
  migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

 migration/rdma.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

-- 
2.26.3

[PATCH 1/2] migration/rdma: Fix cm event use after free

[Philippe Mathieu-Daudé]

From: Li Zhijian <lizhijian@cn.fujitsu.com>

Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20210602023506.3821293-1-lizhijian@cn.fujitsu.com>
[PMD: Do not add missing rdma_ack_cm_event() calls]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 migration/rdma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index 1cdb4561f32..b50ebb9183a 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1539,8 +1539,10 @@ static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
 
                 if (pfds[1].revents) {
                     ret = rdma_get_cm_event(rdma->channel, &cm_event);
-                    if (!ret) {
-                        rdma_ack_cm_event(cm_event);
+                    if (ret) {
+                        error_report("failed to get cm event while wait "
+                                     "completion channel");
+                        return -EPIPE;
                     }
 
                     error_report("receive cm event while wait comp channel,"
-- 
2.26.3

[RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[Philippe Mathieu-Daudé]

Since 00f2cfbbec6 ("glib: bump min required glib library version to
2.48") we can use g_auto/g_autoptr to have the compiler automatically
free an allocated variable when it goes out of scope, removing this
burden on the developers.

Per rdma_cm(7) and rdma_ack_cm_event(3) man pages:

  "rdma_ack_cm_event() - Free a communication event.

   All events which are allocated by rdma_get_cm_event() must be
   released, there should be a one-to-one correspondence between
   successful gets and acks. This call frees the event structure
   and any memory that it references."

Since the 'ack' description doesn't explicit the event is also
released (free'd), it is safer to use the GLib g_autoptr feature.
The G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro expects a single word
for the type name, so add a type definition to achieve this.
Convert to use g_autoptr and remove the rdma_ack_cm_event() calls.

Inspired-by: Li Zhijian <lizhijian@cn.fujitsu.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
RFC: build-tested only
---
 migration/rdma.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/migration/rdma.c b/migration/rdma.c
index b50ebb9183a..b703bf1b918 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -38,6 +38,9 @@
 #include "qom/object.h"
 #include <poll.h>
 
+typedef struct rdma_cm_event rdma_cm_event;
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(rdma_cm_event, rdma_ack_cm_event)
+
 /*
  * Print and error on both the Monitor and the Log file.
  */
@@ -939,7 +942,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error **errp)
     int ret;
     struct rdma_addrinfo *res;
     char port_str[16];
-    struct rdma_cm_event *cm_event;
+    g_autoptr(rdma_cm_event) cm_event = NULL;
     char ip[40] = "unknown";
     struct rdma_addrinfo *e;
 
@@ -1007,11 +1010,11 @@ route:
         ERROR(errp, "result not equal to event_addr_resolved %s",
                 rdma_event_str(cm_event->event));
         perror("rdma_resolve_addr");
-        rdma_ack_cm_event(cm_event);
         ret = -EINVAL;
         goto err_resolve_get_addr;
     }
     rdma_ack_cm_event(cm_event);
+    cm_event = NULL;
 
     /* resolve route */
     ret = rdma_resolve_route(rdma->cm_id, RDMA_RESOLVE_TIMEOUT_MS);
@@ -1028,11 +1031,9 @@ route:
     if (cm_event->event != RDMA_CM_EVENT_ROUTE_RESOLVED) {
         ERROR(errp, "result not equal to event_route_resolved: %s",
                         rdma_event_str(cm_event->event));
-        rdma_ack_cm_event(cm_event);
         ret = -EINVAL;
         goto err_resolve_get_addr;
     }
-    rdma_ack_cm_event(cm_event);
     rdma->verbs = rdma->cm_id->verbs;
     qemu_rdma_dump_id("source_resolve_host", rdma->cm_id->verbs);
     qemu_rdma_dump_gid("source_resolve_host", rdma->cm_id);
@@ -1501,7 +1502,7 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
  */
 static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
 {
-    struct rdma_cm_event *cm_event;
+    g_autoptr(rdma_cm_event) cm_event = NULL;
     int ret = -1;
 
     /*
@@ -2503,7 +2504,7 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
                                           .private_data = &cap,
                                           .private_data_len = sizeof(cap),
                                         };
-    struct rdma_cm_event *cm_event;
+    g_autoptr(rdma_cm_event) cm_event = NULL;
     int ret;
 
     /*
@@ -2544,7 +2545,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
     if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
         perror("rdma_get_cm_event != EVENT_ESTABLISHED after rdma_connect");
         ERROR(errp, "connecting to destination!");
-        rdma_ack_cm_event(cm_event);
         goto err_rdma_source_connect;
     }
     rdma->connected = true;
@@ -2564,8 +2564,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
 
     trace_qemu_rdma_connect_pin_all_outcome(rdma->pin_all);
 
-    rdma_ack_cm_event(cm_event);
-
     rdma->control_ready_expected = 1;
     rdma->nb_sent = 0;
     return 0;
@@ -3279,7 +3277,7 @@ static void rdma_cm_poll_handler(void *opaque)
 {
     RDMAContext *rdma = opaque;
     int ret;
-    struct rdma_cm_event *cm_event;
+    g_autoptr(rdma_cm_event) cm_event = NULL;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
     ret = rdma_get_cm_event(rdma->channel, &cm_event);
@@ -3287,7 +3285,6 @@ static void rdma_cm_poll_handler(void *opaque)
         error_report("get_cm_event failed %d", errno);
         return;
     }
-    rdma_ack_cm_event(cm_event);
 
     if (cm_event->event == RDMA_CM_EVENT_DISCONNECTED ||
         cm_event->event == RDMA_CM_EVENT_DEVICE_REMOVAL) {
@@ -3317,7 +3314,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
                                             .private_data_len = sizeof(cap),
                                          };
     RDMAContext *rdma_return_path = NULL;
-    struct rdma_cm_event *cm_event;
+    g_autoptr(rdma_cm_event) cm_event = NULL;
     struct ibv_context *verbs;
     int ret = -EINVAL;
     int idx;
@@ -3328,7 +3325,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
     }
 
     if (cm_event->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
-        rdma_ack_cm_event(cm_event);
         goto err_rdma_dest_wait;
     }
 
@@ -3339,7 +3335,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
     if (migrate_postcopy() && !rdma->is_return_path) {
         rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
         if (rdma_return_path == NULL) {
-            rdma_ack_cm_event(cm_event);
             goto err_rdma_dest_wait;
         }
 
@@ -3353,7 +3348,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
     if (cap.version < 1 || cap.version > RDMA_CONTROL_VERSION_CURRENT) {
             error_report("Unknown source RDMA version: %d, bailing...",
                             cap.version);
-            rdma_ack_cm_event(cm_event);
             goto err_rdma_dest_wait;
     }
 
@@ -3374,6 +3368,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
     verbs = cm_event->id->verbs;
 
     rdma_ack_cm_event(cm_event);
+    cm_event = NULL;
 
     trace_qemu_rdma_accept_pin_state(rdma->pin_all);
 
@@ -3441,11 +3436,9 @@ static int qemu_rdma_accept(RDMAContext *rdma)
 
     if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
         error_report("rdma_accept not event established");
-        rdma_ack_cm_event(cm_event);
         goto err_rdma_dest_wait;
     }
 
-    rdma_ack_cm_event(cm_event);
     rdma->connected = true;
 
     ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
-- 
2.26.3

Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[lizhijian]

On 03/06/2021 01.51, Philippe Mathieu-Daudé wrote:
> Since 00f2cfbbec6 ("glib: bump min required glib library version to
> 2.48") we can use g_auto/g_autoptr to have the compiler automatically
> free an allocated variable when it goes out of scope,
Glad to know this feature.

However per its code, a  'ack' does much more than just free the memory.
not sure g_autoptr have the ability to do the same.

2212 static void ucma_complete_event(struct cma_id_private *id_priv)
2213 {
2214 pthread_mutex_lock(&id_priv->mut);
2215 id_priv->events_completed++;
2216 pthread_cond_signal(&id_priv->cond);
2217 pthread_mutex_unlock(&id_priv->mut);
2218 }
2219
2220 static void ucma_complete_mc_event(struct cma_multicast *mc)
2221 {
2222 pthread_mutex_lock(&mc->id_priv->mut);
2223 mc->events_completed++;
2224 pthread_cond_signal(&mc->cond);
2225 mc->id_priv->events_completed++;
2226 pthread_cond_signal(&mc->id_priv->cond);
2227 pthread_mutex_unlock(&mc->id_priv->mut);
2228 }
2229
2230 int rdma_ack_cm_event(struct rdma_cm_event *event)
2231 {
2232     struct cma_event *evt;
2233
2234     if (!event)
2235         return ERR(EINVAL);
2236
2237     evt = container_of(event, struct cma_event, event);
2238
2239     if (evt->mc)
2240 ucma_complete_mc_event(evt->mc);
2241 else
2242 ucma_complete_event(evt->id_priv);
2243 free(evt);
2244     return 0;
2245 }

Thanks
Zhijian

> removing this
> burden on the developers.
>
> Per rdma_cm(7) and rdma_ack_cm_event(3) man pages:
>
>    "rdma_ack_cm_event() - Free a communication event.
>
>     All events which are allocated by rdma_get_cm_event() must be
>     released, there should be a one-to-one correspondence between
>     successful gets and acks. This call frees the event structure
>     and any memory that it references."
>
> Since the 'ack' description doesn't explicit the event is also
> released (free'd), it is safer to use the GLib g_autoptr feature.
> The G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro expects a single word
> for the type name, so add a type definition to achieve this.
> Convert to use g_autoptr and remove the rdma_ack_cm_event() calls.
>
> Inspired-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> RFC: build-tested only
> ---
>   migration/rdma.c | 27 ++++++++++-----------------
>   1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index b50ebb9183a..b703bf1b918 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -38,6 +38,9 @@
>   #include "qom/object.h"
>   #include <poll.h>
>   
> +typedef struct rdma_cm_event rdma_cm_event;
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(rdma_cm_event, rdma_ack_cm_event)
> +
>   /*
>    * Print and error on both the Monitor and the Log file.
>    */
> @@ -939,7 +942,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error **errp)
>       int ret;
>       struct rdma_addrinfo *res;
>       char port_str[16];
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>       char ip[40] = "unknown";
>       struct rdma_addrinfo *e;
>   
> @@ -1007,11 +1010,11 @@ route:
>           ERROR(errp, "result not equal to event_addr_resolved %s",
>                   rdma_event_str(cm_event->event));
>           perror("rdma_resolve_addr");
> -        rdma_ack_cm_event(cm_event);
>           ret = -EINVAL;
>           goto err_resolve_get_addr;
>       }
>       rdma_ack_cm_event(cm_event);
> +    cm_event = NULL;
>   
>       /* resolve route */
>       ret = rdma_resolve_route(rdma->cm_id, RDMA_RESOLVE_TIMEOUT_MS);
> @@ -1028,11 +1031,9 @@ route:
>       if (cm_event->event != RDMA_CM_EVENT_ROUTE_RESOLVED) {
>           ERROR(errp, "result not equal to event_route_resolved: %s",
>                           rdma_event_str(cm_event->event));
> -        rdma_ack_cm_event(cm_event);
>           ret = -EINVAL;
>           goto err_resolve_get_addr;
>       }
> -    rdma_ack_cm_event(cm_event);
>       rdma->verbs = rdma->cm_id->verbs;
>       qemu_rdma_dump_id("source_resolve_host", rdma->cm_id->verbs);
>       qemu_rdma_dump_gid("source_resolve_host", rdma->cm_id);
> @@ -1501,7 +1502,7 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
>    */
>   static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
>   {
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>       int ret = -1;
>   
>       /*
> @@ -2503,7 +2504,7 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
>                                             .private_data = &cap,
>                                             .private_data_len = sizeof(cap),
>                                           };
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>       int ret;
>   
>       /*
> @@ -2544,7 +2545,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
>       if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
>           perror("rdma_get_cm_event != EVENT_ESTABLISHED after rdma_connect");
>           ERROR(errp, "connecting to destination!");
> -        rdma_ack_cm_event(cm_event);
>           goto err_rdma_source_connect;
>       }
>       rdma->connected = true;
> @@ -2564,8 +2564,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
>   
>       trace_qemu_rdma_connect_pin_all_outcome(rdma->pin_all);
>   
> -    rdma_ack_cm_event(cm_event);
> -
>       rdma->control_ready_expected = 1;
>       rdma->nb_sent = 0;
>       return 0;
> @@ -3279,7 +3277,7 @@ static void rdma_cm_poll_handler(void *opaque)
>   {
>       RDMAContext *rdma = opaque;
>       int ret;
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>       MigrationIncomingState *mis = migration_incoming_get_current();
>   
>       ret = rdma_get_cm_event(rdma->channel, &cm_event);
> @@ -3287,7 +3285,6 @@ static void rdma_cm_poll_handler(void *opaque)
>           error_report("get_cm_event failed %d", errno);
>           return;
>       }
> -    rdma_ack_cm_event(cm_event);
>   
>       if (cm_event->event == RDMA_CM_EVENT_DISCONNECTED ||
>           cm_event->event == RDMA_CM_EVENT_DEVICE_REMOVAL) {
> @@ -3317,7 +3314,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>                                               .private_data_len = sizeof(cap),
>                                            };
>       RDMAContext *rdma_return_path = NULL;
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>       struct ibv_context *verbs;
>       int ret = -EINVAL;
>       int idx;
> @@ -3328,7 +3325,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>       }
>   
>       if (cm_event->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
> -        rdma_ack_cm_event(cm_event);
>           goto err_rdma_dest_wait;
>       }
>   
> @@ -3339,7 +3335,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>       if (migrate_postcopy() && !rdma->is_return_path) {
>           rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
>           if (rdma_return_path == NULL) {
> -            rdma_ack_cm_event(cm_event);
>               goto err_rdma_dest_wait;
>           }
>   
> @@ -3353,7 +3348,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>       if (cap.version < 1 || cap.version > RDMA_CONTROL_VERSION_CURRENT) {
>               error_report("Unknown source RDMA version: %d, bailing...",
>                               cap.version);
> -            rdma_ack_cm_event(cm_event);
>               goto err_rdma_dest_wait;
>       }
>   
> @@ -3374,6 +3368,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>       verbs = cm_event->id->verbs;
>   
>       rdma_ack_cm_event(cm_event);
> +    cm_event = NULL;
>   
>       trace_qemu_rdma_accept_pin_state(rdma->pin_all);
>   
> @@ -3441,11 +3436,9 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>   
>       if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
>           error_report("rdma_accept not event established");
> -        rdma_ack_cm_event(cm_event);
>           goto err_rdma_dest_wait;
>       }
>   
> -    rdma_ack_cm_event(cm_event);
>       rdma->connected = true;
>   
>       ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);

Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[Philippe Mathieu-Daudé]

On 6/3/21 3:34 AM, lizhijian@fujitsu.com wrote:
> 
> 
> On 03/06/2021 01.51, Philippe Mathieu-Daudé wrote:
>> Since 00f2cfbbec6 ("glib: bump min required glib library version to
>> 2.48") we can use g_auto/g_autoptr to have the compiler automatically
>> free an allocated variable when it goes out of scope,
> Glad to know this feature.
> 
> However per its code, a  'ack' does much more than just free the memory.
> not sure g_autoptr have the ability to do the same.

See
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#G-DEFINE-AUTOPTR-CLEANUP-FUNC:CAPS

  Defines the appropriate cleanup function for a pointer type.

  The function will not be called if the variable to be cleaned
  up contains NULL.

  This will typically be the _free() or _unref() function for
  the given type.

This does not change the code to call free(ptr), but to call the
registered cleanup function, which is rdma_ack_cm_event().

> 
> 2212 static void ucma_complete_event(struct cma_id_private *id_priv)
> 2213 {
> 2214 pthread_mutex_lock(&id_priv->mut);
> 2215 id_priv->events_completed++;
> 2216 pthread_cond_signal(&id_priv->cond);
> 2217 pthread_mutex_unlock(&id_priv->mut);
> 2218 }
> 2219
> 2220 static void ucma_complete_mc_event(struct cma_multicast *mc)
> 2221 {
> 2222 pthread_mutex_lock(&mc->id_priv->mut);
> 2223 mc->events_completed++;
> 2224 pthread_cond_signal(&mc->cond);
> 2225 mc->id_priv->events_completed++;
> 2226 pthread_cond_signal(&mc->id_priv->cond);
> 2227 pthread_mutex_unlock(&mc->id_priv->mut);
> 2228 }
> 2229
> 2230 int rdma_ack_cm_event(struct rdma_cm_event *event)
> 2231 {
> 2232     struct cma_event *evt;
> 2233
> 2234     if (!event)
> 2235         return ERR(EINVAL);
> 2236
> 2237     evt = container_of(event, struct cma_event, event);
> 2238
> 2239     if (evt->mc)
> 2240 ucma_complete_mc_event(evt->mc);
> 2241 else
> 2242 ucma_complete_event(evt->id_priv);
> 2243 free(evt);
> 2244     return 0;
> 2245 }
> 
> Thanks
> Zhijian
> 
>> removing this
>> burden on the developers.
>>
>> Per rdma_cm(7) and rdma_ack_cm_event(3) man pages:
>>
>>    "rdma_ack_cm_event() - Free a communication event.
>>
>>     All events which are allocated by rdma_get_cm_event() must be
>>     released, there should be a one-to-one correspondence between
>>     successful gets and acks. This call frees the event structure
>>     and any memory that it references."
>>
>> Since the 'ack' description doesn't explicit the event is also
>> released (free'd), it is safer to use the GLib g_autoptr feature.
>> The G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro expects a single word
>> for the type name, so add a type definition to achieve this.
>> Convert to use g_autoptr and remove the rdma_ack_cm_event() calls.
>>
>> Inspired-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> RFC: build-tested only
>> ---
>>   migration/rdma.c | 27 ++++++++++-----------------
>>   1 file changed, 10 insertions(+), 17 deletions(-)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index b50ebb9183a..b703bf1b918 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -38,6 +38,9 @@
>>   #include "qom/object.h"
>>   #include <poll.h>
>>   
>> +typedef struct rdma_cm_event rdma_cm_event;
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(rdma_cm_event, rdma_ack_cm_event)
>> +
>>   /*
>>    * Print and error on both the Monitor and the Log file.
>>    */
>> @@ -939,7 +942,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error **errp)
>>       int ret;
>>       struct rdma_addrinfo *res;
>>       char port_str[16];
>> -    struct rdma_cm_event *cm_event;
>> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>>       char ip[40] = "unknown";
>>       struct rdma_addrinfo *e;
>>   
>> @@ -1007,11 +1010,11 @@ route:
>>           ERROR(errp, "result not equal to event_addr_resolved %s",
>>                   rdma_event_str(cm_event->event));
>>           perror("rdma_resolve_addr");
>> -        rdma_ack_cm_event(cm_event);
>>           ret = -EINVAL;
>>           goto err_resolve_get_addr;
>>       }

Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[lizhijian]

On 03/06/2021 17.30, Philippe Mathieu-Daudé wrote:
> On 6/3/21 3:34 AM, lizhijian@fujitsu.com wrote:
>>
>> On 03/06/2021 01.51, Philippe Mathieu-Daudé wrote:
>>> Since 00f2cfbbec6 ("glib: bump min required glib library version to
>>> 2.48") we can use g_auto/g_autoptr to have the compiler automatically
>>> free an allocated variable when it goes out of scope,
>> Glad to know this feature.
>>
>> However per its code, a  'ack' does much more than just free the memory.
>> not sure g_autoptr have the ability to do the same.
> See
> https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#G-DEFINE-AUTOPTR-CLEANUP-FUNC:CAPS
>
>    Defines the appropriate cleanup function for a pointer type.
>
>    The function will not be called if the variable to be cleaned
>    up contains NULL.
>
>    This will typically be the _free() or _unref() function for
>    the given type.
>
> This does not change the code to call free(ptr), but to call the
> registered cleanup function, which is rdma_ack_cm_event().
*

Thanks for your explanation.

Tested-by: Li Zhijian <lizhijian@cn.fujitsu.com>

*


>
>> 2212 static void ucma_complete_event(struct cma_id_private *id_priv)
>> 2213 {
>> 2214 pthread_mutex_lock(&id_priv->mut);
>> 2215 id_priv->events_completed++;
>> 2216 pthread_cond_signal(&id_priv->cond);
>> 2217 pthread_mutex_unlock(&id_priv->mut);
>> 2218 }
>> 2219
>> 2220 static void ucma_complete_mc_event(struct cma_multicast *mc)
>> 2221 {
>> 2222 pthread_mutex_lock(&mc->id_priv->mut);
>> 2223 mc->events_completed++;
>> 2224 pthread_cond_signal(&mc->cond);
>> 2225 mc->id_priv->events_completed++;
>> 2226 pthread_cond_signal(&mc->id_priv->cond);
>> 2227 pthread_mutex_unlock(&mc->id_priv->mut);
>> 2228 }
>> 2229
>> 2230 int rdma_ack_cm_event(struct rdma_cm_event *event)
>> 2231 {
>> 2232     struct cma_event *evt;
>> 2233
>> 2234     if (!event)
>> 2235         return ERR(EINVAL);
>> 2236
>> 2237     evt = container_of(event, struct cma_event, event);
>> 2238
>> 2239     if (evt->mc)
>> 2240 ucma_complete_mc_event(evt->mc);
>> 2241 else
>> 2242 ucma_complete_event(evt->id_priv);
>> 2243 free(evt);
>> 2244     return 0;
>> 2245 }
>>
>> Thanks
>> Zhijian
>>
>>> removing this
>>> burden on the developers.
>>>
>>> Per rdma_cm(7) and rdma_ack_cm_event(3) man pages:
>>>
>>>     "rdma_ack_cm_event() - Free a communication event.
>>>
>>>      All events which are allocated by rdma_get_cm_event() must be
>>>      released, there should be a one-to-one correspondence between
>>>      successful gets and acks. This call frees the event structure
>>>      and any memory that it references."
>>>
>>> Since the 'ack' description doesn't explicit the event is also
>>> released (free'd), it is safer to use the GLib g_autoptr feature.
>>> The G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro expects a single word
>>> for the type name, so add a type definition to achieve this.
>>> Convert to use g_autoptr and remove the rdma_ack_cm_event() calls.
>>>
>>> Inspired-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> RFC: build-tested only
>>> ---
>>>    migration/rdma.c | 27 ++++++++++-----------------
>>>    1 file changed, 10 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>> index b50ebb9183a..b703bf1b918 100644
>>> --- a/migration/rdma.c
>>> +++ b/migration/rdma.c
>>> @@ -38,6 +38,9 @@
>>>    #include "qom/object.h"
>>>    #include <poll.h>
>>>    
>>> +typedef struct rdma_cm_event rdma_cm_event;
>>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(rdma_cm_event, rdma_ack_cm_event)
>>> +
>>>    /*
>>>     * Print and error on both the Monitor and the Log file.
>>>     */
>>> @@ -939,7 +942,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error **errp)
>>>        int ret;
>>>        struct rdma_addrinfo *res;
>>>        char port_str[16];
>>> -    struct rdma_cm_event *cm_event;
>>> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>>>        char ip[40] = "unknown";
>>>        struct rdma_addrinfo *e;
>>>    
>>> @@ -1007,11 +1010,11 @@ route:
>>>            ERROR(errp, "result not equal to event_addr_resolved %s",
>>>                    rdma_event_str(cm_event->event));
>>>            perror("rdma_resolve_addr");
>>> -        rdma_ack_cm_event(cm_event);
>>>            ret = -EINVAL;
>>>            goto err_resolve_get_addr;
>>>        }

Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> Since 00f2cfbbec6 ("glib: bump min required glib library version to
> 2.48") we can use g_auto/g_autoptr to have the compiler automatically
> free an allocated variable when it goes out of scope, removing this
> burden on the developers.
> 
> Per rdma_cm(7) and rdma_ack_cm_event(3) man pages:
> 
>   "rdma_ack_cm_event() - Free a communication event.
> 
>    All events which are allocated by rdma_get_cm_event() must be
>    released, there should be a one-to-one correspondence between
>    successful gets and acks. This call frees the event structure
>    and any memory that it references."
> 
> Since the 'ack' description doesn't explicit the event is also
> released (free'd), it is safer to use the GLib g_autoptr feature.
> The G_DEFINE_AUTOPTR_CLEANUP_FUNC() macro expects a single word
> for the type name, so add a type definition to achieve this.
> Convert to use g_autoptr and remove the rdma_ack_cm_event() calls.
> 
> Inspired-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

It's unclear to me whether the changes in qemu_rdma_accept are legal
rdma or not.
If we look at the err_rdma_dest_wait: path, it does a
qemu_rdma_cleanup(rdma) before the exit; that does rdma_disconnect's and
calls loads of other rdma/ibv cleanup calls to destroy state - is there
any ordering requirement saying you're supposed to clean up your
cm_event's before you nuke the rest of the channel? It feels like you
probably should - but I have no idea if it's a requirement.

Dave

> ---
> RFC: build-tested only
> ---
>  migration/rdma.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index b50ebb9183a..b703bf1b918 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -38,6 +38,9 @@
>  #include "qom/object.h"
>  #include <poll.h>
>  
> +typedef struct rdma_cm_event rdma_cm_event;
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(rdma_cm_event, rdma_ack_cm_event)
> +
>  /*
>   * Print and error on both the Monitor and the Log file.
>   */
> @@ -939,7 +942,7 @@ static int qemu_rdma_resolve_host(RDMAContext *rdma, Error **errp)
>      int ret;
>      struct rdma_addrinfo *res;
>      char port_str[16];
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>      char ip[40] = "unknown";
>      struct rdma_addrinfo *e;
>  
> @@ -1007,11 +1010,11 @@ route:
>          ERROR(errp, "result not equal to event_addr_resolved %s",
>                  rdma_event_str(cm_event->event));
>          perror("rdma_resolve_addr");
> -        rdma_ack_cm_event(cm_event);
>          ret = -EINVAL;
>          goto err_resolve_get_addr;
>      }
>      rdma_ack_cm_event(cm_event);
> +    cm_event = NULL;
>  
>      /* resolve route */
>      ret = rdma_resolve_route(rdma->cm_id, RDMA_RESOLVE_TIMEOUT_MS);
> @@ -1028,11 +1031,9 @@ route:
>      if (cm_event->event != RDMA_CM_EVENT_ROUTE_RESOLVED) {
>          ERROR(errp, "result not equal to event_route_resolved: %s",
>                          rdma_event_str(cm_event->event));
> -        rdma_ack_cm_event(cm_event);
>          ret = -EINVAL;
>          goto err_resolve_get_addr;
>      }
> -    rdma_ack_cm_event(cm_event);
>      rdma->verbs = rdma->cm_id->verbs;
>      qemu_rdma_dump_id("source_resolve_host", rdma->cm_id->verbs);
>      qemu_rdma_dump_gid("source_resolve_host", rdma->cm_id);
> @@ -1501,7 +1502,7 @@ static uint64_t qemu_rdma_poll(RDMAContext *rdma, uint64_t *wr_id_out,
>   */
>  static int qemu_rdma_wait_comp_channel(RDMAContext *rdma)
>  {
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>      int ret = -1;
>  
>      /*
> @@ -2503,7 +2504,7 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
>                                            .private_data = &cap,
>                                            .private_data_len = sizeof(cap),
>                                          };
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>      int ret;
>  
>      /*
> @@ -2544,7 +2545,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
>      if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
>          perror("rdma_get_cm_event != EVENT_ESTABLISHED after rdma_connect");
>          ERROR(errp, "connecting to destination!");
> -        rdma_ack_cm_event(cm_event);
>          goto err_rdma_source_connect;
>      }
>      rdma->connected = true;
> @@ -2564,8 +2564,6 @@ static int qemu_rdma_connect(RDMAContext *rdma, Error **errp, bool return_path)
>  
>      trace_qemu_rdma_connect_pin_all_outcome(rdma->pin_all);
>  
> -    rdma_ack_cm_event(cm_event);
> -
>      rdma->control_ready_expected = 1;
>      rdma->nb_sent = 0;
>      return 0;
> @@ -3279,7 +3277,7 @@ static void rdma_cm_poll_handler(void *opaque)
>  {
>      RDMAContext *rdma = opaque;
>      int ret;
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
>      ret = rdma_get_cm_event(rdma->channel, &cm_event);
> @@ -3287,7 +3285,6 @@ static void rdma_cm_poll_handler(void *opaque)
>          error_report("get_cm_event failed %d", errno);
>          return;
>      }
> -    rdma_ack_cm_event(cm_event);
>  
>      if (cm_event->event == RDMA_CM_EVENT_DISCONNECTED ||
>          cm_event->event == RDMA_CM_EVENT_DEVICE_REMOVAL) {
> @@ -3317,7 +3314,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>                                              .private_data_len = sizeof(cap),
>                                           };
>      RDMAContext *rdma_return_path = NULL;
> -    struct rdma_cm_event *cm_event;
> +    g_autoptr(rdma_cm_event) cm_event = NULL;
>      struct ibv_context *verbs;
>      int ret = -EINVAL;
>      int idx;
> @@ -3328,7 +3325,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>      }
>  
>      if (cm_event->event != RDMA_CM_EVENT_CONNECT_REQUEST) {
> -        rdma_ack_cm_event(cm_event);
>          goto err_rdma_dest_wait;
>      }
>  
> @@ -3339,7 +3335,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>      if (migrate_postcopy() && !rdma->is_return_path) {
>          rdma_return_path = qemu_rdma_data_init(rdma->host_port, NULL);
>          if (rdma_return_path == NULL) {
> -            rdma_ack_cm_event(cm_event);
>              goto err_rdma_dest_wait;
>          }
>  
> @@ -3353,7 +3348,6 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>      if (cap.version < 1 || cap.version > RDMA_CONTROL_VERSION_CURRENT) {
>              error_report("Unknown source RDMA version: %d, bailing...",
>                              cap.version);
> -            rdma_ack_cm_event(cm_event);
>              goto err_rdma_dest_wait;
>      }
>  
> @@ -3374,6 +3368,7 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>      verbs = cm_event->id->verbs;
>  
>      rdma_ack_cm_event(cm_event);
> +    cm_event = NULL;
>  
>      trace_qemu_rdma_accept_pin_state(rdma->pin_all);
>  
> @@ -3441,11 +3436,9 @@ static int qemu_rdma_accept(RDMAContext *rdma)
>  
>      if (cm_event->event != RDMA_CM_EVENT_ESTABLISHED) {
>          error_report("rdma_accept not event established");
> -        rdma_ack_cm_event(cm_event);
>          goto err_rdma_dest_wait;
>      }
>  
> -    rdma_ack_cm_event(cm_event);
>      rdma->connected = true;
>  
>      ret = qemu_rdma_post_recv_control(rdma, RDMA_WRID_READY);
> -- 
> 2.26.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK