Re: [PATCH 06/10] Replace FWRITE with a function

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5958 bytes --]

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@gibson.dropbear.id.au> wrote:
> 
> > 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@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.

Right, but there is also a message in write_file() itself.

> > +	}
> >  }
> >  
> >  /**
> > 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.

Yeah, maybe later.

> > +{
> > +	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.

Fair enough, done.

> > +			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 */
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]