[lxc-devel] [PATCH 1/1] preserve_ns: ignore open failures due to ENOENT

[Stéphane Graber]

On Tue, Nov 17, 2015 at 06:48:59PM +0000, Serge Hallyn wrote:
> Quoting Stéphane Graber (stgraber at ubuntu.com):
> > On Tue, Nov 17, 2015 at 06:30:59PM +0000, Serge Hallyn wrote:
> > > If some /proc/pid/ns/ns file we want is not supported by the kernel,
> > > then just don't do the preserve_ns: don't fail the whole container
> > > start.
> > 
> > Should we differentiate between the user actually requesting some
> > namespaces to be preserved, and fail in that case vs us preserving all
> > namespaces automatically (in which case, we should just skip silently)?
> 
> If /proc/pid/ns doesn't exist and the user requests preservation, we currently
> skip silently.

Which I'd argue is a bug. If the user does specifically request to
preserve namespaces, it should lead to failure if it's not possible.

> 
> > > Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> > > ---
> > >  src/lxc/start.c | 23 +++++++++++++----------
> > >  1 file changed, 13 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/src/lxc/start.c b/src/lxc/start.c
> > > index a294d18..6ed4814 100644
> > > --- a/src/lxc/start.c
> > > +++ b/src/lxc/start.c
> > > @@ -125,7 +125,7 @@ static void close_ns(int ns_fd[LXC_NS_MAX]) {
> > >  }
> > >  
> > >  static int preserve_ns(int ns_fd[LXC_NS_MAX], int clone_flags, pid_t pid) {
> > > -	int i, saved_errno;
> > > +	int i;
> > >  	char path[MAXPATHLEN];
> > >  
> > >  	for (i = 0; i < LXC_NS_MAX; i++)
> > > @@ -143,18 +143,21 @@ static int preserve_ns(int ns_fd[LXC_NS_MAX], int clone_flags, pid_t pid) {
> > >  		snprintf(path, MAXPATHLEN, "/proc/%d/ns/%s", pid,
> > >  		         ns_info[i].proc_name);
> > >  		ns_fd[i] = open(path, O_RDONLY | O_CLOEXEC);
> > > -		if (ns_fd[i] < 0)
> > > -			goto error;
> > > +		if (ns_fd[i] < 0) {
> > > +			int ret;
> > > +			if (errno == ENOENT) {
> > > +				SYSERROR("failed to open '%s'; continuing", path);
> > 
> > Doesn't that mean that on pre-3.8 you'll be getting a scary looking
> > error per namespace?
> 
> ... you already get the
> 	
> 	WARN("Kernel does not support attach; preserve_ns ignored");
> 
> and it does say "continuing".

No, on a 3.5 kernel you don't. The message is currently only shown if
/proc/self/ns doesn't exist, if it does exist and is missing entries, no
message is currently displayed.

Also note that as far as users are concerned, they're only seeing the
pre-1.1.5 behavior right now which is to only call preserve_ns when the
user actually requests some ns be preserved.

As we're now calling that code for every single container, we should
make sure that in the common case, no extra user visible messages appear
(INFO/DEBUG/TRACE is fine, those only hit the log).


So anyway, ideally I'd like to see:
 - User requests some namespaces be preserved:
    - If /proc/self/ns is missing => fail (saying kernel misses setns)
    - If /proc/self/ns/<namespace> entry is missing => fail (saying kernel misses setns for <namespace>)
 - User doesn't request some namespaces be preserved:
    - If /proc/self/ns is missing => log an INFO message (kernel misses setns) and continue
    - If /proc/self/ns/<namespace> entry is missing => log an INFO message (kernel misses setns for <namespace>) and continue

That won't cause any regression for users of old kernels, the only
change in behavior is that if you request some namespaces be preserved
on pre-3.8, LXC will now fail rather than pretend it succeeded in
attaching you to those namespaces.

> 
> > > +				ret = 0;
> > > +			} else {
> > > +				SYSERROR("failed to open '%s'; aborting", path);
> > > +				ret = -1;
> > > +			}
> > > +			close_ns(ns_fd);
> > > +			return ret;
> > > +		}
> > >  	}
> > >  
> > >  	return 0;
> > > -
> > > -error:
> > > -	saved_errno = errno;
> > > -	close_ns(ns_fd);
> > > -	errno = saved_errno;
> > > -	SYSERROR("failed to open '%s'", path);
> > > -	return -1;
> > >  }
> > >  
> > >  static int attach_ns(const int ns_fd[LXC_NS_MAX]) {
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > lxc-devel mailing list
> > > lxc-devel at lists.linuxcontainers.org
> > > http://lists.linuxcontainers.org/listinfo/lxc-devel
> > 
> > -- 
> > Stéphane Graber
> > Ubuntu developer
> > http://www.ubuntu.com
> 
> 
> 
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> 
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel

-- 
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20151117/060179a4/attachment.sig>