* [RFC PATCH 0/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event @ 2021-06-02 17:51 Philippe Mathieu-Daudé 2021-06-02 17:51 ` [PATCH 1/2] migration/rdma: Fix cm event use after free Philippe Mathieu-Daudé 2021-06-02 17:51 ` [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé 0 siblings, 2 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2021-06-02 17:51 UTC (permalink / raw) To: qemu-devel Cc: Philippe Mathieu-Daudé, Dr. David Alan Gilbert, Li Zhijian, Juan Quintela 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] migration/rdma: Fix cm event use after free 2021-06-02 17:51 [RFC PATCH 0/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé @ 2021-06-02 17:51 ` Philippe Mathieu-Daudé 2021-06-02 17:51 ` [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé 1 sibling, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2021-06-02 17:51 UTC (permalink / raw) To: qemu-devel Cc: Philippe Mathieu-Daudé, Dr. David Alan Gilbert, Li Zhijian, Juan Quintela 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event 2021-06-02 17:51 [RFC PATCH 0/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé 2021-06-02 17:51 ` [PATCH 1/2] migration/rdma: Fix cm event use after free Philippe Mathieu-Daudé @ 2021-06-02 17:51 ` Philippe Mathieu-Daudé 2021-06-03 1:34 ` lizhijian 2021-06-03 17:51 ` Dr. David Alan Gilbert 1 sibling, 2 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2021-06-02 17:51 UTC (permalink / raw) To: qemu-devel Cc: Philippe Mathieu-Daudé, Dr. David Alan Gilbert, Li Zhijian, Juan Quintela 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event 2021-06-02 17:51 ` [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé @ 2021-06-03 1:34 ` lizhijian 2021-06-03 9:30 ` Philippe Mathieu-Daudé 2021-06-03 17:51 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 7+ messages in thread From: lizhijian @ 2021-06-03 1:34 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Dr. David Alan Gilbert, lizhijian, Juan Quintela 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event 2021-06-03 1:34 ` lizhijian @ 2021-06-03 9:30 ` Philippe Mathieu-Daudé 2021-06-03 12:57 ` lizhijian 0 siblings, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2021-06-03 9:30 UTC (permalink / raw) To: lizhijian, qemu-devel; +Cc: Dr. David Alan Gilbert, Juan Quintela 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; >> } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event 2021-06-03 9:30 ` Philippe Mathieu-Daudé @ 2021-06-03 12:57 ` lizhijian 0 siblings, 0 replies; 7+ messages in thread From: lizhijian @ 2021-06-03 12:57 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Dr. David Alan Gilbert, Juan Quintela 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; >>> } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event 2021-06-02 17:51 ` [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé 2021-06-03 1:34 ` lizhijian @ 2021-06-03 17:51 ` Dr. David Alan Gilbert 1 sibling, 0 replies; 7+ messages in thread From: Dr. David Alan Gilbert @ 2021-06-03 17:51 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: qemu-devel, Li Zhijian, Juan Quintela * 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-03 17:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-02 17:51 [RFC PATCH 0/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé 2021-06-02 17:51 ` [PATCH 1/2] migration/rdma: Fix cm event use after free Philippe Mathieu-Daudé 2021-06-02 17:51 ` [RFC PATCH 2/2] migration/rdma: Enable use of g_autoptr with struct rdma_cm_event Philippe Mathieu-Daudé 2021-06-03 1:34 ` lizhijian 2021-06-03 9:30 ` Philippe Mathieu-Daudé 2021-06-03 12:57 ` lizhijian 2021-06-03 17:51 ` Dr. David Alan Gilbert
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.