Re: [PATCH 7/9] sock, cgroup: add sock->sk_cgroup

[Daniel Wagner]

Hi Tejun,

On 11/21/2015 05:13 PM, Tejun Heo wrote:
> Signed-off-by: Tejun Heo 
> Cc: Daniel Borkmann 
> Cc: Daniel Wagner 

I did a quick test and for new connection the cgroup2 match worked as
expected. For an existing connection I wasn't able to trigger the match.

It is quite likely I do something wrong:

ssh into the box
# mkdir /sys/fs/cgroup/test
# echo $$ > /sys/fs/cgroup/test/cgroup.procs
# echo $PPID > /sys/fs/cgroup/test/cgroup.procs
# iptables -A OUTPUT -m cgroup --path test

Should I see matches with the existing ssh session?

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

Re: [PATCH 7/9] sock, cgroup: add sock->sk_cgroup

[Daniel Wagner]

On 11/23/2015 04:48 PM, Tejun Heo wrote:
> On Mon, Nov 23, 2015 at 02:02:03PM +0100, Daniel Wagner wrote:
>> On 11/21/2015 05:13 PM, Tejun Heo wrote:
>>> Signed-off-by: Tejun Heo 
>>> Cc: Daniel Borkmann 
>>> Cc: Daniel Wagner 
>>
>> I did a quick test and for new connection the cgroup2 match worked as
>> expected. For an existing connection I wasn't able to trigger the match.
>>
>> It is quite likely I do something wrong:
>>
>>  ssh into the box
>>  # mkdir /sys/fs/cgroup/test
>>  # echo $$ > /sys/fs/cgroup/test/cgroup.procs
>>  # echo $PPID > /sys/fs/cgroup/test/cgroup.procs
>>  # iptables -A OUTPUT -m cgroup --path test
>>
>> Should I see matches with the existing ssh session?
> 
> Socket is associated with the creating cgroup and stays associated
> with that cgroup until it's released.  Migrating the process doesn't
> change the ownership of the sockets it has created.  This is in line
> with how other stateful resources such as memory are handled in
> cgroup2 hierarchy.

Thanks for the explanation. Looks good to me:

Tested-by: Daniel Wagner 
Acked-by: Daniel Wagner 

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

Re: [PATCH 7/9] sock, cgroup: add sock->sk_cgroup

[Tejun Heo]

Hello,

On Mon, Nov 23, 2015 at 02:02:03PM +0100, Daniel Wagner wrote:
> On 11/21/2015 05:13 PM, Tejun Heo wrote:
> > Signed-off-by: Tejun Heo 
> > Cc: Daniel Borkmann 
> > Cc: Daniel Wagner 
> 
> I did a quick test and for new connection the cgroup2 match worked as
> expected. For an existing connection I wasn't able to trigger the match.
> 
> It is quite likely I do something wrong:
> 
>   ssh into the box
>   # mkdir /sys/fs/cgroup/test
>   # echo $$ > /sys/fs/cgroup/test/cgroup.procs
>   # echo $PPID > /sys/fs/cgroup/test/cgroup.procs
>   # iptables -A OUTPUT -m cgroup --path test
> 
> Should I see matches with the existing ssh session?

Socket is associated with the creating cgroup and stays associated
with that cgroup until it's released.  Migrating the process doesn't
change the ownership of the sockets it has created.  This is in line
with how other stateful resources such as memory are handled in
cgroup2 hierarchy.

Thanks.

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

[PATCH 7/9] sock, cgroup: add sock->sk_cgroup

[Tejun Heo]

In cgroup v1, dealing with cgroup membership was difficult because the
number of membership associations was unbound.  As a result, cgroup v1
grew several controllers whose primary purpose is either tagging
membership or pull in configuration knobs from other subsystems so
that cgroup membership test can be avoided.

net_cls and net_prio controllers are examples of the latter.  They
allow configuring network-specific attributes from cgroup side so that
network subsystem can avoid testing cgroup membership; unfortunately,
these are not only cumbersome but also problematic.

Both net_cls and net_prio aren't properly hierarchical.  Both inherit
configuration from the parent on creation but there's no interaction
afterwards.  An ancestor doesn't restrict the behavior in its subtree
in anyway and configuration changes aren't propagated downwards.
Especially when combined with cgroup delegation, this is problematic
because delegatees can mess up whatever network configuration
implemented at the system level.  net_prio would allow the delegatees
to set whatever priority value regardless of CAP_NET_ADMIN and net_cls
the same for classid.

While it is possible to solve these issues from controller side by
implementing hierarchical allowable ranges in both controllers, it
would involve quite a bit of complexity in the controllers and further
obfuscate network configuration as it becomes even more difficult to
tell what's actually being configured looking from the network side.
While not much can be done for v1 at this point, as membership
handling is sane on cgroup v2, it'd be better to make cgroup matching
behave like other network matches and classifiers than introducing
further complications.

In preparation, this patch updates sock->sk_cgrp_data handling so that
it points to the v2 cgroup that sock was created in until either
net_prio or net_cls is used.  Once either of the two is used,
sock->sk_cgrp_data reverts to its previous role of carrying prioidx
and classid.  This is to avoid adding yet another cgroup related field
to struct sock.

As the mode switching can happen at most once per boot, the switching
mechanism is aimed at lowering hot path overhead.  It may leak a
finite, likely small, number of cgroup refs and report spurious
prioidx or classid on switching; however, dynamic updates of prioidx
and classid have always been racy and lossy - socks between creation
and fd installation are never updated, config changes don't update
existing sockets at all, and prioidx may index with dead and recycled
cgroup IDs.  Non-critical inaccuracies from small race windows won't
make any noticeable difference.

This patch doesn't make use of the pointer yet.  The following patch
will implement netfilter match for cgroup2 membership.

v2: Use sock_cgroup_data to avoid inflating struct sock w/ another
cgroup specific field.

v3: Add comments explaining why sock_data_prioidx() and
sock_data_classid() use different fallback values.

Signed-off-by: Tejun Heo 
Cc: Daniel Borkmann 
Cc: Daniel Wagner 
CC: Neil Horman 
---
 include/linux/cgroup-defs.h  | 88 +---
 include/linux/cgroup.h   | 41 +
 kernel/cgroup.c  | 55 ++-
 net/core/netclassid_cgroup.c |  7 +++-
 net/core/netprio_cgroup.c|  7 +++-
 net/core/sock.c  |  2 +
 6 files changed, 191 insertions(+), 9 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index ed128fed..9dc2263 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -544,31 +544,107 @@ static inline void cgroup_threadgroup_change_end(struct 
task_struct *tsk) {}
 
 #ifdef CONFIG_SOCK_CGROUP_DATA
 
+/*
+ * sock_cgroup_data is embedded at sock->sk_cgrp_data and contains
+ * per-socket cgroup information except for memcg association.
+ *
+ * On legacy hierarchies, net_prio and net_cls controllers directly set
+ * attributes on each sock which can then be tested by the network layer.
+ * On the default hierarchy, each sock is associated with the cgroup it was
+ * created in and the networking layer can match the cgroup directly.
+ *
+ * To avoid carrying all three cgroup related fields separately in sock,
+ * sock_cgroup_data overloads (prioidx, classid) and the cgroup pointer.
+ * On boot, sock_cgroup_data records the cgroup that the sock was created
+ * in so that cgroup2 matches can be made; however, once either net_prio or
+ * net_cls starts being used, the area is overriden to carry prioidx and/or
+ * classid.  The two modes are distinguished by whether the lowest bit is
+ * set.  Clear bit indicates cgroup pointer while set bit prioidx and
+ * classid.
+ *
+ * While userland may start using net_prio or net_cls at any time, once
+ * either is used, cgroup2 matching no longer works.  There is no reason to
+ * mix the two and this is in line