From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id CAAA95A026B for ; Thu, 13 Oct 2022 11:37:28 +0200 (CEST) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Mp4Dg1H4Kz4xH0; Thu, 13 Oct 2022 20:37:23 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1665653843; bh=xT3WVwcPG9FNoZ6oPthvpmsNa8iuhKIb1X9Z/8RqEYw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CW/2PeTJoyA4NwVa7GFoDnyrfZeqrqFnyCwNjkzTsQsEaUO0f50OfZ0NriA9SbNIZ 6cXYCKZb5LDlbPvIbLsOTWobyC/StgZ97qD38XylZ8b4v70qNkBKSf6FrisLpmZqd4 hpK/Esoji0RH894dovAQolP8EwCk79nC5dZMQSEQ= Date: Thu, 13 Oct 2022 19:51:04 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 06/10] Replace FWRITE with a function Message-ID: References: <20221011054018.1449506-1-david@gibson.dropbear.id.au> <20221011054018.1449506-7-david@gibson.dropbear.id.au> <20221013041709.316ec052@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9GXbMT1QK6P69xY0" Content-Disposition: inline In-Reply-To: <20221013041709.316ec052@elisabeth> Message-ID-Hash: AHZECGVXBTVGE6MLP6IH5G7SEIZGKFSC X-Message-ID-Hash: AHZECGVXBTVGE6MLP6IH5G7SEIZGKFSC X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.3 Precedence: list List-Id: Development discussion and patches for passt Archived-At: <> Archived-At: List-Archive: <> List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --9GXbMT1QK6P69xY0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 13, 2022 at 04:17:09AM +0200, Stefano Brivio wrote: > On Tue, 11 Oct 2022 16:40:14 +1100 > David Gibson wrote: >=20 > > 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. >=20 > Well, there's a bit of a reason: it gives more descriptive messages in > isolate_user() with the same LoCs. >=20 > 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. >=20 > > 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 me= ssage > > - Handle the case of short write()s (unlikely, but possible) > > - Add O_TRUNC, to make sure we replace the existing contents entirely > >=20 > > Signed-off-by: David Gibson > > --- > > isolation.c | 17 +++++++++-------- > > pasta.c | 4 ++-- > > util.c | 33 +++++++++++++++++++++++++++++++++ > > util.h | 13 +------------ > > 4 files changed, 45 insertions(+), 22 deletions(-) > >=20 > > 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 *u= serns) > > { > > - char nsmap[BUFSIZ]; > > + char uidmap[BUFSIZ]; > > + char gidmap[BUFSIZ]; > > =20 > > /* 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) > > } > > =20 > > /* 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); > > =20 > > - 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"); >=20 > 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. Right, but there is also a message in write_file() itself. > > + } > > } > > =20 > > /** > > 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 > > =3D (const struct pasta_setup_ns_arg *)arg; > > =20 > > - 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"); > > =20 > > execvp(a->exe, a->argv); > > =20 > > 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) > > =20 > > 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) >=20 > We could also use this for the PID file by optionally taking a file numbe= r, but > I haven't tried how it looks like. Yeah, maybe later. > > +{ > > + int fd =3D open(path, O_WRONLY | O_TRUNC | O_CLOEXEC); > > + size_t len =3D strlen(buf); > > + > > + if (fd < 0) { > > + warn("Could not open %s: %s", path, strerror(errno)); > > + return -1; > > + } > > + > > + while (len) { > > + ssize_t rc =3D write(fd, buf, len); > > + > > + if (rc < 0) { >=20 > I would change this to <=3D 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. Fair enough, done. > > + warn("Couldn't write to %s: %s", path, strerror(errno)); > > + break; > > + } > > + > > + buf +=3D rc; > > + len -=3D rc; > > + } > > + > > + close(fd); > > + return len =3D=3D 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 > > =20 > > -#define FWRITE(path, buf, str) \ > > - do { \ > > - int flags =3D O_WRONLY | O_CLOEXEC; \ > > - int fd =3D open(path, flags); \ > > - \ > > - if (fd < 0 || \ > > - write(fd, buf, strlen(buf)) !=3D (int)strlen(buf)) \ > > - warn(str); \ > > - if (fd >=3D 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); > > =20 > > #endif /* UTIL_H */ >=20 --=20 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 --9GXbMT1QK6P69xY0 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmNH0WIACgkQgypY4gEw YSKENBAAsshTi/Q4yu0QKnczawShKC9xcTLA9Cg8IC5+p4SoH+OLnyQhKwIWQN3w AnVNV9fsgS53Kb3wfnwwS6Y+ZTf8HSNFWJlUHWfgGEoQlnEkCpLTPOhxRLdFQDsd KK0Xj5OkxW2s+o6nUdwZm42ZGRxTQXGpLxSaSOfLX0h3uZeryUF2EZweQtKe32KZ LbE+dXhOVcCl966YeoXixPOP/F/2mXhl9IGZ69Ug+kzioBnIXuVlqb8Q6DYDoCOs LfGue77W0UV7AESJbM2XRkiBPBlRnkWzYcInmA9eEce1W75tlxJvbJF17QpdVAym ZWFfvVvOGaDehpZExxt6AsfQV1Lgt5TsBPzNDczKBrecB3l6P/s0uugYJR8YstZc nnKS8xKWzj0rZlkm/AXvLR5FPPk/KddEQREidQvUOPpEHyZoLKER22PHvL6b0nry j4o5FXBwCDrbp3aFryPFcftWQxrHeA09K0TvqxEeMgoqx9OmITKO9azV9zmQFFyY 38h3x64hzYKLhpJhNkFaJPF2gnMJaPnAjY0vIWz0+5JFjELPEE/JdDHXUEoyjTVo tWipJMzH3/zTvsGCRx058BStStjvuaauB5xMmlx1nNpzvQFx+zi1iHQA1lj7oVrl CgvGOe33APdc2PxQjHEj98rWpGDCnWIBE/hGfYg9/q/jVDlQoaE= =5uxI -----END PGP SIGNATURE----- --9GXbMT1QK6P69xY0--