[Devel] Re: [PATCH 1/1] Syslog are now containerized

[Matt Helsley]

On Sat, Feb 13, 2010 at 10:11:58AM -0800, Matt Helsley wrote:
> On Thu, Feb 11, 2010 at 11:48:43AM -0600, Serge E. Hallyn wrote:
> > Quoting Jean-Marc Pigeon (jmp at safe.ca):
> > > 	Added syslog.c such container /proc/kmsg and host /proc/kmsg
> > > 	do not leak in each other.
> > > 	Running rsyslog daemon within a container won't destroy
> > > 	host kernel messages.
> > 
> > Thanks Jean-Marc.  But this really isn't doing most of what I'd
> > recommended in my last emails (both public and private.  In
> > particular:
> > 
> > > index cc4f453..3d0c73e 100644
> > > --- a/include/linux/user_namespace.h
> > > +++ b/include/linux/user_namespace.h
> > > @@ -14,6 +14,7 @@ struct user_namespace {
> > >  	struct hlist_head	uidhash_table[UIDHASH_SZ];
> > >  	struct user_struct	*creator;
> > >  	struct work_struct	destroyer;
> > > +	struct syslog_ns        *syslog;
> > 
> > syslog_ns should be moved into nsproxy and unshared with a
> > separate clone(CLONE_SYSLOG);
> 
> I agree. Keeping it in the user_namespace is strange. It should
> be in the nsproxy along with all of the other namespace pointers.
> 
> > 
> > > -static void emit_log_char(char c)
> > > +static void emit_log_char(struct syslog_ns *syslog_ns, char c)
> > >  {
> > > -	LOG_BUF(log_end) = c;
> > > -	log_end++;
> > > -	if (log_end - log_start > log_buf_len)
> > > -		log_start = log_end - log_buf_len;
> > > -	if (log_end - con_start > log_buf_len)
> > > -		con_start = log_end - log_buf_len;
> > > -	if (logged_chars < log_buf_len)
> > > -		logged_chars++;
> > > +	LOG_BUF(syslog_ns, sys_log_end) = c;
> > 
> > Taking syslog_ns from current in emit_log_char is not right.
> > Emit_log_char() is called from printk which (the comment above
> > printk warns us) can be called from any context.
> > 
> > That was why I suggested:
> > 
> > >! I think my patch is fundamentally wrong anyway:  we should not
> > >! use current's context at vprintk like i did.  We should use an
> > >! optional passed-in context from those callsites where we want to,
> > >! and default to init otherwise.  That means
> > >! 
> > >! 1. put the core of vprintk() into vnsprintk() which takes a syslog
> > >! namespace as argument
> > >! 
> > >! 2. make vprintk() a wrapper around vnsprintk() which passes in
> > >! init_syslog_ns to vnsprintk().  printk can remain unchanged.
> > >! 
> > >! 4. make nsprintk() a wrapper around vnsprintk() which takes a syslog
> > >! ns argument and pass it to vnsprintk()
> > >! 
> > >! 3. do_syslog() can obviously be containerized the same way it
> > >! is now.
> > >! 
> > >! 4. take a printk call like the iptables ones you want and turn
> > >! int into nsprintk syscall.
> 
> Definitely. Actually, I think we can go one step further and say that
> passing syslog_ns directly to nsprintk() is wrong too. It's wrong
> because we only know the namespace that's relevant to the printk
> contents -- and that won't usually be a syslog_ns.
> 
> Better to have the "find corresponding syslog_ns" logic inside
> nsprintk(). The great thing about doing it this way is if we
> ever decide to duplicate printk output to multiple containers it
> can be done inside nsprintk rather than doing a flag-day patch.

It's also easy to see that current's nsproxy won't always point to
the right namespace. Consider kernel threads that do "work" which
was initiated by a userspace task (e.g. a process doing a read on
an NFS file triggers rpc IO kthreads). Usually the relevant namespace
pointer will referenced from the kernel objects related to the work --
not "current" since that's the kernel thread. But those structures
don't reference nsproxies -- they refer to a netns for example. So
being able to obtain a syslog_ns from a netns is critical yet can't
rely on current.

Cheers,
	-Matt Helsley
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers