api.c: write dirty settings only in cgroup_set_values_recursive()

cgroup_set_values_recursive() is called by cgroup_modify_cgroup() to
modify controller values, where all the settings modified or not are
written to the disk.  This breaks when writing a new value of a setting
that is linked to another setting of the controller, followed by writing
an unmodified value to the linked setting. This effectively undoes the
modification. For example, consider two linked settings of the cpu
controller: cpu.weight and cpu.weight.nice, where writing the new value
of cpu.weight is followed by unmodified cpu.weight.nice value. Writing
of the latter will undo the new value of the former.

Reproducer:
-----------

int print_cpu_weight()
{
        FILE *fp;
        char value[10];

        fp = fopen(PROC_CPU_WEIGHT, "r");
        if (!fp) {
                fprintf(stderr, "Failed to open %s\n", PROC_CPU_WEIGHT);
                return 1;
        }

        fgets(value, 10, fp);
        fprintf(stderr, "cpu.weight %s", value);
        fclose(fp);

        return 0;
}

int main(void)
{
        struct cgroup_controller *cgc;
        struct cgroup *cgroup;
        int ret;

        ret = cgroup_init();
        if (ret) {
                fprintf(stderr, "cgroup initialization failed\n");
                exit (1);
        }

         /* Create */
        cgroup = cgroup_new_cgroup(CGRP_NAME);
        if (!cgroup) {
                fprintf(stderr, "Failed to allocate cgroup %s\n", CGRP_NAME);
                exit(1);
        }

        cgc = cgroup_add_controller(cgroup, CTRL_NAME);
        if (!cgc) {
                fprintf(stderr, "Failed to add controller %s\n", CTRL_NAME);
                exit (1);
        }

        ret = cgroup_create_cgroup(cgroup, 0);
        if (ret) {
                fprintf(stderr, "Failed to create cgroup %s\n", CGRP_NAME);
                goto out;
        }

        ret = print_cpu_weight();
        if (ret)
                goto out;

        cgroup_free(&cgroup);

        /* Load and modify */
        cgroup = cgroup_new_cgroup(CGRP_NAME);
        if (!cgroup) {
                fprintf(stderr, "Failed to allocate cgroup %s\n", CGRP_NAME);
                exit(1);
        }

        ret = cgroup_get_cgroup(cgroup);
        if (ret) {
                fprintf(stderr, "Failed to get cgroup %s\n", CGRP_NAME);
                goto out;
        }

        cgc = NULL;
        cgc = cgroup_get_controller(cgroup, CTRL_NAME);
        if (!cgc) {
                fprintf(stderr, "Failed to get controller %s\n", CTRL_NAME);
                exit (1);
        }

        ret = cgroup_set_value_string(cgc, CTRL_SETTING, "8");
        if (ret) {
                fprintf(stderr, "Failed to set the %s value\n", CTRL_SETTING);
                goto out;
        }

        ret = cgroup_modify_cgroup(cgroup);
        if (ret) {
                fprintf(stderr, "Failed to modify cgroup\n");\
                goto out;
        }

        ret = print_cpu_weight();
        if (ret)
                goto out;

out:
        cgroup_free(&cgroup);
        return 0;
}

This patch additionally cleans up cgroup_set_values_recursive(), by
renaming the third argument ignore_non_dirty_failure to
ignore_non_dirty_values.  This rename also changes the purpose of the
flag, where the calling functions, set it to ignore the writing of the
controller setting, which is not modified/dirty and introduces extensive
checks for writing the controller setting.

Fixes: https://github.com/libcgroup/libcgroup/issues/323
Reported-by: Justin Israel <justinisrael@gmail.com>
[Justin contributed to the reproducer]
Signed-off-by: Kamalesh Babulal <kamalesh.babulal@oracle.com>
Signed-off-by: Tom Hromatka <tom.hromatka@oracle.com>

diff --git a/src/api.c b/src/api.c
index ad08bd3706704b1ef0fb38e1a0cdb1ced8bf7f35..14f703dce88b76f8388b8ab836f1357cddf5d971 100644 (file)
--- a/src/api.c
+++ b/src/api.c
@@ -2287,41 +2287,73 @@ static int cg_set_control_value(char *path, const char *val)
  *
  * @param base The full path to the base of this cgroup
  * @param controller The controller whose values are being updated
+ * @param ignore_non_dirty_values If set skips writing non-dirty controller settings
  */
 STATIC int cgroup_set_values_recursive(const char * const base,
                                       const struct cgroup_controller * const controller,
-                                      bool ignore_non_dirty_failures)
+                                      bool ignore_non_dirty_values)
 {
+       struct control_value *cv;
+       struct stat path_stat;
        int ret, j, error = 0;
        char *path = NULL;
 
        for (j = 0; j < controller->index; j++) {
-               ret = asprintf(&path, "%s%s", base, controller->values[j]->name);
+               cv = controller->values[j];
+
+               /*
+                * ignore_non_dirty_values is set while writing into
+                * existing cgroup to modify controller settings and
+                * unset during cgroup creation.  The subtle difference
+                * is that dirty flag is unset for all the controller
+                * settings during cgroup creation, whereas some or all
+                * controller settings have the dirty flag set during
+                * modification.
+                *
+                * Be careful with ignore_non_dirty_values flag, setting
+                * it writing only the controller settings that has it
+                * dirty value set.
+                */
+               if (ignore_non_dirty_values && cv->dirty == false)
+                       continue;
+
+               /* We don't support, writing multiline settings */
+               if (strcspn(cv->value, "\n")  < (strlen(cv->value) - 1))
+                       continue;
+
+               ret = asprintf(&path, "%s%s", base, cv->name);
                if (ret < 0) {
                        last_errno = errno;
                        error = ECGOTHER;
                        goto err;
                }
-               cgroup_dbg("setting %s to \"%s\", pathlen %d\n", path,
-                          controller->values[j]->value, ret);
 
-               error = cg_set_control_value(path, controller->values[j]->value);
-               free(path);
-               path = NULL;
+               /* skip read-only settings */
+               ret = (stat(path, &path_stat));
+               if (ret < 0) {
+                       last_errno = errno;
+                       error = ECGROUPVALUENOTEXIST;;
+                       goto err;
+               }
 
-               if (error && ignore_non_dirty_failures && !controller->values[j]->dirty) {
-                       /*
-                        * We failed to set this value, but it wasn't marked
-                        * as dirty, so ignore the failure.
-                        */
-                       error = 0;
+               if (!(path_stat.st_mode & S_IWUSR))
                        continue;
-               }
 
-               if (error)
-                       goto err;
+               cgroup_dbg("setting %s to \"%s\", pathlen %d\n", path, cv->value, ret);
 
-               controller->values[j]->dirty = false;
+               error = cg_set_control_value(path, cv->value);
+               free(path);
+               path = NULL;
+
+               if (error) {
+                       /* Ignore the errors on deprecated settings */
+                       if (last_errno == EOPNOTSUPP) {
+                               error = 0;
+                               continue;
+                       }
+                       goto err;
+               }
+               cv->dirty = false;
        }
 
 err: