[systemd-devel] [PATCH 2/7] logind: close races on user and session states during login

Djalal Harouni tixxdz at opendz.org
Thu Feb 13 14:19:31 PST 2014


On Fri, Feb 07, 2014 at 04:48:39PM +0100, Lennart Poettering wrote:
> On Thu, 06.02.14 21:37, Djalal Harouni (tixxdz at opendz.org) wrote:
> 
> I think this one I fixed by adding a new "stopping" variable. Could you check?
Yes, the stopping variable caught most of the races, I've sent another
patch that will close which I think is the last one:
http://lists.freedesktop.org/archives/systemd-devel/2014-February/016889.html

Thanks Lennart!

> > Currently the user and session states are not stable, they are affected
> > by several races during login:
> > 
> > 1) session state:
> >    To get the session state the function session_get_state() is used.
> > 
> > Opening state:
> > At login the D-Bus CreateSession() method will call session_start() which
> > calls user_start() and session_start_scope() to queue the scope job and
> > set the session->scope_job
> > 
> >    =>  session_get_state() == SESSION_OPENING   (correct)
> > 
> > Then execution will continue from session_send_create() which is called
> > by the D-Bus match_job_removed() signal. math_job_removed() will check if
> > this is the session scope and if this is the previously queued scope job.
> > If so it will clear the session->scope_job
> > 
> >    =>  session_get_state() == SESSION_CLOSING   (incorrect)
> >                               (session closing since fifo_fd == -1)
> > 
> > So scope job has finished and scope was created successfully, later the
> > session_send_create_reply() will wait for the session scope *and* the
> > user service to be successfully created.
> > 
> >   /* user service is still pending */
> >   if (s->scope_job || s->user->service_job))
> >      return 0
> > 
> >    =>  session_get_state() == SESSION_CLOSING   (incorrect)
> > 
> >   else
> >      session_create_fifo()    /* fifo_fd finally created */
> > 
> >    =>  session_get_state() == SESSION_ACTIVE   (correct)
> > 
> > To sum it up, current state during login:
> > "SESSION_OPENING"=>"SESSION_CLOSING"x2=>"SESSION_ACTIVE"
> > 
> > To fix the session state and remove the two incorrect SESSION_CLOSING,
> > we do not clear up the "session->scope_job" when we detect that this is
> > the session scope, we setup a temporary variable "scope_job" that will
> > get the current value of "session->scope_job" and update it if
> > necessary.
> > 
> > Add a new "active" variable to check if the session scope and user
> > service jobs are still being created.
> > 
> > Update session_jobs_replay() and session_send_create_reply() function to
> > receive the "opening" variable as an argument, so it will still wait for
> > the scope and service jobs to finish before creating the session fifo.
> > 
> > The "session->scope_job" will be cleared when session_jobs_reply()
> > finishes. This ensures that the state will just go from:
> > "SESSION_OPENING" => "SESSION_ACTIVE"
> > 
> > 2) user state:
> >    To get the user state the function user_get_state() is used.
> > 
> > I'll add that the user_get_state() and session_get_state() do not have
> > the same logic when it comes to sessions, this will just add ambiguity.
> > user_get_state() calls session_is_active() before checking
> > session_get_state(), and session_is_active() will return true right from
> > the start since the logic is set during D-Bus CreateSession(). This will
> > we be fixed in the followup patches.
> > 
> > Opening state:
> > At login we have session_start() which calls user_start()
> > 
> > here we already:
> > 
> >    =>  user_get_state() == USER_ACTIVE   (incorrect)
> >                            (not fixed in this patch)
> > 
> > user_start() calls:
> > user_start_slice() queue the slice and set user->slice_job
> > user_start_service() queue the service and set user->service_job
> > 
> >    =>  user_get_state() == USER_OPENING   (correct)
> > 
> > Then execution will continue from session_send_create() which is called
> > by the D-Bus match_job_removed() signal. math_job_removed() will check if
> > these are the user service and slice and if they are the previously queued
> > jobs. If so it will clear the: user->service_job and user->slice_job
> > 
> >    =>  user_get_state() == USER_ACTIVE   (incorrect)
> >                  (incorrect since the fifo_fd has not been created,
> >                   here the state should stay USER_OPENING)
> > 
> > Later when the user service is created successfully,
> > session_send_create_reply() will also wait for the session scope to be
> > created. If so then session_send_create_reply() will continue and call
> > session_create_fifo()
> > 
> >    =>  user_get_state() == USER_ACTIVE   (correct)
> >                            (fifo_fd was created)
> > 
> > To fix this, we use the same logic as used to fix session states. In
> > order to remove the incorrect state when the fifo_fd is not created but
> > the state shows USER_ACTIVE, we do not clear the "user->service_job" and
> > "user->slice_job" right away, we store the state in a temporary variable
> > "service_job" and update it later if we detect that this is the user
> > service.
> > 
> > The new "active" variable will be used to check if the session scope and
> > user service are still being created. If so we'll wait for the next
> > match_job_removed() signal and continue, otherwise we proceed with
> > session_jobs_reply() and session_send_create_reply() in order to notify
> > clients.
> > ---
> >  src/login/logind-dbus.c         | 44 ++++++++++++++++++++++++++++++-----------
> >  src/login/logind-session-dbus.c |  8 +++++---
> >  src/login/logind-session.h      |  2 +-
> >  3 files changed, 39 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c
> > index 7b050fb..0560707 100644
> > --- a/src/login/logind-dbus.c
> > +++ b/src/login/logind-dbus.c
> > @@ -1919,7 +1919,9 @@ const sd_bus_vtable manager_vtable[] = {
> >          SD_BUS_VTABLE_END
> >  };
> >  
> > -static int session_jobs_reply(Session *s, const char *unit, const char *result) {
> > +/* session_jobs_reply() calls session_send_create_reply() to
> > + * notify client that they are able to login now. */
> > +static int session_jobs_reply(Session *s, const char *unit, const char *result, bool opening) {
> >          int r = 0;
> >  
> >          assert(s);
> > @@ -1929,12 +1931,12 @@ static int session_jobs_reply(Session *s, const char *unit, const char *result)
> >                  return r;
> >  
> >          if (streq(result, "done"))
> > -                r = session_send_create_reply(s, NULL);
> > +                r = session_send_create_reply(s, NULL, opening);
> >          else {
> >                  _cleanup_bus_error_free_ sd_bus_error e = SD_BUS_ERROR_NULL;
> >  
> >                  sd_bus_error_setf(&e, BUS_ERROR_JOB_FAILED, "Start job for unit %s failed with '%s'", unit, result);
> > -                r = session_send_create_reply(s, &e);
> > +                r = session_send_create_reply(s, &e, opening);
> >          }
> >  
> >          return r;
> > @@ -1973,22 +1975,46 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
> >  
> >          session = hashmap_get(m->session_units, unit);
> >          if (session) {
> > +                bool active, scope_job = !!session->scope_job;
> >  
> > -                if (streq_ptr(path, session->scope_job)) {
> > +                /* Set to false if the scope job has finished */
> > +                if (streq_ptr(path, session->scope_job))
> > +                        scope_job = false;
> > +
> > +                /* If the session scope and the user service are still
> > +                 * being created this will be set to true, otherwise
> > +                 * it will be false */
> > +                active = scope_job || !!session->user->service_job;
> > +                session_jobs_reply(session, unit, result, active);
> > +
> > +                if (!scope_job) {
> > +                        /* Clean this up after session_jobs_reply() */
> >                          free(session->scope_job);
> >                          session->scope_job = NULL;
> >                  }
> >  
> > -                session_jobs_reply(session, unit, result);
> > -
> >                  session_save(session);
> >                  session_add_to_gc_queue(session);
> >          }
> >  
> >          user = hashmap_get(m->user_units, unit);
> >          if (user) {
> > +                bool active, service_job = !!user->service_job;
> > +
> > +                /* Set to false if the user service has finished */
> > +                if (streq_ptr(path, user->service_job))
> > +                        service_job = false;
> > +
> > +                LIST_FOREACH(sessions_by_user, session, user->sessions) {
> > +                        /* If the session scope and the user service are
> > +                         * still being created this will be set to true,
> > +                         * otherwise it will be false */
> > +                        active = service_job || !!session->scope_job;
> > +                        session_jobs_reply(session, unit, result, active);
> > +                }
> >  
> > -                if (streq_ptr(path, user->service_job)) {
> > +                if (!service_job) {
> > +                        /* Clean this up after session_jobs_reply() */
> >                          free(user->service_job);
> >                          user->service_job = NULL;
> >                  }
> > @@ -1998,10 +2024,6 @@ int match_job_removed(sd_bus *bus, sd_bus_message *message, void *userdata, sd_b
> >                          user->slice_job = NULL;
> >                  }
> >  
> > -                LIST_FOREACH(sessions_by_user, session, user->sessions) {
> > -                        session_jobs_reply(session, unit, result);
> > -                }
> > -
> >                  user_save(user);
> >                  user_add_to_gc_queue(user);
> >          }
> > diff --git a/src/login/logind-session-dbus.c b/src/login/logind-session-dbus.c
> > index 7ee4956..54db864 100644
> > --- a/src/login/logind-session-dbus.c
> > +++ b/src/login/logind-session-dbus.c
> > @@ -641,7 +641,7 @@ int session_send_lock_all(Manager *m, bool lock) {
> >          return r;
> >  }
> >  
> > -int session_send_create_reply(Session *s, sd_bus_error *error) {
> > +int session_send_create_reply(Session *s, sd_bus_error *error, bool opening) {
> >          _cleanup_bus_message_unref_ sd_bus_message *c = NULL;
> >          _cleanup_close_ int fifo_fd = -1;
> >          _cleanup_free_ char *p = NULL;
> > @@ -650,12 +650,12 @@ int session_send_create_reply(Session *s, sd_bus_error *error) {
> >  
> >          /* This is called after the session scope and the user service
> >           * were successfully created, and finishes where
> > -         * bus_manager_create_session() left off. */
> > +         * method_create_session() left off. */
> >  
> >          if (!s->create_message)
> >                  return 0;
> >  
> > -        if (!sd_bus_error_is_set(error) && (s->scope_job || s->user->service_job))
> > +        if (!sd_bus_error_is_set(error) && opening)
> >                  return 0;
> >  
> >          c = s->create_message;
> > @@ -664,6 +664,8 @@ int session_send_create_reply(Session *s, sd_bus_error *error) {
> >          if (error)
> >                  return sd_bus_reply_method_error(c, error);
> >  
> > +        /* At this stage the session scope and the user service
> > +         * were successfully created */
> >          fifo_fd = session_create_fifo(s);
> >          if (fifo_fd < 0)
> >                  return fifo_fd;
> > diff --git a/src/login/logind-session.h b/src/login/logind-session.h
> > index 7bf1932..ebe3902 100644
> > --- a/src/login/logind-session.h
> > +++ b/src/login/logind-session.h
> > @@ -152,7 +152,7 @@ int session_send_changed(Session *s, const char *properties, ...) _sentinel_;
> >  int session_send_lock(Session *s, bool lock);
> >  int session_send_lock_all(Manager *m, bool lock);
> >  
> > -int session_send_create_reply(Session *s, sd_bus_error *error);
> > +int session_send_create_reply(Session *s, sd_bus_error *error, bool opening);
> >  
> >  const char* session_state_to_string(SessionState t) _const_;
> >  SessionState session_state_from_string(const char *s) _pure_;
> 
> 
> Lennart
> 
> -- 
> Lennart Poettering, Red Hat

-- 
Djalal Harouni
http://opendz.org


More information about the systemd-devel mailing list