Skip to content

Commit 0985c7c

Browse files
keszybzkeszybz
committed
Rework cpu affinity parsing
The CPU_SET_S api is pretty bad. In particular, it has a parameter for the size of the array, but operations which take two (CPU_EQUAL_S) or even three arrays (CPU_{AND,OR,XOR}_S) still take just one size. This means that all arrays must be of the same size, or buffer overruns will occur. This is exactly what our code would do, if it received an array of unexpected size over the network. ("Unexpected" here means anything different from what cpu_set_malloc() detects as the "right" size.) Let's rework this, and store the size in bytes of the allocated storage area. The code will now parse any number up to 8191, independently of what the current kernel supports. This matches the kernel maximum setting for any architecture, to make things more portable. Fixes systemd#12605.
1 parent b12ef71 commit 0985c7c

File tree

14 files changed

+295
-256
lines changed

14 files changed

+295
-256
lines changed

‎src/core/dbus-execute.c‎

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ static int property_get_cpu_affinity(
219219
assert(reply);
220220
assert(c);
221221

222-
return sd_bus_message_append_array(reply, 'y', c->cpuset, CPU_ALLOC_SIZE(c->cpuset_ncpus));
222+
return sd_bus_message_append_array(reply, 'y', c->cpu_set.set, c->cpu_set.allocated);
223223
}
224224

225225
static int property_get_timer_slack_nsec(
@@ -1566,37 +1566,22 @@ int bus_exec_context_set_transient_property(
15661566

15671567
if (!UNIT_WRITE_FLAGS_NOOP(flags)) {
15681568
if (n == 0) {
1569-
c->cpuset = cpu_set_mfree(c->cpuset);
1570-
c->cpuset_ncpus = 0;
1569+
cpu_set_reset(&c->cpu_set);
15711570
unit_write_settingf(u, flags, name, "%s=", name);
15721571
} else {
15731572
_cleanup_free_ char *str = NULL;
1574-
size_t ncpus;
1573+
const CPUSet set = {(cpu_set_t*) a, n};
15751574

1576-
str = cpu_set_to_string(a, n);
1575+
str = cpu_set_to_string(&set);
15771576
if (!str)
15781577
return -ENOMEM;
15791578

1580-
ncpus = CPU_SIZE_TO_NUM(n);
1581-
1582-
if (!c->cpuset || c->cpuset_ncpus < ncpus) {
1583-
cpu_set_t *cpuset;
1584-
1585-
cpuset = CPU_ALLOC(ncpus);
1586-
if (!cpuset)
1587-
return -ENOMEM;
1588-
1589-
CPU_ZERO_S(n, cpuset);
1590-
if (c->cpuset) {
1591-
CPU_OR_S(CPU_ALLOC_SIZE(c->cpuset_ncpus), cpuset, c->cpuset, (cpu_set_t*) a);
1592-
CPU_FREE(c->cpuset);
1593-
} else
1594-
CPU_OR_S(n, cpuset, cpuset, (cpu_set_t*) a);
1595-
1596-
c->cpuset = cpuset;
1597-
c->cpuset_ncpus = ncpus;
1598-
} else
1599-
CPU_OR_S(n, c->cpuset, c->cpuset, (cpu_set_t*) a);
1579+
/* We forego any optimizations here, and always create the structure using
1580+
* cpu_set_add_all(), because we don't want to care if the existing size we
1581+
* got over dbus is appropriate. */
1582+
r = cpu_set_add_all(&c->cpu_set, &set);
1583+
if (r < 0)
1584+
return r;
16001585

16011586
unit_write_settingf(u, flags, name, "%s=%s", name, str);
16021587
}

‎src/core/execute.c‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3144,8 +3144,8 @@ static int exec_child(
31443144
}
31453145
}
31463146

3147-
if (context->cpuset)
3148-
if (sched_setaffinity(0, CPU_ALLOC_SIZE(context->cpuset_ncpus), context->cpuset) < 0) {
3147+
if (context->cpu_set.set)
3148+
if (sched_setaffinity(0, context->cpu_set.allocated, context->cpu_set.set) < 0) {
31493149
*exit_status = EXIT_CPUAFFINITY;
31503150
return log_unit_error_errno(unit, errno, "Failed to set up CPU affinity: %m");
31513151
}
@@ -3896,7 +3896,7 @@ void exec_context_done(ExecContext *c) {
38963896
c->temporary_filesystems = NULL;
38973897
c->n_temporary_filesystems = 0;
38983898

3899-
c->cpuset = cpu_set_mfree(c->cpuset);
3899+
cpu_set_reset(&c->cpu_set);
39003900

39013901
c->utmp_id = mfree(c->utmp_id);
39023902
c->selinux_context = mfree(c->selinux_context);
@@ -4328,10 +4328,10 @@ void exec_context_dump(const ExecContext *c, FILE* f, const char *prefix) {
43284328
prefix, yes_no(c->cpu_sched_reset_on_fork));
43294329
}
43304330

4331-
if (c->cpuset) {
4331+
if (c->cpu_set.set) {
43324332
fprintf(f, "%sCPUAffinity:", prefix);
4333-
for (i = 0; i < c->cpuset_ncpus; i++)
4334-
if (CPU_ISSET_S(i, CPU_ALLOC_SIZE(c->cpuset_ncpus), c->cpuset))
4333+
for (i = 0; i < c->cpu_set.allocated * 8; i++)
4334+
if (CPU_ISSET_S(i, c->cpu_set.allocated, c->cpu_set.set))
43354335
fprintf(f, " %u", i);
43364336
fputs("\n", f);
43374337
}

‎src/core/execute.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ typedef struct Manager Manager;
1414
#include <sys/capability.h>
1515

1616
#include "cgroup-util.h"
17+
#include "cpu-set-util.h"
1718
#include "fdset.h"
1819
#include "list.h"
1920
#include "missing_resource.h"
@@ -172,8 +173,7 @@ struct ExecContext {
172173
int cpu_sched_policy;
173174
int cpu_sched_priority;
174175

175-
unsigned cpuset_ncpus;
176-
cpu_set_t *cpuset;
176+
CPUSet cpu_set;
177177

178178
ExecInput std_input;
179179
ExecOutput std_output;

‎src/core/load-fragment.c‎

Lines changed: 1 addition & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1263,42 +1263,13 @@ int config_parse_exec_cpu_affinity(const char *unit,
12631263
void *userdata) {
12641264

12651265
ExecContext *c = data;
1266-
_cleanup_cpu_free_ cpu_set_t *cpuset = NULL;
1267-
int ncpus;
12681266

12691267
assert(filename);
12701268
assert(lvalue);
12711269
assert(rvalue);
12721270
assert(data);
12731271

1274-
ncpus = parse_cpu_set_and_warn(rvalue, &cpuset, unit, filename, line, lvalue);
1275-
if (ncpus < 0)
1276-
return ncpus;
1277-
1278-
if (ncpus == 0) {
1279-
/* An empty assignment resets the CPU list */
1280-
c->cpuset = cpu_set_mfree(c->cpuset);
1281-
c->cpuset_ncpus = 0;
1282-
return 0;
1283-
}
1284-
1285-
if (!c->cpuset) {
1286-
c->cpuset = TAKE_PTR(cpuset);
1287-
c->cpuset_ncpus = (unsigned) ncpus;
1288-
return 0;
1289-
}
1290-
1291-
if (c->cpuset_ncpus < (unsigned) ncpus) {
1292-
CPU_OR_S(CPU_ALLOC_SIZE(c->cpuset_ncpus), cpuset, c->cpuset, cpuset);
1293-
CPU_FREE(c->cpuset);
1294-
c->cpuset = TAKE_PTR(cpuset);
1295-
c->cpuset_ncpus = (unsigned) ncpus;
1296-
return 0;
1297-
}
1298-
1299-
CPU_OR_S(CPU_ALLOC_SIZE((unsigned) ncpus), c->cpuset, c->cpuset, cpuset);
1300-
1301-
return 0;
1272+
return parse_cpu_set_extend(rvalue, &c->cpu_set, true, unit, filename, line, lvalue);
13021273
}
13031274

13041275
int config_parse_capability_set(

‎src/core/main.c‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -567,16 +567,18 @@ static int config_parse_cpu_affinity2(
567567
void *data,
568568
void *userdata) {
569569

570-
_cleanup_cpu_free_ cpu_set_t *c = NULL;
571-
int ncpus;
570+
_cleanup_(cpu_set_reset) CPUSet c = {};
571+
int r;
572572

573-
ncpus = parse_cpu_set_and_warn(rvalue, &c, unit, filename, line, lvalue);
574-
if (ncpus < 0)
575-
return ncpus;
573+
r = parse_cpu_set_full(rvalue, &c, true, unit, filename, line, lvalue);
574+
if (r < 0)
575+
return r;
576576

577-
if (sched_setaffinity(0, CPU_ALLOC_SIZE(ncpus), c) < 0)
577+
if (sched_setaffinity(0, c.allocated, c.set) < 0)
578578
log_warning_errno(errno, "Failed to set CPU affinity: %m");
579579

580+
// FIXME: parsing and execution should be seperated.
581+
580582
return 0;
581583
}
582584

‎src/nspawn/nspawn-oci.c‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1266,8 +1266,7 @@ struct cpu_data {
12661266
uint64_t shares;
12671267
uint64_t quota;
12681268
uint64_t period;
1269-
cpu_set_t *cpuset;
1270-
unsigned ncpus;
1269+
CPUSet cpu_set;
12711270
};
12721271

12731272
static int oci_cgroup_cpu_shares(const char *name, JsonVariant *v, JsonDispatchFlags flags, void *userdata) {
@@ -1302,21 +1301,20 @@ static int oci_cgroup_cpu_quota(const char *name, JsonVariant *v, JsonDispatchFl
13021301

13031302
static int oci_cgroup_cpu_cpus(const char *name, JsonVariant *v, JsonDispatchFlags flags, void *userdata) {
13041303
struct cpu_data *data = userdata;
1305-
cpu_set_t *set;
1304+
CPUSet set;
13061305
const char *n;
1307-
int ncpus;
1306+
int r;
13081307

13091308
assert(data);
13101309

13111310
assert_se(n = json_variant_string(v));
13121311

1313-
ncpus = parse_cpu_set(n, &set);
1314-
if (ncpus < 0)
1315-
return json_log(v, flags, ncpus, "Failed to parse CPU set specification: %s", n);
1312+
r = parse_cpu_set(n, &set);
1313+
if (r < 0)
1314+
return json_log(v, flags, r, "Failed to parse CPU set specification: %s", n);
13161315

1317-
CPU_FREE(data->cpuset);
1318-
data->cpuset = set;
1319-
data->ncpus = ncpus;
1316+
cpu_set_reset(&data->cpu_set);
1317+
data->cpu_set = set;
13201318

13211319
return 0;
13221320
}
@@ -1345,13 +1343,12 @@ static int oci_cgroup_cpu(const char *name, JsonVariant *v, JsonDispatchFlags fl
13451343

13461344
r = json_dispatch(v, table, oci_unexpected, flags, &data);
13471345
if (r < 0) {
1348-
CPU_FREE(data.cpuset);
1346+
cpu_set_reset(&data.cpu_set);
13491347
return r;
13501348
}
13511349

1352-
CPU_FREE(s->cpuset);
1353-
s->cpuset = data.cpuset;
1354-
s->cpuset_ncpus = data.ncpus;
1350+
cpu_set_reset(&s->cpu_set);
1351+
s->cpu_set = data.cpu_set;
13551352

13561353
if (data.shares != UINT64_MAX) {
13571354
r = settings_allocate_properties(s);

‎src/nspawn/nspawn-settings.c‎

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ Settings* settings_free(Settings *s) {
133133
strv_free(s->syscall_blacklist);
134134
rlimit_free_all(s->rlimit);
135135
free(s->hostname);
136-
s->cpuset = cpu_set_mfree(s->cpuset);
136+
cpu_set_reset(&s->cpu_set);
137137

138138
strv_free(s->network_interfaces);
139139
strv_free(s->network_macvlan);
@@ -803,41 +803,12 @@ int config_parse_cpu_affinity(
803803
void *data,
804804
void *userdata) {
805805

806-
_cleanup_cpu_free_ cpu_set_t *cpuset = NULL;
807806
Settings *settings = data;
808-
int ncpus;
809807

810808
assert(rvalue);
811809
assert(settings);
812810

813-
ncpus = parse_cpu_set_and_warn(rvalue, &cpuset, unit, filename, line, lvalue);
814-
if (ncpus < 0)
815-
return ncpus;
816-
817-
if (ncpus == 0) {
818-
/* An empty assignment resets the CPU list */
819-
settings->cpuset = cpu_set_mfree(settings->cpuset);
820-
settings->cpuset_ncpus = 0;
821-
return 0;
822-
}
823-
824-
if (!settings->cpuset) {
825-
settings->cpuset = TAKE_PTR(cpuset);
826-
settings->cpuset_ncpus = (unsigned) ncpus;
827-
return 0;
828-
}
829-
830-
if (settings->cpuset_ncpus < (unsigned) ncpus) {
831-
CPU_OR_S(CPU_ALLOC_SIZE(settings->cpuset_ncpus), cpuset, settings->cpuset, cpuset);
832-
CPU_FREE(settings->cpuset);
833-
settings->cpuset = TAKE_PTR(cpuset);
834-
settings->cpuset_ncpus = (unsigned) ncpus;
835-
return 0;
836-
}
837-
838-
CPU_OR_S(CPU_ALLOC_SIZE((unsigned) ncpus), settings->cpuset, settings->cpuset, cpuset);
839-
840-
return 0;
811+
return parse_cpu_set_extend(rvalue, &settings->cpu_set, true, unit, filename, line, lvalue);
841812
}
842813

843814
DEFINE_CONFIG_PARSE_ENUM(config_parse_resolv_conf, resolv_conf_mode, ResolvConfMode, "Failed to parse resolv.conf mode");

‎src/nspawn/nspawn-settings.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
#include "capability-util.h"
1515
#include "conf-parser.h"
16+
#include "cpu-set-util.h"
1617
#include "macro.h"
1718
#include "missing_resource.h"
1819
#include "nspawn-expose-ports.h"
@@ -163,8 +164,7 @@ typedef struct Settings {
163164
int no_new_privileges;
164165
int oom_score_adjust;
165166
bool oom_score_adjust_set;
166-
cpu_set_t *cpuset;
167-
unsigned cpuset_ncpus;
167+
CPUSet cpu_set;
168168
ResolvConfMode resolv_conf;
169169
LinkJournal link_journal;
170170
bool link_journal_try;

‎src/nspawn/nspawn.c‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -220,8 +220,7 @@ static struct rlimit *arg_rlimit[_RLIMIT_MAX] = {};
220220
static bool arg_no_new_privileges = false;
221221
static int arg_oom_score_adjust = 0;
222222
static bool arg_oom_score_adjust_set = false;
223-
static cpu_set_t *arg_cpuset = NULL;
224-
static unsigned arg_cpuset_ncpus = 0;
223+
static CPUSet arg_cpu_set = {};
225224
static ResolvConfMode arg_resolv_conf = RESOLV_CONF_AUTO;
226225
static TimezoneMode arg_timezone = TIMEZONE_AUTO;
227226
static unsigned arg_console_width = (unsigned) -1, arg_console_height = (unsigned) -1;
@@ -259,7 +258,7 @@ STATIC_DESTRUCTOR_REGISTER(arg_syscall_blacklist, strv_freep);
259258
#if HAVE_SECCOMP
260259
STATIC_DESTRUCTOR_REGISTER(arg_seccomp, seccomp_releasep);
261260
#endif
262-
STATIC_DESTRUCTOR_REGISTER(arg_cpuset, CPU_FREEp);
261+
STATIC_DESTRUCTOR_REGISTER(arg_cpu_set, cpu_set_reset);
263262
STATIC_DESTRUCTOR_REGISTER(arg_sysctl, strv_freep);
264263

265264
static int help(void) {
@@ -1329,17 +1328,14 @@ static int parse_argv(int argc, char *argv[]) {
13291328
break;
13301329

13311330
case ARG_CPU_AFFINITY: {
1332-
_cleanup_cpu_free_ cpu_set_t *cpuset = NULL;
1331+
CPUSet cpuset;
13331332

13341333
r = parse_cpu_set(optarg, &cpuset);
13351334
if (r < 0)
1336-
return log_error_errno(r, "Failed to parse CPU affinity mask: %s", optarg);
1335+
return log_error_errno(r, "Failed to parse CPU affinity mask %s: %m", optarg);
13371336

1338-
if (arg_cpuset)
1339-
CPU_FREE(arg_cpuset);
1340-
1341-
arg_cpuset = TAKE_PTR(cpuset);
1342-
arg_cpuset_ncpus = r;
1337+
cpu_set_reset(&arg_cpu_set);
1338+
arg_cpu_set = cpuset;
13431339
arg_settings_mask |= SETTING_CPU_AFFINITY;
13441340
break;
13451341
}
@@ -2922,8 +2918,8 @@ static int inner_child(
29222918
return log_error_errno(r, "Failed to adjust OOM score: %m");
29232919
}
29242920

2925-
if (arg_cpuset)
2926-
if (sched_setaffinity(0, CPU_ALLOC_SIZE(arg_cpuset_ncpus), arg_cpuset) < 0)
2921+
if (arg_cpu_set.set)
2922+
if (sched_setaffinity(0, arg_cpu_set.allocated, arg_cpu_set.set) < 0)
29272923
return log_error_errno(errno, "Failed to set CPU affinity: %m");
29282924

29292925
(void) setup_hostname();
@@ -3869,15 +3865,14 @@ static int merge_settings(Settings *settings, const char *path) {
38693865
}
38703866

38713867
if ((arg_settings_mask & SETTING_CPU_AFFINITY) == 0 &&
3872-
settings->cpuset) {
3868+
settings->cpu_set.set) {
38733869

38743870
if (!arg_settings_trusted)
38753871
log_warning("Ignoring CPUAffinity= setting, file '%s' is not trusted.", path);
38763872
else {
3877-
if (arg_cpuset)
3878-
CPU_FREE(arg_cpuset);
3879-
arg_cpuset = TAKE_PTR(settings->cpuset);
3880-
arg_cpuset_ncpus = settings->cpuset_ncpus;
3873+
cpu_set_reset(&arg_cpu_set);
3874+
arg_cpu_set = settings->cpu_set;
3875+
settings->cpu_set = (CPUSet) {};
38813876
}
38823877
}
38833878

‎src/shared/bus-unit-util.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -992,13 +992,13 @@ static int bus_append_execute_property(sd_bus_message *m, const char *field, con
992992
}
993993

994994
if (streq(field, "CPUAffinity")) {
995-
_cleanup_cpu_free_ cpu_set_t *cpuset = NULL;
995+
_cleanup_(cpu_set_reset) CPUSet cpuset = {};
996996

997997
r = parse_cpu_set(eq, &cpuset);
998998
if (r < 0)
999999
return log_error_errno(r, "Failed to parse %s value: %s", field, eq);
10001000

1001-
return bus_append_byte_array(m, field, cpuset, CPU_ALLOC_SIZE(r));
1001+
return bus_append_byte_array(m, field, cpuset.set, cpuset.allocated);
10021002
}
10031003

10041004
if (STR_IN_SET(field, "RestrictAddressFamilies", "SystemCallFilter")) {

0 commit comments

Comments
 (0)