Attachment 104446 Details for Bug 81469 – [PATCH 3/4] enforce new limit max_connections_per_cgroup

[patch] [PATCH 3/4] enforce new limit max_connections_per_cgroup

0003-enforce-new-limit-max_connections_per_cgroup.patch (text/plain), 11.95 KB, created by Alban Crequy on 2014-08-11 14:53:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Alban Crequy

Created: 2014-08-11 14:53:00 UTC

Size: 11.95 KB

patch

obsolete

>From 8d8ad4e4239ffd506c8c23aa1a50115905fe599a Mon Sep 17 00:00:00 2001
>From: Alban Crequy <alban.crequy@collabora.co.uk>
>Date: Wed, 16 Jul 2014 16:36:52 +0100
>Subject: [PATCH 3/4] enforce new limit max_connections_per_cgroup
>
>When it is not possible to get the Unix pid of a connection, that connection
>bypass the limit. It can happen on Windows or on the non-supported case where
>dbus-daemon is configured to listen on TCP rather than a Unix socket.
>
>v2:
> - fix memory leaks in adjust_connections_for_cgroup in case of oom
> - fix memory release in bus_connections_setup_connection in case of oom
>
>https://bugs.freedesktop.org/show_bug.cgi?id=81469
>---
> bus/connection.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 178 insertions(+), 9 deletions(-)
>
>diff --git a/bus/connection.c b/bus/connection.c
>index 4c19c8c..dca2137 100644
>--- a/bus/connection.c
>+++ b/bus/connection.c
>@@ -60,6 +60,7 @@ struct BusConnections
>   BusContext *context;
>   DBusHashTable *completed_by_user; /**< Number of completed connections for each UID */
>   DBusHashTable *completed_by_process; /**< Number of completed connections for each pid */
>+  DBusHashTable *completed_by_cgroup; /**< Number of completed connections for each cgroup */
>   DBusTimeout *expire_timeout; /**< Timeout for expiring incomplete connections. */
>   int stamp;                   /**< Incrementing number */
>   BusExpireList *pending_replies; /**< List of pending replies */
>@@ -92,6 +93,8 @@ typedef struct
>   DBusPreallocatedSend *oom_preallocated;
>   BusClientPolicy *policy;
>   unsigned long pid;
>+  dbus_bool_t tracking_cgroup;
>+  DBusString cgroup;
> 
>   char *cached_loginfo_string;
>   BusSELinuxID *selinux_id;
>@@ -129,6 +132,23 @@ connection_get_loop (DBusConnection *connection)
> 
> 
> static int
>+get_connections_for_cgroup (BusConnections *connections,
>+                            DBusString     *cgroup)
>+{
>+  void *val;
>+  int current_count;
>+
>+  /* val is NULL is 0 when it isn't in the hash yet */
>+
>+  val = _dbus_hash_table_lookup_string (connections->completed_by_cgroup,
>+                                        _dbus_string_get_data (cgroup));
>+
>+  current_count = _DBUS_POINTER_TO_INT (val);
>+
>+  return current_count;
>+}
>+
>+static int
> get_connections_for_uid (BusConnections *connections,
>                          dbus_uid_t      uid)
> {
>@@ -163,6 +183,82 @@ get_connections_for_pid (BusConnections *connections,
> }
> 
> static dbus_bool_t
>+adjust_connections_for_cgroup (BusConnections *connections,
>+                               DBusString     *cgroup,
>+                               int             adjustment)
>+{
>+  int current_count;
>+  char *cgroup_str;
>+
>+  cgroup_str = _dbus_string_get_data (cgroup);
>+
>+  current_count = get_connections_for_cgroup (connections, cgroup);
>+
>+  _dbus_verbose ("Adjusting connection count for cgroup %s: "
>+                 "was %d adjustment %d making %d\n",
>+                 cgroup_str, current_count, adjustment,
>+                 current_count + adjustment);
>+
>+  _dbus_assert (current_count >= 0);
>+
>+  current_count += adjustment;
>+
>+  _dbus_assert (current_count >= 0);
>+
>+  if (current_count == 0)
>+    {
>+      _dbus_hash_table_remove_string (connections->completed_by_cgroup,
>+                                      cgroup_str);
>+      return TRUE;
>+    }
>+  else if (adjustment < 0)
>+    {
>+      dbus_bool_t retval;
>+      DBusHashIter iter;
>+
>+      /* Memory allocations are not allowed */
>+      retval = _dbus_hash_iter_lookup (connections->completed_by_cgroup,
>+                                       cgroup_str, FALSE, &iter);
>+      _dbus_assert (retval);
>+
>+      _dbus_hash_iter_set_value (&iter, _DBUS_INT_TO_POINTER (current_count));
>+
>+      return retval;
>+    }
>+  else
>+    {
>+      dbus_bool_t retval;
>+      DBusString copy;
>+
>+      /* duplicate the key */
>+
>+      if (!_dbus_string_init (&copy))
>+        return FALSE;
>+
>+      if (!_dbus_string_copy (cgroup, 0, &copy, 0))
>+        {
>+          _dbus_string_free (&copy);
>+          return FALSE;
>+        }
>+
>+      if (!_dbus_string_steal_data (&copy, &cgroup_str))
>+        {
>+          _dbus_string_free (&copy);
>+          return FALSE;
>+        }
>+
>+      _dbus_string_free (&copy);
>+
>+      retval = _dbus_hash_table_insert_string (connections->completed_by_cgroup,
>+                   cgroup_str, _DBUS_INT_TO_POINTER (current_count));
>+      if (!retval)
>+        dbus_free (cgroup_str);
>+
>+      return retval;
>+    }
>+}
>+
>+static dbus_bool_t
> adjust_connections_for_uid (BusConnections *connections,
>                             dbus_uid_t      uid,
>                             int             adjustment)
>@@ -346,6 +442,12 @@ bus_connection_disconnected (DBusConnection *connection)
>               if (!adjust_connections_for_uid (d->connections,
>                                                uid, -1))
>                 _dbus_assert_not_reached ("adjusting downward should never fail");
>+
>+            }
>+          if (d->tracking_cgroup)
>+            {
>+              if (!adjust_connections_for_cgroup (d->connections, &d->cgroup, -1))
>+                _dbus_assert_not_reached ("adjusting downward should never fail");
>             }
>           if (d->pid > 0)
>             {
>@@ -365,6 +467,8 @@ bus_connection_disconnected (DBusConnection *connection)
>       _dbus_assert (d->connections->n_completed >= 0);
>     }
> 
>+  _dbus_string_free (&d->cgroup);
>+
>   bus_connection_drop_pending_replies (d->connections, connection);
>   
>   /* frees "d" as side effect */
>@@ -500,11 +604,16 @@ bus_connections_new (BusContext *context)
>   if (connections->completed_by_process == NULL)
>     goto failed_3;
> 
>+  connections->completed_by_cgroup = _dbus_hash_table_new (DBUS_HASH_STRING,
>+                                                         dbus_free, NULL);
>+  if (connections->completed_by_cgroup == NULL)
>+    goto failed_4;
>+
>   connections->expire_timeout = _dbus_timeout_new (100, /* irrelevant */
>                                                    expire_incomplete_timeout,
>                                                    connections, NULL);
>   if (connections->expire_timeout == NULL)
>-    goto failed_4;
>+    goto failed_5;
> 
>   _dbus_timeout_set_enabled (connections->expire_timeout, FALSE);
> 
>@@ -513,21 +622,23 @@ bus_connections_new (BusContext *context)
>                                                       bus_pending_reply_expired,
>                                                       connections);
>   if (connections->pending_replies == NULL)
>-    goto failed_5;
>+    goto failed_6;
>   
>   if (!_dbus_loop_add_timeout (bus_context_get_loop (context),
>                                connections->expire_timeout))
>-    goto failed_6;
>+    goto failed_7;
>   
>   connections->refcount = 1;
>   connections->context = context;
>   
>   return connections;
> 
>- failed_6:
>+ failed_7:
>   bus_expire_list_free (connections->pending_replies);
>- failed_5:
>+ failed_6:
>   _dbus_timeout_unref (connections->expire_timeout);
>+ failed_5:
>+  _dbus_hash_table_unref (connections->completed_by_cgroup);
>  failed_4:
>   _dbus_hash_table_unref (connections->completed_by_process);
>  failed_3:
>@@ -595,6 +706,7 @@ bus_connections_unref (BusConnections *connections)
>       
>       _dbus_hash_table_unref (connections->completed_by_user);
>       _dbus_hash_table_unref (connections->completed_by_process);
>+      _dbus_hash_table_unref (connections->completed_by_cgroup);
>       
>       dbus_free (connections);
> 
>@@ -670,7 +782,7 @@ bus_connections_setup_connection (BusConnections *connections,
>   BusConnectionData *d;
>   dbus_bool_t retval;
>   DBusError error;
>-
>+  dbus_bool_t cgroup_release_if_oom = FALSE;
>   
>   d = dbus_new0 (BusConnectionData, 1);
>   
>@@ -735,6 +847,10 @@ bus_connections_setup_connection (BusConnections *connections,
>                                           allow_unix_user_function,
>                                           NULL, NULL);
> 
>+  if (!_dbus_string_init (&d->cgroup))
>+    goto out;
>+  cgroup_release_if_oom = TRUE;
>+
>   dbus_connection_set_dispatch_status_function (connection,
>                                                 dispatch_status_function,
>                                                 bus_context_get_loop (connections->context),
>@@ -812,6 +928,16 @@ bus_connections_setup_connection (BusConnections *connections,
>       dbus_connection_set_unix_user_function (connection,
>                                               NULL, NULL, NULL);
> 
>+      /* If d->cgroup has not been initialized with _dbus_string_init(), it
>+       * is still zerod as part of its container BusConnectionData. It must
>+       * not be released in this case (see assertions in _dbus_string_free
>+       * and DBUS_GENERIC_STRING_PREAMBLE).
>+       *
>+       * But it must be released in other cases.
>+       */
>+      if (cgroup_release_if_oom)
>+        _dbus_string_free (&d->cgroup);
>+
>       dbus_connection_set_windows_user_function (connection,
>                                                  NULL, NULL, NULL);
>       
>@@ -1434,6 +1560,7 @@ bus_connection_complete (DBusConnection   *connection,
>   unsigned long uid;
>   dbus_bool_t rollback_uid = FALSE;
>   dbus_bool_t rollback_pid = FALSE;
>+  dbus_bool_t rollback_cgroup = FALSE;
>   
>   d = BUS_CONNECTION_DATA (connection);
>   _dbus_assert (d != NULL);
>@@ -1473,12 +1600,18 @@ bus_connection_complete (DBusConnection   *connection,
>   
>   if (dbus_connection_get_unix_user (connection, &uid))
>     {
>-      if (!adjust_connections_for_uid (d->connections,
>-                                       uid, 1))
>+      if (!adjust_connections_for_uid (d->connections, uid, 1))
>         goto fail;
>       rollback_uid = TRUE;
>     }
> 
>+  if (d->tracking_cgroup)
>+    {
>+      if (!adjust_connections_for_cgroup (d->connections, &d->cgroup, 1))
>+        goto fail;
>+      rollback_cgroup = TRUE;
>+    }
>+
>   if (d->pid > 0)
>     {
>       if (!adjust_connections_for_pid (d->connections, d->pid, 1))
>@@ -1515,6 +1648,8 @@ fail:
>     adjust_connections_for_uid (d->connections, uid, -1);
>   if (rollback_pid)
>     adjust_connections_for_pid (d->connections, d->pid, -1);
>+  if (rollback_cgroup)
>+    adjust_connections_for_cgroup (d->connections, &d->cgroup, -1);
>   dbus_free (d->name);
>   d->name = NULL;
>   if (d->policy)
>@@ -1581,6 +1716,7 @@ bus_connections_check_limits (BusConnections  *connections,
> {
>   unsigned long uid;
>   unsigned long pid;
>+  int count;
>   BusConnectionData *d;
> 
>   d = BUS_CONNECTION_DATA (requesting_completion);
>@@ -1605,9 +1741,10 @@ bus_connections_check_limits (BusConnections  *connections,
>           return FALSE;
>         }
>     }
>-  
>+
>   /* bus_connections_check_limits() could be called several times on the same
>    * connection. Make sure to be idempotent. */
>+  
>   if (d->pid == 0 &&
>       dbus_connection_get_unix_process_id (requesting_completion, &pid))
>     {
>@@ -1626,6 +1763,38 @@ bus_connections_check_limits (BusConnections  *connections,
>         }
>     }
> 
>+  if (!d->tracking_cgroup && d->pid > 0)
>+    {
>+      d->tracking_cgroup = TRUE;
>+
>+      if (!_dbus_cgroup_for_pid (d->pid, &d->cgroup, 4096, error))
>+        {
>+          _dbus_verbose ("Couldn't get connection's cgroup\n");
>+          dbus_error_free (error);
>+          /* ignore the error: the connection will be accounted for cgroup ""
>+           */
>+          d->tracking_cgroup = FALSE;
>+        }
>+    }
>+
>+  _dbus_verbose ("Checking max_connections_per_cgroup? %d\n", d->tracking_cgroup);
>+  if (d->tracking_cgroup)
>+    {
>+      _dbus_verbose ("Checking max_connections_per_cgroup: current count=%d max=%d\n",
>+                     get_connections_for_cgroup (connections, &d->cgroup),
>+                     bus_context_get_max_connections_per_cgroup (connections->context));
>+
>+      count = get_connections_for_cgroup (connections, &d->cgroup);
>+      if (count >=
>+          bus_context_get_max_connections_per_cgroup (connections->context))
>+        {
>+          dbus_set_error (error, DBUS_ERROR_LIMITS_EXCEEDED,
>+                          "The maximum number of active connections for cgroup %s has been reached",
>+                          _dbus_string_get_data (&d->cgroup));
>+          return FALSE;
>+        }
>+    }
>+
>   return TRUE;
> }
> 
>-- 
>1.8.5.3
>

Actions: View | Splinter Review

Attachments on bug 81469: 102994 | 102995 | 102996 | 104444 | 104445 | 104446 | 104447 | 104664 | 104665 | 104666

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.