Bonjour Matt,
Thanks for comments. your time is really appreciated.
On Tue, 2010-02-16 at 15:59 -0800, Matt Helsley wrote:
> I think this last point deserves significant elaboration. What
> does it mean to "not leak or compromise hosts syslog data"? Is it possible
> to guarantee that we don't leak or is this just a best-effort sort of thing?
> My hunch is it's the latter. You really must respond to Eric's question of
> why this is useful. I know it might seem obvious to you but it's not
> actualy clear. I've found that carefuly explaining my reasoning when it
> seemed obvious often uncovers hidden assumptions I made and may even
> suggest problems and/or alternate approaches.
You are right, I'll try to give a better rational when posting
patches next time.
>
> Kernel patch guidelines suggests you should split the patch up so it's
> one change at a time. One possible way to divide this into a series of
> patches is:
>
> patch to add krefs
> patch to move syslog_ns from user_namespace to nsproxy
> patch to rename syslog_current_ns() (or whatever)
> patch to rename X to Y
>
> (There may be better ways to break up the patches -- these just came to mind)
In this case I disagree.
First patch posted is patch to previous (working set) to try
to follow Serge suggestion, I should have send both part instead
only the last one (not very confortable yet with git).
Patch proposed is a complet set needed to reach a working stage,
there is nothing achieved if cut patch in slice, nobody
can say it is (not)working without the full patch set...
>
> Check with Suka on this one -- I seem to recall specific unused CLONE_ flags
> are, unfortunately, reserved. Otherwise yes, this is the right place and what
> Serge suggested.
This very CLONE_SYSLOG 0x0001000 flag was suggested by Serge
too.
>
[...]
>
> You shouldn't be able to unshare syslog -- I think it's a security concern.
> I think just keeping CLONE_SYSLOG out of this set is sufficient for sys_unshare.
> There's a corresponding check in clone/clone2/eclone which would be similar to
> how CLONE_NEWPID is privileged.
We must be able to unshare syslog, this the whole point of the
exercice. (to make sure about understanding, unshare means
"I, process xyz, want to replace current syslog buffer with
my own".
My personal feeling it shouldn't be a CLONE_SYSLOG, but
rather going with a more generic as CLONE_NEWPID.
[...]
> > + ns->syslog_ns=release_syslog_ns(ns->syslog_ns);
>
> style: space around the assignment operator.
> See the kernel Documentation on contributing patches and the style of
> kernel code -- those are our standards too. scripts/checkpatch.pl will
> also find many of these things for you.
Tried to keep local standard, seems my personal way to write
code find their way....
Thanks for the pointer scripts/checkpatch.pl
[..[ CLONE_NEWNET | CLONE_SYSLOG )))
>
> You shouldn't be able to unshare syslog. It should require using a privileged
> clone() call like CLONE_NEWPID. So CLONE_SYSLOG doesn't belong here I think.
If you mean we shouldn't be able to unshare syslog in specific,
but link unsharing to another CLONE flag I agree. NEWPID seems
to be an excellent candidate....
But question... what is the best way to reach consensus on this
project??? seems everybody is not in pace with the other.
>
> In kernel code, especially namespaces, "get" implies taking an extra reference.
> So get_current_syslog_ns() needs a different name. I'd name it like the pid
> namespace: anything without _ns on the end uses "current".
OK, point taken about "get", I'll check about pid_namespace.
>
> obj-$(CONFIG_PRINTK) += syslog.o
>
> Then I think you can avoid this #ifdef entirely.
Interesting, I was not satisfied with my #ifdef,
that should clean the code, Thanks.
>
> > /*
> > * Static memory definition, used to assign a syslog
> > * to the kernel itself
> > *
> > */
> > -
> > -#ifdef CONFIG_PRINTK
>
> I suppose. Again, not a substantial change. To me the comment
> is unnecessary. This init_foo_ns pattern is quite recognizable to
> anyone familiar with how things are initialized in the kernel.
Well, just notice, for a guy looking at the kernel,
I won't call it "cristal clear" (I know it is
now an old code), a little bit of help is not that bad.
I understand it is not good to write 100 lines of
explanation for 10 lines of code, but data pointer
are not that bad.
syslog need to be unshared is crystal clear
for VZ real user, but you need some data pointer
to figure it out (no offense :-}}}).
> > -struct syslog_ns * syslog_malloc(unsigned container_buf_len)
> > +static struct syslog_ns * malloc_syslog_ns(unsigned container_buf_len)
>
> For namespaces we've been using "create" rather than foo_[m]alloc.
> create_pid_namespace, create_user_ns, create_uts_ns, etc.
My understanding of "create" it doesn't assign memory
space each time....syslog_malloc does and I was keeping
myself here to the malloc standard library routine.
(and malloc_syslog_ns is static, which should give me
a little bit of freedom).
{..]
> > +struct syslog_ns * realloc_syslog_ns(struct syslog_ns *syslog_ns, unsigned container_buf_len)
>
> Does this change the syslog_ns pointer, or just the buffer size? If the latter
> then resize might be a better word.
good idea.
--
A bientôt
==========================================================================
Jean-Marc Pigeon Internet: jmp@xxxxxxx
SAFE Inc. Phone: (514) 493-4280
Fax: (514) 493-1946
Clement, 'a kiss solution' to get rid of SPAM (at last)
Clement' Home base <"http://www.clement.safe.ca";>
==========================================================================
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
