netdev - Re: [PATCH 2/2] net: Implement SO_PASSCGROUP to enable passing cgroup path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140416185153.GI31074@redhat.com>
Date:	Wed, 16 Apr 2014 14:51:54 -0400
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Simo Sorce <ssorce@...hat.com>, David Miller <davem@...emloft.net>,
	Tejun Heo <tj@...nel.org>, Daniel Walsh <dwalsh@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	lpoetter@...hat.com, cgroups@...r.kernel.org, kay@...hat.com,
	Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/2] net: Implement SO_PASSCGROUP to enable passing
 cgroup path

On Wed, Apr 16, 2014 at 11:40:44AM -0700, Andy Lutomirski wrote:
> On Wed, Apr 16, 2014 at 11:36 AM, Vivek Goyal <vgoyal@...hat.com> wrote:
> > On Wed, Apr 16, 2014 at 10:29:08AM -0700, Andy Lutomirski wrote:
> >
> > [..]
> >> >> Admittedly cgroups aren't currently as important as uid, but if this
> >> >> changes, then SO_PASSCGROUP, as currently written, will have *exactly*
> >> >> the same problem.
> >> >
> >> > Which is easy to foil by using SO_PEERCGROUP and find out who originally
> >> > opened the socket, which is why that is also available!
> >>
> >> Then please remove SO_PASSCGROUP.
> >
> > SO_PASSCGROUP is important because SO_PEERCGROUP does not work with unix
> > datagram sockets.
> 
> Right.  I forgot about that.
> 
> >
> > Again going back to logging example, if some clients are logging to unix
> > datagram sockets, SO_PASSCGROUP is the only option to figure out cgroup
> > of client.
> 
> Hmm.  I think that, in your patch, the cgroup that is sent is the
> cgroup of the caller of write/send/sendmsg.  What if you changed it to
> use the same cgroup that SO_PEERCRED would use?  Would that still
> work?

No. SO_PEERCRED stores the cgroup information once at the time of
connect(). After that it never changes.

What if sender changes the cgroup. That information will not be captured.
Also what if multiple client use the same socket fd to writer to logger?
In that case too storing cgroup info in socket will not help.

Cgroup is sender task's property and not client side socket's property. 

> 
> >>
> >> I still haven't seen any explanation for what's wrong with requiring
> >> senders to ask the kernel to transmit their cgroup.
> >
> > If nothing else, additional complexity and ovhead. Extra pair of messages
> > need to be exchanged to request and then provide the information.
> >
> > How would it work in logging example? Every time logger receives a
> > message, is it supposed to send another message to client to send
> > SCM_CGROUP? That does not sound right.
> 
> No -- just have the logger send the cgroup with every message.  Yes,
> it seems silly, but it's probably barely more expensive than with the
> code in your patch.

So receiver gets the cgroup messages even if it might not want to. There
is no way to say "Hey don't send me SCM_CGROUP's messages".

Now all loggers need to be modifed to always send SCM_CGROUP messages. And
all other more complicated cases might need a different consideration and
clients and servers will need to be modified accordingly.

I think it is much simpler to allow passing of cgroup information and
once we figure out some concrete cases where passing of that info is
not desirabe, implement SO_NOPASSCGROUP and modify those *selected few
corner cases* to set this flag on sockets.

Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html