On Thu, Oct 13, 2022 at 04:17:09AM +0200, Stefano Brivio wrote:On Tue, 11 Oct 2022 16:40:14 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Right, but there is also a message in write_file() itself.In a few places we use the FWRITE() macro to open a file, replace it's contents with a given string and close it again. There's no real reason this needs to be a macro rather than just a function though.Well, there's a bit of a reason: it gives more descriptive messages in isolate_user() with the same LoCs. On the other hand I would also prefer a function here, probably better than the alternative I'm not sure why this series needs this by the way.Turn it into a function 'write_file()' and make some ancillary cleanups while we're there: - Add a return code so the caller can handle giving a useful error message - Handle the case of short write()s (unlikely, but possible) - Add O_TRUNC, to make sure we replace the existing contents entirely Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- isolation.c | 17 +++++++++-------- pasta.c | 4 ++-- util.c | 33 +++++++++++++++++++++++++++++++++ util.h | 13 +------------ 4 files changed, 45 insertions(+), 22 deletions(-) diff --git a/isolation.c b/isolation.c index 10cef05..4aa75e6 100644 --- a/isolation.c +++ b/isolation.c @@ -129,7 +129,8 @@ void isolate_initial(void) */ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns) { - char nsmap[BUFSIZ]; + char uidmap[BUFSIZ]; + char gidmap[BUFSIZ]; /* First set our UID & GID in the original namespace */ if (setgroups(0, NULL)) { @@ -184,14 +185,14 @@ void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns) } /* Configure user and group mappings */ - snprintf(nsmap, BUFSIZ, "0 %u 1", uid); - FWRITE("/proc/self/uid_map", nsmap, "Cannot set uid_map in namespace"); + snprintf(uidmap, BUFSIZ, "0 %u 1", uid); + snprintf(gidmap, BUFSIZ, "0 %u 1", gid); - FWRITE("/proc/self/setgroups", "deny", - "Cannot write to setgroups in namespace"); - - snprintf(nsmap, BUFSIZ, "0 %u 1", gid); - FWRITE("/proc/self/gid_map", nsmap, "Cannot set gid_map in namespace"); + if (write_file("/proc/self/uid_map", uidmap) || + write_file("/proc/self/setgroups", "deny") || + write_file("/proc/self/gid_map", gidmap)) { + warn("Couldn't configure user namespace");It's unlikely that we can write to uid_map but not to setgroups. Still, having separate messages, as we had them, could help investigating some issues.Yeah, maybe later.+ } } /** diff --git a/pasta.c b/pasta.c index 4ff322c..0ab2fe4 100644 --- a/pasta.c +++ b/pasta.c @@ -167,8 +167,8 @@ static int pasta_setup_ns(void *arg) const struct pasta_setup_ns_arg *a = (const struct pasta_setup_ns_arg *)arg; - FWRITE("/proc/sys/net/ipv4/ping_group_range", "0 0", - "Cannot set ping_group_range, ICMP requests might fail"); + if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0")) + warn("Cannot set ping_group_range, ICMP requests might fail"); execvp(a->exe, a->argv); diff --git a/util.c b/util.c index 6b86ead..93b93b1 100644 --- a/util.c +++ b/util.c @@ -547,3 +547,36 @@ int fls(unsigned long x) return y; } + +/** + * write_file() - Replace contents of file with a string + * @path: File to write + * @buf: String to write + * + * Return: 0 on success, -1 on any error + */ +int write_file(const char *path, const char *buf)We could also use this for the PID file by optionally taking a file number, but I haven't tried how it looks like.Fair enough, done.+{ + int fd = open(path, O_WRONLY | O_TRUNC | O_CLOEXEC); + size_t len = strlen(buf); + + if (fd < 0) { + warn("Could not open %s: %s", path, strerror(errno)); + return -1; + } + + while (len) { + ssize_t rc = write(fd, buf, len); + + if (rc < 0) {I would change this to <= 0. Not that it matters with write(), but should we ever change that another function, we run the (absolutely not critical) risk of forgetting to adjust this and get stuck in a loop here.-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson+ warn("Couldn't write to %s: %s", path, strerror(errno)); + break; + } + + buf += rc; + len -= rc; + } + + close(fd); + return len == 0 ? 0 : -1; +} diff --git a/util.h b/util.h index 0c06e34..f957522 100644 --- a/util.h +++ b/util.h @@ -58,18 +58,6 @@ void trace_init(int enable); #define TMPDIR "/tmp" #endif -#define FWRITE(path, buf, str) \ - do { \ - int flags = O_WRONLY | O_CLOEXEC; \ - int fd = open(path, flags); \ - \ - if (fd < 0 || \ - write(fd, buf, strlen(buf)) != (int)strlen(buf)) \ - warn(str); \ - if (fd >= 0) \ - close(fd); \ - } while (0) - #define V4 0 #define V6 1 #define IP_VERSIONS 2 @@ -215,5 +203,6 @@ int ns_enter(const struct ctx *c); void write_pidfile(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); +int write_file(const char *path, const char *buf); #endif /* UTIL_H */