From edb6738816b442fb81f12ea0079260c96c3a5759 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 29 Nov 2011 17:09:27 -0500 Subject: [PATCH] Fix enable-event for UST events When enabling an existing event, a new event creation occured which was simply the bad thing to do. This patch fix that by adding the enable_ust_event call for the case. Also fixes a segfault where any UST object could be release with the data pointer being set to NULL. This patch introduce also assert() in the UST app code for create/enable/disable event. The purpose is to catch, in dev, the bad code execution flow and fix it. Signed-off-by: David Goulet --- lttng-sessiond/event.c | 76 +++++++++---- lttng-sessiond/event.h | 4 +- lttng-sessiond/main.c | 33 ++---- lttng-sessiond/ust-app.c | 223 ++++++++++++++++++++++++++++----------- lttng-sessiond/ust-app.h | 8 ++ 5 files changed, 235 insertions(+), 109 deletions(-) diff --git a/lttng-sessiond/event.c b/lttng-sessiond/event.c index 2d1fb6e68..407f8441c 100644 --- a/lttng-sessiond/event.c +++ b/lttng-sessiond/event.c @@ -28,6 +28,9 @@ #include "hashtable.h" #include "kernel.h" #include "ust-ctl.h" +#include "ust-app.h" +#include "trace-kernel.h" +#include "trace-ust.h" /* * Setup a lttng_event used to enable *all* syscall tracing. @@ -236,35 +239,66 @@ end: /* * Enable UST tracepoint event for a channel from a UST session. */ -#ifdef DISABLE -int event_ust_enable_tracepoint(struct ltt_ust_session *usess, - struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent) +int event_ust_enable_tracepoint(struct ltt_ust_session *usess, int domain, + struct ltt_ust_channel *uchan, struct lttng_event *event) { - int ret; - struct lttng_ust_event ltt_uevent; - struct object_data *obj_event; + int ret, to_create = 0; + struct ltt_ust_event *uevent; + + uevent = trace_ust_find_event_by_name(uchan->events, event->name); + if (uevent == NULL) { + uevent = trace_ust_create_event(event); + if (uevent == NULL) { + ret = LTTCOMM_FATAL; + goto error; + } + to_create = 1; + } - strncpy(ltt_uevent.name, uevent->attr.name, sizeof(ltt_uevent.name)); - ltt_uevent.name[sizeof(ltt_uevent.name) - 1] = '\0'; - /* TODO: adjust to other instrumentation types */ - ltt_uevent.instrumentation = LTTNG_UST_TRACEPOINT; + switch (domain) { + case LTTNG_DOMAIN_UST: + { + if (to_create) { + /* Create event on all UST registered apps for session */ + ret = ust_app_create_event_all(usess, uchan, uevent); + } else { + /* Enable event on all UST registered apps for session */ + ret = ust_app_enable_event_all(usess, uchan, uevent); + } - ret = ustctl_create_event(app->key.sock, <t_uevent, - uchan->obj, &obj_event); - if (ret < 0) { - DBG("Error ustctl create event %s for app pid: %d, sock: %d ret %d", - uevent->attr.name, app->key.pid, app->key.sock, ret); - goto next; + if (ret < 0) { + if (ret == -EEXIST) { + ret = LTTCOMM_UST_EVENT_EXIST; + } else { + ret = LTTCOMM_UST_ENABLE_FAIL; + } + goto error; + } + + DBG("Event UST %s added to channel %s", uevent->attr.name, + uchan->name); + break; + } + case LTTNG_DOMAIN_UST_EXEC_NAME: + case LTTNG_DOMAIN_UST_PID: + case LTTNG_DOMAIN_UST_PID_FOLLOW_CHILDREN: + default: + ret = LTTCOMM_NOT_IMPLEMENTED; + goto error; } - uevent->obj = obj_event; - uevent->handle = obj_event->handle; uevent->enabled = 1; - ret = LTTCOMM_OK; -end: + /* Add ltt ust event to channel */ + rcu_read_lock(); + hashtable_add_unique(uchan->events, &uevent->node); + rcu_read_unlock(); + + return LTTCOMM_OK; + +error: + trace_ust_destroy_event(uevent); return ret; } -#endif #ifdef DISABLE int event_ust_disable_tracepoint(struct ltt_ust_session *ustsession, diff --git a/lttng-sessiond/event.h b/lttng-sessiond/event.h index 2852b3535..2a7d8c2c3 100644 --- a/lttng-sessiond/event.h +++ b/lttng-sessiond/event.h @@ -40,8 +40,8 @@ int event_kernel_enable_all_syscalls(struct ltt_kernel_session *ksession, int event_kernel_enable_all(struct ltt_kernel_session *ksession, struct ltt_kernel_channel *kchan, int kernel_tracer_fd); -int event_ust_enable_tracepoint(struct ltt_ust_session *ustsession, - struct ltt_ust_channel *ustchan, struct ltt_ust_event *uevent); +int event_ust_enable_tracepoint(struct ltt_ust_session *usess, int domain, + struct ltt_ust_channel *uchan, struct lttng_event *event); int event_ust_disable_tracepoint(struct ltt_ust_session *ustsession, struct ltt_ust_channel *ustchan, char *event_name); diff --git a/lttng-sessiond/main.c b/lttng-sessiond/main.c index 6bb471a9d..63d7f0e2d 100644 --- a/lttng-sessiond/main.c +++ b/lttng-sessiond/main.c @@ -2580,10 +2580,10 @@ static int cmd_enable_event(struct ltt_session *session, int domain, } case LTTNG_DOMAIN_UST: { - struct ltt_ust_channel *uchan; - struct ltt_ust_event *uevent; struct lttng_channel *attr; + struct ltt_ust_channel *uchan; + /* Get channel from global UST domain */ uchan = trace_ust_find_channel_by_name(usess->domain_global.channels, channel_name); if (uchan == NULL) { @@ -2594,14 +2594,14 @@ static int cmd_enable_event(struct ltt_session *session, int domain, goto error; } snprintf(attr->name, NAME_MAX, "%s", channel_name); + attr->name[NAME_MAX - 1] = '\0'; /* Use the internal command enable channel */ ret = cmd_enable_channel(session, domain, attr); - if (ret < 0) { + if (ret != LTTCOMM_OK) { free(attr); goto error; } - free(attr); /* Get the newly created channel reference back */ @@ -2614,31 +2614,12 @@ static int cmd_enable_event(struct ltt_session *session, int domain, } } - uevent = trace_ust_find_event_by_name(uchan->events, event->name); - if (uevent == NULL) { - uevent = trace_ust_create_event(event); - if (uevent == NULL) { - ret = LTTCOMM_FATAL; - goto error; - } + /* At this point, the session and channel exist on the tracer */ - } - - ret = ust_app_create_event_all(usess, uchan, uevent); - if (ret < 0) { - ret = LTTCOMM_UST_ENABLE_FAIL; + ret = event_ust_enable_tracepoint(usess, domain, uchan, event); + if (ret != LTTCOMM_OK) { goto error; } - - /* Add ltt ust event to channel */ - rcu_read_lock(); - hashtable_add_unique(uchan->events, &uevent->node); - rcu_read_unlock(); - - uevent->enabled = 1; - - DBG3("UST ltt event %s added to channel %s", uevent->attr.name, - uchan->name); break; } case LTTNG_DOMAIN_UST_EXEC_NAME: diff --git a/lttng-sessiond/ust-app.c b/lttng-sessiond/ust-app.c index 51ae71c4f..30ac8ceaf 100644 --- a/lttng-sessiond/ust-app.c +++ b/lttng-sessiond/ust-app.c @@ -47,8 +47,10 @@ static void delete_ust_app_event(int sock, struct ust_app_event *ua_event) // delete_ust_app_ctx(sock, ltctx); //} - ustctl_release_object(sock, ua_event->obj); - free(ua_event->obj); + if (ua_event->obj != NULL) { + ustctl_release_object(sock, ua_event->obj); + free(ua_event->obj); + } free(ua_event); } @@ -96,8 +98,11 @@ static void delete_ust_app_channel(int sock, struct ust_app_channel *ua_chan) ERR("UST app destroy session hashtable failed"); goto error; } - ustctl_release_object(sock, ua_chan->obj); - free(ua_chan->obj); + + if (ua_chan->obj != NULL) { + ustctl_release_object(sock, ua_chan->obj); + free(ua_chan->obj); + } free(ua_chan); error: @@ -317,6 +322,29 @@ error: return ret; } +/* + * Enable the specified event on to UST tracer for the UST session. + */ +static int enable_ust_event(struct ust_app *app, + struct ust_app_session *ua_sess, struct ust_app_event *ua_event) +{ + int ret; + + ret = ustctl_enable(app->key.sock, ua_event->obj); + if (ret < 0) { + ERR("UST app event %s enable failed for app (pid: %d) " + "and session handle %d with ret %d", + ua_event->attr.name, app->key.pid, ua_sess->handle, ret); + goto error; + } + + DBG2("UST app event %s enabled successfully for app (pid: %d)", + ua_event->attr.name, app->key.pid); + +error: + return ret; +} + /* * Open metadata onto the UST tracer for a UST session. */ @@ -401,9 +429,9 @@ error: /* * Create the specified event onto the UST tracer for a UST session. */ -static int create_ust_event(struct ust_app *app, - struct ust_app_session *ua_sess, struct ust_app_channel *ua_chan, - struct ust_app_event *ua_event) +static +int create_ust_event(struct ust_app *app, struct ust_app_session *ua_sess, + struct ust_app_channel *ua_chan, struct ust_app_event *ua_event) { int ret = 0; @@ -727,6 +755,27 @@ error: return NULL; } +/* + * Enable on the tracer side a ust app event for the session and channel. + */ +static +int enable_ust_app_event(struct ust_app_session *ua_sess, + struct ust_app_channel *ua_chan, struct ust_app_event *ua_event, + struct ust_app *app) +{ + int ret; + + ret = enable_ust_event(app, ua_sess, ua_event); + if (ret < 0) { + goto error; + } + + ua_event->enabled = 1; + +error: + return ret; +} + /* * Disable on the tracer side a ust app event for the session and channel. */ @@ -850,11 +899,12 @@ error: /* * Create UST app event and create it on the tracer side. */ -static struct ust_app_event *create_ust_app_event( - struct ust_app_session *ua_sess, struct ust_app_channel *ua_chan, - struct ltt_ust_event *uevent, struct ust_app *app) +static +int create_ust_app_event(struct ust_app_session *ua_sess, + struct ust_app_channel *ua_chan, struct ltt_ust_event *uevent, + struct ust_app *app) { - int ret; + int ret = 0; struct cds_lfht_iter iter; struct cds_lfht_node *ua_event_node; struct ust_app_event *ua_event; @@ -862,30 +912,33 @@ static struct ust_app_event *create_ust_app_event( /* Get event node */ ua_event_node = hashtable_lookup(ua_chan->events, (void *)uevent->attr.name, strlen(uevent->attr.name), &iter); - if (ua_event_node == NULL) { - DBG2("UST app event %s not found, creating it", uevent->attr.name); - /* Does not exist so create one */ - ua_event = alloc_ust_app_event(uevent->attr.name, &uevent->attr); - if (ua_event == NULL) { - /* Only malloc can failed so something is really wrong */ - goto error; - } - shadow_copy_event(ua_event, uevent); + if (ua_event_node != NULL) { + ERR("UST app event %s already exist. Stopping creation.", + uevent->attr.name); + goto end; + } - hashtable_add_unique(ua_chan->events, &ua_event->node); - } else { - ua_event = caa_container_of(ua_event_node, struct ust_app_event, node); + /* Does not exist so create one */ + ua_event = alloc_ust_app_event(uevent->attr.name, &uevent->attr); + if (ua_event == NULL) { + /* Only malloc can failed so something is really wrong */ + ret = -ENOMEM; + goto error; } + shadow_copy_event(ua_event, uevent); + /* Create it on the tracer side */ ret = create_ust_event(app, ua_sess, ua_chan, ua_event); if (ret < 0) { + delete_ust_app_event(app->key.sock, ua_event); goto error; } - return ua_event; + hashtable_add_unique(ua_chan->events, &ua_event->node); +end: error: - return NULL; + return ret; } /* @@ -1327,7 +1380,7 @@ int ust_app_disable_event(struct ltt_ust_session *usess, } /* - * For a specific UST session and UST channel, create the event for all + * For a specific UST session and UST channel, the event for all * registered apps. */ int ust_app_disable_event_all(struct ltt_ust_session *usess, @@ -1349,20 +1402,15 @@ int ust_app_disable_event_all(struct ltt_ust_session *usess, /* For all registered applications */ cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) { ua_sess = lookup_session_by_app(usess, app); - if (ua_sess == NULL) { - /* Next app */ - continue; - } + /* If ua_sess is NULL, there is a code flow error */ + assert(ua_sess); /* Lookup channel in the ust app session */ - ua_chan_node = hashtable_lookup(ua_sess->channels, - (void *)uchan->name, strlen(uchan->name), - &uiter); - if (ua_chan_node == NULL) { - DBG2("Channel %s not found in session uid %d for app pid %d." - "Skipping", uchan->name, app->key.pid, usess->uid); - continue; - } + ua_chan_node = hashtable_lookup(ua_sess->channels, (void *)uchan->name, + strlen(uchan->name), &uiter); + /* If the channel is not found, there is a code flow error */ + assert(ua_chan_node); + ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node); /* Disable each events of channel */ @@ -1405,7 +1453,11 @@ int ust_app_create_channel_all(struct ltt_ust_session *usess, /* For every registered applications */ cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) { - /* Create session on the tracer side and add it to app session HT */ + /* + * Create session on the tracer side and add it to app session HT. Note + * that if session exist, it will simply return a pointer to the ust + * app session. + */ ua_sess = create_ust_app_session(usess, app); if (ua_sess == NULL) { continue; @@ -1425,48 +1477,100 @@ error: } /* - * For a specific UST session and UST channel, create the event for all - * registered apps. + * Enable event for a specific session and channel on the tracer. */ -int ust_app_create_event_all(struct ltt_ust_session *usess, +int ust_app_enable_event_all(struct ltt_ust_session *usess, struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent) { int ret = 0; - struct cds_lfht_iter iter; + struct cds_lfht_iter iter, uiter; struct cds_lfht_node *ua_chan_node; struct ust_app *app; struct ust_app_session *ua_sess; struct ust_app_channel *ua_chan; struct ust_app_event *ua_event; - DBG("UST app creating event %s for all apps for session uid %d", + DBG("UST app enabling event %s for all apps for session uid %d", uevent->attr.name, usess->uid); + /* + * NOTE: At this point, this function is called only if the session and + * channel passed are already created for all apps. and enabled on the + * tracer also. + */ + rcu_read_lock(); /* For all registered applications */ cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) { - struct cds_lfht_iter uiter; + ua_sess = lookup_session_by_app(usess, app); + /* If ua_sess is NULL, there is a code flow error */ + assert(ua_sess); - /* Create session on the tracer side and add it to app session HT */ - ua_sess = create_ust_app_session(usess, app); - if (ua_sess == NULL) { - continue; + /* Lookup channel in the ust app session */ + ua_chan_node = hashtable_lookup(ua_sess->channels, (void *)uchan->name, + strlen(uchan->name), &uiter); + /* If the channel is not found, there is a code flow error */ + assert(ua_chan_node); + + ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node); + + /* Enable each events of channel */ + cds_lfht_for_each_entry(ua_chan->events, &uiter, ua_event, node) { + ret = enable_ust_app_event(ua_sess, ua_chan, ua_event, app); + if (ret < 0) { + /* XXX: Report error someday... */ + continue; + } } + } + + rcu_read_unlock(); + + return ret; +} + +/* + * For a specific existing UST session and UST channel, creates the event for + * all registered apps. + */ +int ust_app_create_event_all(struct ltt_ust_session *usess, + struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent) +{ + int ret = 0; + struct cds_lfht_iter iter, uiter; + struct cds_lfht_node *ua_chan_node; + struct ust_app *app; + struct ust_app_session *ua_sess; + struct ust_app_channel *ua_chan; + + DBG("UST app creating event %s for all apps for session uid %d", + uevent->attr.name, usess->uid); + + /* + * NOTE: At this point, this function is called only if the session and + * channel passed are already created for all apps. and enabled on the + * tracer also. + */ + + rcu_read_lock(); + + /* For all registered applications */ + cds_lfht_for_each_entry(ust_app_ht, &iter, app, node) { + ua_sess = lookup_session_by_app(usess, app); + /* If ua_sess is NULL, there is a code flow error */ + assert(ua_sess); /* Lookup channel in the ust app session */ - ua_chan_node = hashtable_lookup(ua_sess->channels, - (void *)uchan->name, strlen(uchan->name), - &uiter); - if (ua_chan_node == NULL) { - ERR("Channel %s not found in session uid %d. Skipping", - uchan->name, usess->uid); - continue; - } + ua_chan_node = hashtable_lookup(ua_sess->channels, (void *)uchan->name, + strlen(uchan->name), &uiter); + /* If the channel is not found, there is a code flow error */ + assert(ua_chan_node); + ua_chan = caa_container_of(ua_chan_node, struct ust_app_channel, node); - ua_event = create_ust_app_event(ua_sess, ua_chan, uevent, app); - if (ua_event == NULL) { + ret = create_ust_app_event(ua_sess, ua_chan, uevent, app); + if (ret < 0) { continue; } } @@ -1494,7 +1598,6 @@ int ust_app_start_trace(struct ltt_ust_session *usess, struct ust_app *app) ua_sess = lookup_session_by_app(usess, app); if (ua_sess == NULL) { - /* Only malloc can failed so something is really wrong */ goto error_rcu_unlock; } diff --git a/lttng-sessiond/ust-app.h b/lttng-sessiond/ust-app.h index 2e8ce58da..87d84ffb9 100644 --- a/lttng-sessiond/ust-app.h +++ b/lttng-sessiond/ust-app.h @@ -126,6 +126,8 @@ int ust_app_disable_channel_all(struct ltt_ust_session *usess, struct ltt_ust_channel *uchan); int ust_app_enable_channel_all(struct ltt_ust_session *usess, struct ltt_ust_channel *uchan); +int ust_app_enable_event_all(struct ltt_ust_session *usess, + struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent); int ust_app_disable_event_all(struct ltt_ust_session *usess, struct ltt_ust_channel *uchan); int ust_app_disable_event(struct ltt_ust_session *usess, @@ -259,6 +261,12 @@ int ust_app_create_event_all(struct ltt_ust_session *usess, { return 0; } +static inline +int ust_app_enable_event_all(struct ltt_ust_session *usess, + struct ltt_ust_channel *uchan, struct ltt_ust_event *uevent) +{ + return 0; +} #endif /* HAVE_LIBLTTNG_UST_CTL */ -- 2.34.1