Fix: sessiond: client/client_list lock inversion on disconnect
[lttng-tools.git] / src / bin / lttng-sessiond / action-executor.c
index 40ca2bd12a6aa41dd72caf6ba4f2f7aca8c0dd68..e3b387d62d35c33e76c95f2b3745bdfde1e74156 100644 (file)
@@ -54,6 +54,10 @@ struct action_executor {
        uint64_t next_work_item_id;
 };
 
+/*
+ * Only return non-zero on a fatal error that should shut down the action
+ * executor.
+ */
 typedef int (*action_executor_handler)(struct action_executor *executor,
                const struct action_work_item *,
                const struct lttng_action *action);
@@ -103,6 +107,33 @@ static const char *get_action_name(const struct lttng_action *action)
        return action_type_names[lttng_action_get_type_const(action)];
 }
 
+/* Check if this trigger allowed to interect with a given session. */
+static bool is_trigger_allowed_for_session(const struct lttng_trigger *trigger,
+               struct ltt_session *session)
+{
+       bool is_allowed = false;
+       const struct lttng_credentials session_creds = {
+               .uid = session->uid,
+               .gid = session->gid,
+       };
+       /* Can never be NULL. */
+       const struct lttng_credentials *trigger_creds =
+                       lttng_trigger_get_credentials(trigger);
+
+       is_allowed = (trigger_creds->uid == session_creds.uid) ||
+                       (trigger_creds->uid == 0);
+       if (!is_allowed) {
+               WARN("Trigger is not allowed to interact with session `%s`: session uid = %ld, session gid = %ld, trigger uid = %ld, trigger gid = %ld",
+                               session->name,
+                               (long int) session->uid,
+                               (long int) session->gid,
+                               (long int) trigger_creds->uid,
+                               (long int) trigger_creds->gid);
+       }
+
+       return is_allowed;
+}
+
 static int client_handle_transmission_status(
                struct notification_client *client,
                enum client_transmission_status status,
@@ -112,8 +143,6 @@ static int client_handle_transmission_status(
        struct action_executor *executor = user_data;
        bool update_communication = true;
 
-       ASSERT_LOCKED(client->lock);
-
        switch (status) {
        case CLIENT_TRANSMISSION_STATUS_COMPLETE:
                DBG("Successfully sent full notification to client, client_id = %" PRIu64,
@@ -127,12 +156,10 @@ static int client_handle_transmission_status(
        case CLIENT_TRANSMISSION_STATUS_FAIL:
                DBG("Communication error occurred while sending notification to client, client_id = %" PRIu64,
                                client->id);
-               client->communication.active = false;
                break;
        default:
                ERR("Fatal error encoutered while sending notification to client, client_id = %" PRIu64,
                                client->id);
-               client->communication.active = false;
                ret = -1;
                goto end;
        }
@@ -141,6 +168,7 @@ static int client_handle_transmission_status(
                goto end;
        }
 
+       /* Safe to read client's id without locking as it is immutable. */
        ret = notification_thread_client_communication_update(
                        executor->notification_thread_handle, client->id,
                        status);
@@ -169,11 +197,12 @@ static int action_executor_start_session_handler(struct action_executor *executo
        const char *session_name;
        enum lttng_action_status action_status;
        struct ltt_session *session;
+       enum lttng_error_code cmd_ret;
 
        action_status = lttng_action_start_session_get_session_name(
                        action, &session_name);
        if (action_status != LTTNG_ACTION_STATUS_OK) {
-               ERR("Failed to get session name from \"%s\" action",
+               ERR("Failed to get session name from `%s` action",
                                get_action_name(action));
                ret = -1;
                goto end;
@@ -181,37 +210,39 @@ static int action_executor_start_session_handler(struct action_executor *executo
 
        session_lock_list();
        session = session_find_by_name(session_name);
-       if (session) {
-               enum lttng_error_code cmd_ret;
+       if (!session) {
+               DBG("Failed to find session `%s` by name while executing `%s` action of trigger `%p`",
+                               session_name, get_action_name(action),
+                   work_item->trigger);
+               goto error_unlock_list;
+       }
 
-               session_lock(session);
-               cmd_ret = cmd_start_trace(session);
-               session_unlock(session);
+       session_lock(session);
+       if (!is_trigger_allowed_for_session(work_item->trigger, session)) {
+               goto error_dispose_session;
+       }
 
-               switch (cmd_ret) {
-               case LTTNG_OK:
-                       DBG("Successfully started session \"%s\" on behalf of trigger \"%p\"",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               case LTTNG_ERR_TRACE_ALREADY_STARTED:
-                       DBG("Attempted to start session \"%s\" on behalf of trigger \"%p\" but it was already started",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               default:
-                       WARN("Failed to start session \"%s\" on behalf of trigger \"%p\": %s",
-                                       session_name,
-                                       work_item->trigger,
-                                       lttng_strerror(-cmd_ret));
-                       break;
-               }
-               session_put(session);
-       } else {
-               DBG("Failed to find session \"%s\" by name while executing \"%s\" action of trigger \"%p\"",
-                               session_name, get_action_name(action),
-                               work_item->trigger);
+       cmd_ret = cmd_start_trace(session);
+       switch (cmd_ret) {
+       case LTTNG_OK:
+               DBG("Successfully started session `%s` on behalf of trigger `%p`",
+                               session_name, work_item->trigger);
+               break;
+       case LTTNG_ERR_TRACE_ALREADY_STARTED:
+               DBG("Attempted to start session `%s` on behalf of trigger `%p` but it was already started",
+                               session_name, work_item->trigger);
+               break;
+       default:
+               WARN("Failed to start session `%s` on behalf of trigger `%p`: %s",
+                               session_name, work_item->trigger,
+                               lttng_strerror(-cmd_ret));
+               break;
        }
+
+error_dispose_session:
+       session_unlock(session);
+       session_put(session);
+error_unlock_list:
        session_unlock_list();
 end:
        return ret;
@@ -225,11 +256,12 @@ static int action_executor_stop_session_handler(struct action_executor *executor
        const char *session_name;
        enum lttng_action_status action_status;
        struct ltt_session *session;
+       enum lttng_error_code cmd_ret;
 
        action_status = lttng_action_stop_session_get_session_name(
                        action, &session_name);
        if (action_status != LTTNG_ACTION_STATUS_OK) {
-               ERR("Failed to get session name from \"%s\" action",
+               ERR("Failed to get session name from `%s` action",
                                get_action_name(action));
                ret = -1;
                goto end;
@@ -237,37 +269,39 @@ static int action_executor_stop_session_handler(struct action_executor *executor
 
        session_lock_list();
        session = session_find_by_name(session_name);
-       if (session) {
-               enum lttng_error_code cmd_ret;
+       if (!session) {
+               DBG("Failed to find session `%s` by name while executing `%s` action of trigger `%p`",
+                               session_name, get_action_name(action),
+                   work_item->trigger);
+               goto error_unlock_list;
+       }
 
-               session_lock(session);
-               cmd_ret = cmd_stop_trace(session);
-               session_unlock(session);
+       session_lock(session);
+       if (!is_trigger_allowed_for_session(work_item->trigger, session)) {
+               goto error_dispose_session;
+       }
 
-               switch (cmd_ret) {
-               case LTTNG_OK:
-                       DBG("Successfully stopped session \"%s\" on behalf of trigger \"%p\"",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               case LTTNG_ERR_TRACE_ALREADY_STOPPED:
-                       DBG("Attempted to stop session \"%s\" on behalf of trigger \"%p\" but it was already stopped",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               default:
-                       WARN("Failed to stop session \"%s\" on behalf of trigger \"%p\": %s",
-                                       session_name,
-                                       work_item->trigger,
-                                       lttng_strerror(-cmd_ret));
-                       break;
-               }
-               session_put(session);
-       } else {
-               DBG("Failed to find session \"%s\" by name while executing \"%s\" action of trigger \"%p\"",
-                               session_name, get_action_name(action),
-                               work_item->trigger);
+       cmd_ret = cmd_stop_trace(session);
+       switch (cmd_ret) {
+       case LTTNG_OK:
+               DBG("Successfully stopped session `%s` on behalf of trigger `%p`",
+                               session_name, work_item->trigger);
+               break;
+       case LTTNG_ERR_TRACE_ALREADY_STOPPED:
+               DBG("Attempted to stop session `%s` on behalf of trigger `%p` but it was already stopped",
+                               session_name, work_item->trigger);
+               break;
+       default:
+               WARN("Failed to stop session `%s` on behalf of trigger `%p`: %s",
+                               session_name, work_item->trigger,
+                               lttng_strerror(-cmd_ret));
+               break;
        }
+
+error_dispose_session:
+       session_unlock(session);
+       session_put(session);
+error_unlock_list:
        session_unlock_list();
 end:
        return ret;
@@ -281,11 +315,12 @@ static int action_executor_rotate_session_handler(struct action_executor *execut
        const char *session_name;
        enum lttng_action_status action_status;
        struct ltt_session *session;
+       enum lttng_error_code cmd_ret;
 
        action_status = lttng_action_rotate_session_get_session_name(
                        action, &session_name);
        if (action_status != LTTNG_ACTION_STATUS_OK) {
-               ERR("Failed to get session name from \"%s\" action",
+               ERR("Failed to get session name from `%s` action",
                                get_action_name(action));
                ret = -1;
                goto end;
@@ -293,44 +328,45 @@ static int action_executor_rotate_session_handler(struct action_executor *execut
 
        session_lock_list();
        session = session_find_by_name(session_name);
-       if (session) {
-               enum lttng_error_code cmd_ret;
-
-               session_lock(session);
-               cmd_ret = cmd_rotate_session(session, NULL, false,
-                               LTTNG_TRACE_CHUNK_COMMAND_TYPE_MOVE_TO_COMPLETED);
-               session_unlock(session);
-
-               switch (cmd_ret) {
-               case LTTNG_OK:
-                       DBG("Successfully started rotation of session \"%s\" on behalf of trigger \"%p\"",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               case LTTNG_ERR_ROTATION_PENDING:
-                       DBG("Attempted to start a rotation of session \"%s\" on behalf of trigger \"%p\" but a rotation is already ongoing",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               case LTTNG_ERR_ROTATION_MULTIPLE_AFTER_STOP:
-               case LTTNG_ERR_ROTATION_AFTER_STOP_CLEAR:
-                       DBG("Attempted to start a rotation of session \"%s\" on behalf of trigger \"%p\" but a rotation has already been completed since the last stop or clear",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               default:
-                       WARN("Failed to start a rotation of session \"%s\" on behalf of trigger \"%p\": %s",
-                                       session_name,
-                                       work_item->trigger,
-                                       lttng_strerror(-cmd_ret));
-                       break;
-               }
-               session_put(session);
-       } else {
-               DBG("Failed to find session \"%s\" by name while executing \"%s\" action of trigger \"%p\"",
+       if (!session) {
+               DBG("Failed to find session `%s` by name while executing `%s` action of trigger `%p`",
                                session_name, get_action_name(action),
-                               work_item->trigger);
+                   work_item->trigger);
+               goto error_unlock_list;
+       }
+
+       session_lock(session);
+       if (!is_trigger_allowed_for_session(work_item->trigger, session)) {
+               goto error_dispose_session;
        }
+
+       cmd_ret = cmd_rotate_session(session, NULL, false,
+                       LTTNG_TRACE_CHUNK_COMMAND_TYPE_MOVE_TO_COMPLETED);
+       switch (cmd_ret) {
+       case LTTNG_OK:
+               DBG("Successfully started rotation of session `%s` on behalf of trigger `%p`",
+                               session_name, work_item->trigger);
+               break;
+       case LTTNG_ERR_ROTATION_PENDING:
+               DBG("Attempted to start a rotation of session `%s` on behalf of trigger `%p` but a rotation is already ongoing",
+                               session_name, work_item->trigger);
+               break;
+       case LTTNG_ERR_ROTATION_MULTIPLE_AFTER_STOP:
+       case LTTNG_ERR_ROTATION_AFTER_STOP_CLEAR:
+               DBG("Attempted to start a rotation of session `%s` on behalf of trigger `%p` but a rotation has already been completed since the last stop or clear",
+                               session_name, work_item->trigger);
+               break;
+       default:
+               WARN("Failed to start a rotation of session `%s` on behalf of trigger `%p`: %s",
+                               session_name, work_item->trigger,
+                               lttng_strerror(-cmd_ret));
+               break;
+       }
+
+error_dispose_session:
+       session_unlock(session);
+       session_put(session);
+error_unlock_list:
        session_unlock_list();
 end:
        return ret;
@@ -349,11 +385,12 @@ static int action_executor_snapshot_session_handler(struct action_executor *exec
        };
        const struct lttng_snapshot_output *snapshot_output =
                        &default_snapshot_output;
+       enum lttng_error_code cmd_ret;
 
        action_status = lttng_action_snapshot_session_get_session_name(
                        action, &session_name);
        if (action_status != LTTNG_ACTION_STATUS_OK) {
-               ERR("Failed to get session name from \"%s\" action",
+               ERR("Failed to get session name from `%s` action",
                                get_action_name(action));
                ret = -1;
                goto end;
@@ -363,7 +400,7 @@ static int action_executor_snapshot_session_handler(struct action_executor *exec
                        action, &snapshot_output);
        if (action_status != LTTNG_ACTION_STATUS_OK &&
                        action_status != LTTNG_ACTION_STATUS_UNSET) {
-               ERR("Failed to get output from \"%s\" action",
+               ERR("Failed to get output from `%s` action",
                                get_action_name(action));
                ret = -1;
                goto end;
@@ -371,32 +408,36 @@ static int action_executor_snapshot_session_handler(struct action_executor *exec
 
        session_lock_list();
        session = session_find_by_name(session_name);
-       if (session) {
-               enum lttng_error_code cmd_ret;
+       if (!session) {
+               DBG("Failed to find session `%s` by name while executing `%s` action of trigger `%p`",
+                               session_name, get_action_name(action),
+                   work_item->trigger);
+               goto error_unlock_list;
+       }
 
-               session_lock(session);
-               cmd_ret = cmd_snapshot_record(session, snapshot_output, 0);
-               session_unlock(session);
 
-               switch (cmd_ret) {
-               case LTTNG_OK:
-                       DBG("Successfully recorded snapshot of session \"%s\" on behalf of trigger \"%p\"",
-                                       session_name,
-                                       work_item->trigger);
-                       break;
-               default:
-                       WARN("Failed to record snapshot of session \"%s\" on behalf of trigger \"%p\": %s",
-                                       session_name,
-                                       work_item->trigger,
-                                       lttng_strerror(-cmd_ret));
-                       break;
-               }
-               session_put(session);
-       } else {
-               DBG("Failed to find session \"%s\" by name while executing \"%s\" action of trigger \"%p\"",
-                               session_name, get_action_name(action),
-                               work_item->trigger);
+       session_lock(session);
+       if (!is_trigger_allowed_for_session(work_item->trigger, session)) {
+               goto error_dispose_session;
+       }
+
+       cmd_ret = cmd_snapshot_record(session, snapshot_output, 0);
+       switch (cmd_ret) {
+       case LTTNG_OK:
+               DBG("Successfully recorded snapshot of session `%s` on behalf of trigger `%p`",
+                               session_name, work_item->trigger);
+               break;
+       default:
+               WARN("Failed to record snapshot of session `%s` on behalf of trigger `%p`: %s",
+                               session_name, work_item->trigger,
+                               lttng_strerror(-cmd_ret));
+               break;
        }
+
+error_dispose_session:
+       session_unlock(session);
+       session_put(session);
+error_unlock_list:
        session_unlock_list();
 end:
        return ret;
@@ -427,7 +468,7 @@ static int action_executor_group_handler(struct action_executor *executor,
                ret = action_executor_generic_handler(
                                executor, work_item, action);
                if (ret) {
-                       ERR("Stopping the execution of the action group of trigger \"%p\" following a fatal error",
+                       ERR("Stopping the execution of the action group of trigger `%p` following a fatal error",
                                        work_item->trigger);
                        goto end;
                }
@@ -440,7 +481,7 @@ static int action_executor_generic_handler(struct action_executor *executor,
                const struct action_work_item *work_item,
                const struct lttng_action *action)
 {
-       DBG("Executing action \"%s\" of trigger \"%p\" action work item %" PRIu64,
+       DBG("Executing action `%s` of trigger `%p` action work item %" PRIu64,
                        get_action_name(action),
                        work_item->trigger,
                        work_item->id);
@@ -456,10 +497,10 @@ static int action_work_item_execute(struct action_executor *executor,
        const struct lttng_action *action =
                        lttng_trigger_get_const_action(work_item->trigger);
 
-       DBG("Starting execution of action work item %" PRIu64 " of trigger \"%p\"",
+       DBG("Starting execution of action work item %" PRIu64 " of trigger `%p`",
                        work_item->id, work_item->trigger);
        ret = action_executor_generic_handler(executor, work_item, action);
-       DBG("Completed execution of action work item %" PRIu64 " of trigger \"%p\"",
+       DBG("Completed execution of action work item %" PRIu64 " of trigger `%p`",
                        work_item->id, work_item->trigger);
        return ret;
 }
@@ -593,7 +634,7 @@ void action_executor_destroy(struct action_executor *executor)
        cds_list_for_each_entry_safe (
                        work_item, tmp, &executor->work.list, list_node) {
                WARN("Discarding action work item %" PRIu64
-                               " associated to trigger \"%p\"",
+                               " associated to trigger `%p`",
                                work_item->id, work_item->trigger);
                cds_list_del(&work_item->list_node);
                action_work_item_destroy(work_item);
@@ -619,7 +660,7 @@ enum action_executor_status action_executor_enqueue(
        /* Check for queue overflow. */
        if (executor->work.pending_count >= MAX_QUEUED_WORK_COUNT) {
                /* Most likely spammy, remove if it is the case. */
-               DBG("Refusing to enqueue action for trigger \"%p\" as work item %" PRIu64
+               DBG("Refusing to enqueue action for trigger `%p` as work item %" PRIu64
                    " (overflow)",
                                trigger, work_item_id);
                executor_status = ACTION_EXECUTOR_STATUS_OVERFLOW;
@@ -628,7 +669,7 @@ enum action_executor_status action_executor_enqueue(
 
        work_item = zmalloc(sizeof(*work_item));
        if (!work_item) {
-               PERROR("Failed to allocate action executor work item on behalf of trigger \"%p\"",
+               PERROR("Failed to allocate action executor work item on behalf of trigger `%p`",
                                trigger);
                executor_status = ACTION_EXECUTOR_STATUS_ERROR;
                goto error_unlock;
@@ -659,7 +700,7 @@ enum action_executor_status action_executor_enqueue(
        evaluation = NULL;
        cds_list_add_tail(&work_item->list_node, &executor->work.list);
        executor->work.pending_count++;
-       DBG("Enqueued action for trigger \"%p\" as work item %" PRIu64,
+       DBG("Enqueued action for trigger `%p` as work item %" PRIu64,
                        trigger, work_item_id);
        signal = true;
 
This page took 0.029696 seconds and 4 git commands to generate.