From 83ca4ee8c2d27ade0a0d733b5bcfd8c6944adeb5 Mon Sep 17 00:00:00 2001 From: David Rheinsberg Date: Thu, 4 Feb 2021 16:27:03 +0100 Subject: [PATCH] broker: pass serial numbers with activations Extend the activation communication between broker and launcher to use serial numbers. This allows us to discard stale activation failures and closes a race condition where we would incorrectly fail new activations immediately after a previous service exited. Reported-by: Luca Bruno Signed-off-by: David Rheinsberg --- docs/dbus-broker.rst | 14 +++++++++++--- src/broker/controller-dbus.c | 28 ++++++++++++++++++++-------- src/broker/controller.c | 10 +++++----- src/broker/controller.h | 6 +++--- src/bus/activation.c | 11 +++++++---- src/bus/activation.h | 6 ++++-- src/bus/bus.h | 1 + src/bus/driver.c | 19 ++++++++----------- src/bus/driver.h | 2 +- src/launch/launcher.c | 7 ++++++- src/launch/service.c | 19 +++++-------------- src/launch/service.h | 3 ++- 12 files changed, 73 insertions(+), 53 deletions(-) diff --git a/docs/dbus-broker.rst b/docs/dbus-broker.rst index 48a1e5c9..bc372ca9 100644 --- a/docs/dbus-broker.rst +++ b/docs/dbus-broker.rst @@ -189,14 +189,22 @@ the broker. See the section below for a list of interfaces on the controller. | **method** Release() -> () | | # Reset the activation state of this name. Any pending activation -| # requests are canceled. -| **method** Reset() -> () +| # requests are canceled. The call requires a serial number to be +| # passed along. This must be the serial number received by the last +| # activation even on this name. Calls for other serial numbers are +| # silently ignored and considered stale. +| **method** Reset(**t** *serial*) -> () | | # This signal is sent whenever a client requests activation of this | # name. Note that multiple activation requests are coalesced by the | # broker. The controller can cancel outstanding requests via the | # **Reset()** method. -| **signal** Activate() +| # The broker sends a serial number with the event. This number +| # represents the activation request and must be used when reacting +| # to the request with methods like *Reset()*. The serial number is +| # unique for each event, and is never reused. A serial number of 0 +| # is never sent and considered invalid. +| **signal** Activate(**t** *serial*) | | } | } diff --git a/src/broker/controller-dbus.c b/src/broker/controller-dbus.c index 6de854a9..449151dc 100644 --- a/src/broker/controller-dbus.c +++ b/src/broker/controller-dbus.c @@ -54,6 +54,13 @@ struct ControllerMethod { _body \ ) +static const CDVarType controller_type_in_t[] = { + C_DVAR_T_INIT( + C_DVAR_T_TUPLE1( + C_DVAR_T_t + ) + ) +}; static const CDVarType controller_type_in_v[] = { C_DVAR_T_INIT( C_DVAR_T_TUPLE1( @@ -304,7 +311,7 @@ static int controller_method_name_release(Controller *controller, const char *pa if (!name) return CONTROLLER_E_NAME_NOT_FOUND; - r = controller_name_reset(name); + r = controller_name_reset(name, name->activation.pending); if (r) return error_trace(r); @@ -352,9 +359,10 @@ static int controller_method_listener_set_policy(Controller *controller, const c static int controller_method_name_reset(Controller *controller, const char *path, CDVar *in_v, FDList *fds, CDVar *out_v) { ControllerName *name; + uint64_t serial; int r; - c_dvar_read(in_v, "()"); + c_dvar_read(in_v, "(t)", &serial); r = controller_end_read(in_v); if (r) @@ -364,7 +372,7 @@ static int controller_method_name_reset(Controller *controller, const char *path if (!name) return CONTROLLER_E_NAME_NOT_FOUND; - r = controller_name_reset(name); + r = controller_name_reset(name, serial); if (r) return error_trace(r); @@ -457,7 +465,7 @@ static int controller_dispatch_controller(Controller *controller, uint32_t seria static int controller_dispatch_name(Controller *controller, uint32_t serial, const char *method, const char *path, const char *signature, Message *message) { static const ControllerMethod methods[] = { - { "Reset", controller_method_name_reset, c_dvar_type_unit, controller_type_out_unit }, + { "Reset", controller_method_name_reset, controller_type_in_t, controller_type_out_unit }, { "Release", controller_method_name_release, c_dvar_type_unit, controller_type_out_unit }, }; @@ -636,11 +644,13 @@ int controller_dbus_dispatch(Controller *controller, Message *message) { /** * controller_dbus_send_activation() - XXX */ -int controller_dbus_send_activation(Controller *controller, const char *path) { +int controller_dbus_send_activation(Controller *controller, const char *path, uint64_t serial) { static const CDVarType type[] = { C_DVAR_T_INIT( CONTROLLER_T_MESSAGE( - C_DVAR_T_TUPLE0 + C_DVAR_T_TUPLE1( + C_DVAR_T_t + ) ) ) }; @@ -651,11 +661,13 @@ int controller_dbus_send_activation(Controller *controller, const char *path) { int r; c_dvar_begin_write(&var, (__BYTE_ORDER == __BIG_ENDIAN), type, 1); - c_dvar_write(&var, "((yyyyuu[(y)(y)(y)])())", + c_dvar_write(&var, "((yyyyuu[(y)(y)(y)(y)])(t))", c_dvar_is_big_endian(&var) ? 'B' : 'l', DBUS_MESSAGE_TYPE_SIGNAL, DBUS_HEADER_FLAG_NO_REPLY_EXPECTED, 1, 0, (uint32_t)-1, DBUS_MESSAGE_FIELD_PATH, c_dvar_type_o, path, DBUS_MESSAGE_FIELD_INTERFACE, c_dvar_type_s, "org.bus1.DBus.Name", - DBUS_MESSAGE_FIELD_MEMBER, c_dvar_type_s, "Activate"); + DBUS_MESSAGE_FIELD_MEMBER, c_dvar_type_s, "Activate", + DBUS_MESSAGE_FIELD_SIGNATURE, c_dvar_type_g, "t", + serial); r = c_dvar_end_write(&var, &data, &n_data); if (r) diff --git a/src/broker/controller.c b/src/broker/controller.c index 092b493e..450c4ad2 100644 --- a/src/broker/controller.c +++ b/src/broker/controller.c @@ -66,10 +66,10 @@ static int controller_name_new(ControllerName **namep, Controller *controller, c /** * controller_name_reset() - XXX */ -int controller_name_reset(ControllerName *name) { +int controller_name_reset(ControllerName *name, uint64_t serial) { int r; - r = driver_name_activation_failed(&name->controller->broker->bus, &name->activation); + r = driver_name_activation_failed(&name->controller->broker->bus, &name->activation, serial); if (r) return error_fold(r); @@ -79,8 +79,8 @@ int controller_name_reset(ControllerName *name) { /** * controller_name_activate() - XXX */ -int controller_name_activate(ControllerName *name) { - return controller_dbus_send_activation(name->controller, name->path); +int controller_name_activate(ControllerName *name, uint64_t serial) { + return controller_dbus_send_activation(name->controller, name->path, serial); } static int controller_reload_compare(CRBTree *t, void *k, CRBNode *rb) { @@ -311,7 +311,7 @@ int controller_add_name(Controller *controller, if (r) return error_trace(r); - r = activation_init(&name->activation, name_entry, user_entry); + r = activation_init(&name->activation, &controller->broker->bus, name_entry, user_entry); if (r) return (r == ACTIVATION_E_ALREADY_ACTIVATABLE) ? CONTROLLER_E_NAME_IS_ACTIVATABLE : error_fold(r); diff --git a/src/broker/controller.h b/src/broker/controller.h index 385bc559..a3530034 100644 --- a/src/broker/controller.h +++ b/src/broker/controller.h @@ -101,8 +101,8 @@ struct Controller { /* names */ ControllerName *controller_name_free(ControllerName *name); -int controller_name_reset(ControllerName *name); -int controller_name_activate(ControllerName *name); +int controller_name_reset(ControllerName *name, uint64_t serial); +int controller_name_activate(ControllerName *name, uint64_t serial); C_DEFINE_CLEANUP(ControllerName *, controller_name_free); @@ -144,7 +144,7 @@ ControllerListener *controller_find_listener(Controller *controller, const char ControllerReload *controller_find_reload(Controller *controller, uint32_t serial); int controller_dbus_dispatch(Controller *controller, Message *message); -int controller_dbus_send_activation(Controller *controller, const char *path); +int controller_dbus_send_activation(Controller *controller, const char *path, uint64_t serial); int controller_dbus_send_reload(Controller *controller, User *user, uint32_t serial); int controller_dbus_send_environment(Controller *controller, const char * const *env, size_t n_env); diff --git a/src/bus/activation.c b/src/bus/activation.c index bc1e6686..094486a4 100644 --- a/src/bus/activation.c +++ b/src/bus/activation.c @@ -7,6 +7,7 @@ #include #include "broker/controller.h" #include "bus/activation.h" +#include "bus/bus.h" #include "bus/name.h" #include "bus/policy.h" #include "dbus/message.h" @@ -48,13 +49,14 @@ C_DEFINE_CLEANUP(ActivationMessage *, activation_message_free); /** * activation_init() - XXX */ -int activation_init(Activation *a, Name *name, User *user) { +int activation_init(Activation *a, Bus *bus, Name *name, User *user) { _c_cleanup_(activation_deinitp) Activation *activation = a; if (name->activation) return ACTIVATION_E_ALREADY_ACTIVATABLE; *activation = (Activation)ACTIVATION_NULL(*activation); + activation->bus = bus; activation->name = name_ref(name); activation->user = user_ref(user); @@ -113,14 +115,15 @@ void activation_get_stats_for(Activation *activation, static int activation_request(Activation *activation) { int r; - if (activation->requested) + if (activation->pending) return 0; - r = controller_name_activate(CONTROLLER_NAME(activation)); + r = controller_name_activate(CONTROLLER_NAME(activation), + ++activation->bus->activation_ids); if (r) return error_fold(r); - activation->requested = true; + activation->pending = activation->bus->activation_ids; return 0; } diff --git a/src/bus/activation.h b/src/bus/activation.h index b7dada10..7cb4f79f 100644 --- a/src/bus/activation.h +++ b/src/bus/activation.h @@ -13,6 +13,7 @@ typedef struct Activation Activation; typedef struct ActivationMessage ActivationMessage; typedef struct ActivationRequest ActivationRequest; +typedef struct Bus Bus; typedef struct Message Message; typedef struct Name Name; typedef struct NameOwner NameOwner; @@ -43,11 +44,12 @@ struct ActivationMessage { }; struct Activation { + Bus *bus; Name *name; User *user; CList activation_messages; CList activation_requests; - bool requested : 1; + uint64_t pending; }; #define ACTIVATION_NULL(_x) { \ @@ -65,7 +67,7 @@ ActivationMessage *activation_message_free(ActivationMessage *message); /* activation */ -int activation_init(Activation *activation, Name *name, User *user); +int activation_init(Activation *activation, Bus *bus, Name *name, User *user); void activation_deinit(Activation *activation); void activation_get_stats_for(Activation *activation, diff --git a/src/bus/bus.h b/src/bus/bus.h index 2d55f76e..07b7369b 100644 --- a/src/bus/bus.h +++ b/src/bus/bus.h @@ -50,6 +50,7 @@ struct Bus { uint64_t n_monitors; uint64_t listener_ids; + uint64_t activation_ids; Metrics metrics; }; diff --git a/src/bus/driver.c b/src/bus/driver.c index ce6900db..b9774d60 100644 --- a/src/bus/driver.c +++ b/src/bus/driver.c @@ -721,22 +721,19 @@ static int driver_name_owner_changed(Bus *bus, MatchRegistry *matches, const cha return 0; } -int driver_name_activation_failed(Bus *bus, Activation *activation) { +int driver_name_activation_failed(Bus *bus, Activation *activation, uint64_t serial) { ActivationRequest *request, *request_safe; ActivationMessage *message, *message_safe; int r; - /* in case the name is activated again in the future, we should request it again */ - activation->requested = false; - /* - * NOTE: We can get activation-failure notifications by the launcher - * even when the name was already claimed. The launcher tracks - * services for their entire lifetime. - * We don't really care for late notifications so far, since both - * the queues are empty in this case. But we need to be aware of - * this if we extend this in the future. + * If there is no pending activation, or if it is an event for an old + * activation, we ignore it silently. */ + if (!activation->pending || serial != activation->pending) + return 0; + + activation->pending = 0; c_list_for_each_entry_safe(request, request_safe, &activation->activation_requests, link) { Peer *sender; @@ -776,7 +773,7 @@ static int driver_name_activated(Activation *activation, Peer *receiver) { return 0; /* in case the name is dropped again in the future, we should request it again */ - activation->requested = false; + activation->pending = 0; c_list_for_each_entry_safe(request, request_safe, &activation->activation_requests, link) { Peer *sender; diff --git a/src/bus/driver.h b/src/bus/driver.h index 6618f599..4de40315 100644 --- a/src/bus/driver.h +++ b/src/bus/driver.h @@ -63,7 +63,7 @@ enum { _DRIVER_E_MAX, }; -int driver_name_activation_failed(Bus *bus, Activation *activation); +int driver_name_activation_failed(Bus *bus, Activation *activation, uint64_t serial); int driver_reload_config_completed(Bus *bus, uint64_t sender_id, uint32_t reply_serial); int driver_reload_config_invalid(Bus *bus, uint64_t sender_id, uint32_t reply_serial); diff --git a/src/launch/launcher.c b/src/launch/launcher.c index df7294d6..3fd69e07 100644 --- a/src/launch/launcher.c +++ b/src/launch/launcher.c @@ -386,8 +386,13 @@ static int launcher_fork(Launcher *launcher, int fd_controller) { static int launcher_on_name_activate(Launcher *launcher, sd_bus_message *m, const char *id) { Service *service; + uint64_t serial; int r; + r = sd_bus_message_read(m, "t", &serial); + if (r < 0) + return error_origin(r); + service = c_rbtree_find_entry(&launcher->services, service_compare, id, @@ -403,7 +408,7 @@ static int launcher_on_name_activate(Launcher *launcher, sd_bus_message *m, cons return 0; } - r = service_activate(service); + r = service_activate(service, serial); if (r) return error_fold(r); diff --git a/src/launch/service.c b/src/launch/service.c index 621364dd..3f9d971d 100644 --- a/src/launch/service.c +++ b/src/launch/service.c @@ -202,18 +202,6 @@ static int service_reset_activation(Service *service) { * dbus error to each pending transaction. * So far, we did not define such errors, so this is left for the * future. - * - * XXX: Additionally, we would probably want a serial-number for each - * activation request, which we then include in this reset. This - * would properly order consequetive activation requests and allow - * the broker to discard resets that come out-of-order. - * However, this is quite unlikely to solve any real issue, since - * there is no defined behavior of failed activation requests, - * anyway. While they are recoverable in most situations, there is - * no defined barrier at which point a followup should succeed. - * Hence, this is left for the future, in case this situation gets - * a well-defined behavior (though, needs coordination with the - * reference implementation and systemd). */ r = sd_bus_call_method(service->launcher->bus_controller, NULL, @@ -222,7 +210,8 @@ static int service_reset_activation(Service *service) { "Reset", NULL, NULL, - ""); + "t", + service->last_serial); if (r < 0) return error_origin(r); @@ -794,6 +783,7 @@ static int service_start_transient_unit(Service *service) { /** * service_activate() - trigger a service activation * @service: service to activate + * @serial: activation serial number * * This activates the specified service. Any previous activation is discarded * silently. The new activation replaces a possible old one. @@ -825,10 +815,11 @@ static int service_start_transient_unit(Service *service) { * * Returns: 0 on success, negative error code on failure. */ -int service_activate(Service *service) { +int service_activate(Service *service, uint64_t serial) { int r; service_discard_activation(service); + service->last_serial = serial; if (!strcmp(service->name, "org.freedesktop.systemd1")) { /* diff --git a/src/launch/service.h b/src/launch/service.h index bb616b82..ede29abb 100644 --- a/src/launch/service.h +++ b/src/launch/service.h @@ -32,6 +32,7 @@ struct Service { uint64_t instance; uint64_t n_missing_unit; uint64_t n_masked_unit; + uint64_t last_serial; char id[]; }; @@ -56,5 +57,5 @@ int service_compare(CRBTree *t, void *k, CRBNode *n); int service_compare_by_name(CRBTree *t, void *k, CRBNode *n); int service_add(Service *service); -int service_activate(Service *service); +int service_activate(Service *service, uint64_t serial); int service_remove(Service *service);