[lxc-devel] [lxc/master] tree-wide: logging fixes and hardening
brauner on Github
lxc-bot at linuxcontainers.org
Thu Mar 19 13:37:33 UTC 2020
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 364 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200319/30f67a59/attachment.bin>
-------------- next part --------------
From fc3b95335b6faad8ae1299fa3b331d4e32cb595f Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Thu, 19 Mar 2020 14:27:29 +0100
Subject: [PATCH 1/2] cgroups: don't call statements from loggers
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
src/lxc/cgroups/cgfsng.c | 105 ++++++++++++++++++++++++---------------
1 file changed, 65 insertions(+), 40 deletions(-)
diff --git a/src/lxc/cgroups/cgfsng.c b/src/lxc/cgroups/cgfsng.c
index b78779ef77..8edaa54c9f 100644
--- a/src/lxc/cgroups/cgfsng.c
+++ b/src/lxc/cgroups/cgfsng.c
@@ -1002,17 +1002,23 @@ __cgfsng_ops static void cgfsng_payload_destroy(struct cgroup_ops *ops,
{
int ret;
- if (!ops)
- log_error_errno(return, ENOENT, "Called with uninitialized cgroup operations");
+ if (!ops) {
+ ERROR("Called with uninitialized cgroup operations");
+ return;
+ }
if (!ops->hierarchies)
return;
- if (!handler)
- log_error_errno(return, EINVAL, "Called with uninitialized handler");
+ if (!handler) {
+ ERROR("Called with uninitialized handler");
+ return;
+ }
- if (!handler->conf)
- log_error_errno(return, EINVAL, "Called with uninitialized conf");
+ if (!handler->conf) {
+ ERROR("Called with uninitialized conf");
+ return;
+ }
#ifdef HAVE_STRUCT_BPF_CGROUP_DEV_CTX
ret = bpf_program_cgroup_detach(handler->conf->cgroup2_devices);
@@ -1033,7 +1039,7 @@ __cgfsng_ops static void cgfsng_payload_destroy(struct cgroup_ops *ops,
ret = cgroup_rmdir(ops->hierarchies, ops->container_cgroup);
}
if (ret < 0)
- log_warn_errno(return, errno, "Failed to destroy cgroups");
+ SYSWARN("Failed to destroy cgroups");
}
__cgfsng_ops static void cgfsng_monitor_destroy(struct cgroup_ops *ops,
@@ -1043,17 +1049,23 @@ __cgfsng_ops static void cgfsng_monitor_destroy(struct cgroup_ops *ops,
char pidstr[INTTYPE_TO_STRLEN(pid_t)];
const struct lxc_conf *conf;
- if (!ops)
- log_error_errno(return, ENOENT, "Called with uninitialized cgroup operations");
+ if (!ops) {
+ ERROR("Called with uninitialized cgroup operations");
+ return;
+ }
if (!ops->hierarchies)
return;
- if (!handler)
- log_error_errno(return, EINVAL, "Called with uninitialized handler");
+ if (!handler) {
+ ERROR("Called with uninitialized handler");
+ return;
+ }
- if (!handler->conf)
- log_error_errno(return, EINVAL, "Called with uninitialized conf");
+ if (!handler->conf) {
+ ERROR("Called with uninitialized conf");
+ return;
+ }
conf = handler->conf;
len = snprintf(pidstr, sizeof(pidstr), "%d", handler->monitor_pid);
@@ -1079,15 +1091,16 @@ __cgfsng_ops static void cgfsng_monitor_destroy(struct cgroup_ops *ops,
CGROUP_PIVOT, NULL);
ret = mkdir_p(pivot_path, 0755);
- if (ret < 0 && errno != EEXIST)
- log_error_errno(goto try_recursive_destroy, errno,
- "Failed to create %s", pivot_path);
+ if (ret < 0 && errno != EEXIST) {
+ ERROR("Failed to create %s", pivot_path);
+ goto try_recursive_destroy;
+ }
ret = lxc_write_openat(pivot_path, "cgroup.procs", pidstr, len);
- if (ret != 0)
- log_warn_errno(continue, errno,
- "Failed to move monitor %s to \"%s\"",
- pidstr, pivot_path);
+ if (ret != 0) {
+ SYSWARN("Failed to move monitor %s to \"%s\"", pidstr, pivot_path);
+ continue;
+ }
try_recursive_destroy:
ret = recursive_destroy(h->monitor_full_path);
@@ -1741,10 +1754,10 @@ __cgfsng_ops static bool cgfsng_mount(struct cgroup_ops *ops,
continue;
ret = mkdir(controllerpath, 0755);
- if (ret < 0)
- log_error_errno(goto on_error, errno,
- "Error creating cgroup path: %s",
- controllerpath);
+ if (ret < 0) {
+ ERROR("Error creating cgroup path: %s", controllerpath);
+ goto on_error;
+ }
if (has_cgns && wants_force_mount) {
/* If cgroup namespaces are supported but the container
@@ -2544,11 +2557,12 @@ __cgfsng_ops static bool cgfsng_setup_limits_legacy(struct cgroup_ops *ops,
if (do_devices == !strncmp("devices", cg->subsystem, 7)) {
if (cg_legacy_set_data(ops, cg->subsystem, cg->value)) {
- if (do_devices && (errno == EACCES || errno == EPERM))
- log_warn_errno(continue, errno, "Failed to set \"%s\" to \"%s\"",
- cg->subsystem, cg->value);
- log_warn_errno(goto out, errno, "Failed to set \"%s\" to \"%s\"",
- cg->subsystem, cg->value);
+ if (do_devices && (errno == EACCES || errno == EPERM)) {
+ SYSWARN("Failed to set \"%s\" to \"%s\"", cg->subsystem, cg->value);
+ continue;
+ }
+ SYSERROR("Failed to set \"%s\" to \"%s\"", cg->subsystem, cg->value);
+ goto out;
}
DEBUG("Set controller \"%s\" set to \"%s\"", cg->subsystem, cg->value);
}
@@ -2830,7 +2844,8 @@ static void cg_unified_delegate(char ***delegate)
idx = append_null_to_list((void ***)delegate);
(*delegate)[idx] = must_copy_string(*p);
}
- log_warn_errno(return, errno, "Failed to read /sys/kernel/cgroup/delegate");
+ SYSWARN("Failed to read /sys/kernel/cgroup/delegate");
+ return;
}
lxc_iterate_parts (token, buf, " \t\n") {
@@ -2909,19 +2924,25 @@ static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileg
continue;
if (type == CGROUP_SUPER_MAGIC)
- if (controller_list_is_dup(ops->hierarchies, controller_list))
- log_trace_errno(continue, EEXIST, "Skipping duplicating controller");
+ if (controller_list_is_dup(ops->hierarchies, controller_list)) {
+ TRACE("Skipping duplicating controller");
+ continue;
+ }
mountpoint = cg_hybrid_get_mountpoint(line);
- if (!mountpoint)
- log_error_errno(continue, EINVAL, "Failed parsing mountpoint from \"%s\"", line);
+ if (!mountpoint) {
+ ERROR("Failed parsing mountpoint from \"%s\"", line);
+ continue;
+ }
if (type == CGROUP_SUPER_MAGIC)
base_cgroup = cg_hybrid_get_current_cgroup(basecginfo, controller_list[0], CGROUP_SUPER_MAGIC);
else
base_cgroup = cg_hybrid_get_current_cgroup(basecginfo, NULL, CGROUP2_SUPER_MAGIC);
- if (!base_cgroup)
- log_error_errno(continue, EINVAL, "Failed to find current cgroup");
+ if (!base_cgroup) {
+ ERROR("Failed to find current cgroup");
+ continue;
+ }
trim(base_cgroup);
prune_init_scope(base_cgroup);
@@ -2929,8 +2950,10 @@ static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileg
writeable = test_writeable_v2(mountpoint, base_cgroup);
else
writeable = test_writeable_v1(mountpoint, base_cgroup);
- if (!writeable)
- log_trace_errno(continue, EROFS, "The %s group is not writeable", base_cgroup);
+ if (!writeable) {
+ TRACE("The %s group is not writeable", base_cgroup);
+ continue;
+ }
if (type == CGROUP2_SUPER_MAGIC) {
char *cgv2_ctrl_path;
@@ -2949,8 +2972,10 @@ static int cg_hybrid_init(struct cgroup_ops *ops, bool relative, bool unprivileg
}
/* Exclude all controllers that cgroup use does not want. */
- if (!cgroup_use_wants_controllers(ops, controller_list))
- log_trace_errno(continue, EINVAL, "Skipping controller");
+ if (!cgroup_use_wants_controllers(ops, controller_list)) {
+ TRACE("Skipping controller");
+ continue;
+ }
new = add_hierarchy(&ops->hierarchies, move_ptr(controller_list), move_ptr(mountpoint), move_ptr(base_cgroup), type);
if (type == CGROUP2_SUPER_MAGIC && !ops->unified) {
From d7d1e27a71baa7d99fc47816d568d52d44a1c82b Mon Sep 17 00:00:00 2001
From: Christian Brauner <christian.brauner at ubuntu.com>
Date: Thu, 19 Mar 2020 14:28:02 +0100
Subject: [PATCH 2/2] log: use global variable to catch statements in loggers
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
---
src/lxc/cgroups/cgroup2_devices.c | 2 +-
src/lxc/log.h | 100 ++++++++++++++++--------------
src/lxc/macro.h | 27 ++++----
3 files changed, 69 insertions(+), 60 deletions(-)
diff --git a/src/lxc/cgroups/cgroup2_devices.c b/src/lxc/cgroups/cgroup2_devices.c
index 11e2bdd986..4efb28fbd7 100644
--- a/src/lxc/cgroups/cgroup2_devices.c
+++ b/src/lxc/cgroups/cgroup2_devices.c
@@ -385,7 +385,7 @@ int bpf_program_cgroup_attach(struct bpf_program *prog, int type,
if (ret < 0)
return log_error_errno(-1, errno, "Failed to attach bpf program");
- free_replace_move_ptr(prog->attached_path, copy);
+ free_move_ptr(prog->attached_path, copy);
prog->attached_type = type;
prog->attached_flags = flags;
diff --git a/src/lxc/log.h b/src/lxc/log.h
index c00638cb74..ec10f53bc5 100644
--- a/src/lxc/log.h
+++ b/src/lxc/log.h
@@ -477,69 +477,79 @@ __lxc_unused static inline void LXC_##LEVEL(struct lxc_log_locinfo* locinfo, \
} while (0)
#endif
-#define log_error_errno(__ret__, __errno__, format, ...) \
- ({ \
- errno = __errno__; \
- SYSERROR(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_error_errno(__ret__, __errno__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ errno = (__errno__); \
+ SYSERROR(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_error(__ret__, format, ...) \
- ({ \
- ERROR(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_error(__ret__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ ERROR(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_trace_errno(__ret__, __errno__, format, ...) \
- ({ \
- errno = __errno__; \
- SYSTRACE(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_trace_errno(__ret__, __errno__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ errno = __errno__; \
+ SYSTRACE(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_trace(__ret__, format, ...) \
- ({ \
- TRACE(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_trace(__ret__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ TRACE(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_warn_errno(__ret__, __errno__, format, ...) \
- ({ \
- errno = __errno__; \
- SYSWARN(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_warn_errno(__ret__, __errno__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ errno = __errno__; \
+ SYSWARN(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_warn(__ret__, format, ...) \
- ({ \
- WARN(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_warn(__ret__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ WARN(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_debug_errno(__ret__, __errno__, format, ...) \
- ({ \
- errno = __errno__; \
- SYSDEBUG(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_debug_errno(__ret__, __errno__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ errno = __errno__; \
+ SYSDEBUG(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_debug(__ret__, format, ...) \
- ({ \
- DEBUG(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_debug(__ret__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ DEBUG(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_info_errno(__ret__, __errno__, format, ...) \
- ({ \
- errno = __errno__; \
- SYSINFO(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_info_errno(__ret__, __errno__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ errno = __errno__; \
+ SYSINFO(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
-#define log_info(__ret__, format, ...) \
- ({ \
- INFO(format, ##__VA_ARGS__); \
- __ret__; \
+#define log_info(__ret__, format, ...) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ INFO(format, ##__VA_ARGS__); \
+ __internal_ret__; \
})
extern int lxc_log_fd;
diff --git a/src/lxc/macro.h b/src/lxc/macro.h
index 612fb11ea4..3df19d6d3a 100644
--- a/src/lxc/macro.h
+++ b/src/lxc/macro.h
@@ -442,24 +442,23 @@ enum {
__internal_fd__; \
})
-#define ret_set_errno(__ret__, __errno__) \
- ({ \
- errno = __errno__; \
- __ret__; \
+#define ret_set_errno(__ret__, __errno__) \
+ ({ \
+ typeof(__ret__) __internal_ret__ = (__ret__); \
+ errno = (__errno__); \
+ __internal_ret__; \
})
-#define ret_errno(__errno__) \
- ({ \
- errno = __errno__; \
- -__errno__; \
+#define ret_errno(__errno__) \
+ ({ \
+ errno = (__errno__); \
+ -(__errno__); \
})
-#define free_replace_move_ptr(a, b) \
- ({ \
- free(a); \
- (a) = (b); \
- (b) = NULL; \
- 0; \
+#define free_move_ptr(a, b) \
+ ({ \
+ free(a); \
+ (a) = move_ptr((b)); \
})
/* Container's specific file/directory names */
More information about the lxc-devel
mailing list