Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560585 > unrolled thread

Re: Potential issues (security and otherwise) with the current cgroup-bpf API

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Potential issues (security and otherwise) with the current  cgroup-bpf API Michal Hocko <mhocko@kernel.org> - 2017-01-17 14:30 +0100
    Re: Potential issues (security and otherwise) with the current  cgroup-bpf API Peter Zijlstra <peterz@infradead.org> - 2017-01-17 14:40 +0100
      Re: Potential issues (security and otherwise) with the current  cgroup-bpf API Michal Hocko <mhocko@kernel.org> - 2017-01-17 15:20 +0100
        Re: Potential issues (security and otherwise) with the current  cgroup-bpf API Andy Lutomirski <luto@amacapital.net> - 2017-01-17 21:40 +0100
        Re: Potential issues (security and otherwise) with the current  cgroup-bpf API Tejun Heo <tj@kernel.org> - 2017-01-18 23:30 +0100
          Re: Potential issues (security and otherwise) with the current  cgroup-bpf API Michal Hocko <mhocko@kernel.org> - 2017-01-19 10:10 +0100

#1560585 — Re: Potential issues (security and otherwise) with the current cgroup-bpf API

On Sun 15-01-17 20:19:01, Tejun Heo wrote:
[...]
> So, what's proposed is a proper part of bpf.  In terms of
> implementation, cgroup helps by hosting the pointers but that doesn't
> necessarily affect the conceptual structure of it.  Given that, I
> don't think it'd be a good idea to add anything to cgroup interface
> for this feature.  Introspection is great to have but this should be
> introspectable together with other bpf programs using the same
> mechanism.  That's where it belongs.

If BPF only piggy backs on top of cgroup to iterate tasks shouldn't we
at least enforce that the cgroup has to be a leaf one and no further
children groups can be created once there is BPF program attached?
This should break the existing usecases AFAIU and it would allow future
changes without major API surprises.

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1560587

On Tue, Jan 17, 2017 at 02:03:03PM +0100, Michal Hocko wrote:
> On Sun 15-01-17 20:19:01, Tejun Heo wrote:
> [...]
> > So, what's proposed is a proper part of bpf.  In terms of
> > implementation, cgroup helps by hosting the pointers but that doesn't
> > necessarily affect the conceptual structure of it.  Given that, I
> > don't think it'd be a good idea to add anything to cgroup interface
> > for this feature.  Introspection is great to have but this should be
> > introspectable together with other bpf programs using the same
> > mechanism.  That's where it belongs.
> 
> If BPF only piggy backs on top of cgroup to iterate tasks shouldn't we
> at least enforce that the cgroup has to be a leaf one and no further
> children groups can be created once there is BPF program attached?

Why (again) this stupid constraint?

If you want to use cgroups for tagging (like perf does), _any_ parent
cgroup will also tag you.

So creating child cgroups, and placing tasks in it, should not be a
problem, the BPF thing should apply to all of them.

[toc] | [prev] | [next] | [standalone]

#1560623

On Tue 17-01-17 14:32:04, Peter Zijlstra wrote:
> On Tue, Jan 17, 2017 at 02:03:03PM +0100, Michal Hocko wrote:
> > On Sun 15-01-17 20:19:01, Tejun Heo wrote:
> > [...]
> > > So, what's proposed is a proper part of bpf.  In terms of
> > > implementation, cgroup helps by hosting the pointers but that doesn't
> > > necessarily affect the conceptual structure of it.  Given that, I
> > > don't think it'd be a good idea to add anything to cgroup interface
> > > for this feature.  Introspection is great to have but this should be
> > > introspectable together with other bpf programs using the same
> > > mechanism.  That's where it belongs.
> > 
> > If BPF only piggy backs on top of cgroup to iterate tasks shouldn't we
> > at least enforce that the cgroup has to be a leaf one and no further
> > children groups can be created once there is BPF program attached?
> 
> Why (again) this stupid constraint?
> 
> If you want to use cgroups for tagging (like perf does), _any_ parent
> cgroup will also tag you.
> 
> So creating child cgroups, and placing tasks in it, should not be a
> problem, the BPF thing should apply to all of them.

This would require using hierarchical cgroup iterators to iterate over
tasks. As per Andy's testing this doesn't seem to be the case. I haven't
checked the implementation closely but my understanding was that using
only cgroup specific tasks was intentional.

I do agree that using hierarchy aware cgroup iterators is the right
approach here and we wouldn't see any issue. But I am still not sure
I've wrapped my head around this feature completely.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1560998

On Tue, Jan 17, 2017 at 5:58 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Tue 17-01-17 14:32:04, Peter Zijlstra wrote:
>> On Tue, Jan 17, 2017 at 02:03:03PM +0100, Michal Hocko wrote:
>> > On Sun 15-01-17 20:19:01, Tejun Heo wrote:
>> > [...]
>> > > So, what's proposed is a proper part of bpf.  In terms of
>> > > implementation, cgroup helps by hosting the pointers but that doesn't
>> > > necessarily affect the conceptual structure of it.  Given that, I
>> > > don't think it'd be a good idea to add anything to cgroup interface
>> > > for this feature.  Introspection is great to have but this should be
>> > > introspectable together with other bpf programs using the same
>> > > mechanism.  That's where it belongs.
>> >
>> > If BPF only piggy backs on top of cgroup to iterate tasks shouldn't we
>> > at least enforce that the cgroup has to be a leaf one and no further
>> > children groups can be created once there is BPF program attached?
>>
>> Why (again) this stupid constraint?
>>
>> If you want to use cgroups for tagging (like perf does), _any_ parent
>> cgroup will also tag you.
>>
>> So creating child cgroups, and placing tasks in it, should not be a
>> problem, the BPF thing should apply to all of them.
>
> This would require using hierarchical cgroup iterators to iterate over
> tasks. As per Andy's testing this doesn't seem to be the case. I haven't
> checked the implementation closely but my understanding was that using
> only cgroup specific tasks was intentional.

The current semantics are AFAIK that only the innermost cgroup that
has a hook installed is in effect.  I think this is the wrong design.

I think that the right semantics are probably to support both
innermost-to-outermost and outermost-to-innermost and to select which
is appropriate for each hook.  Suppose we have a cgroup /a/b where a
and b both have hooks installed.  If the hook is a socket creation or
egress hook, I think that b's hook should run first.  If b's hook
rejects, then a's hook is not run.  If b's hook accepts, then a's hook
is run.  This way a gets the last word on any changes to the socket
settings and a sees exactly what would happen if it were to accept.

Conversely, for ingress hooks, I think that a's hook should run first.
This way a sees the packet as it originally came in and can modify or
reject it, and then b only sees whatever a chooses to let through.

The guiding principle here is that, for actions that originate outside
the machine, the outer hooks should IMO run first and, for actions
that originate from a task in a cgroup, the innermost hooks should run
first.

--Andy

[toc] | [prev] | [next] | [standalone]

#1562236

Hello, Michal.

On Tue, Jan 17, 2017 at 02:58:30PM +0100, Michal Hocko wrote:
> This would require using hierarchical cgroup iterators to iterate over

It does behave hierarchically.

> tasks. As per Andy's testing this doesn't seem to be the case. I haven't

That's not what Andy's testing showed.  What that showed was that
program in a child can override the one from its ancestor.

> checked the implementation closely but my understanding was that using
> only cgroup specific tasks was intentional.

Maybe I'm misreading you but if you're saying that a bpf program would
only execute for tasks on that specific cgroup, that is not true.

> I do agree that using hierarchy aware cgroup iterators is the right
> approach here and we wouldn't see any issue. But I am still not sure
> I've wrapped my head around this feature completely.

It's really simple.  You can install bpf programs with a cgroup path
associated with them.  When that particular type of bpf program needs
to be executed, the bpf program which is attached to the closest
ancestor (including self) is executed.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1562484

On Wed 18-01-17 14:18:50, Tejun Heo wrote:
> Hello, Michal.
> 
> On Tue, Jan 17, 2017 at 02:58:30PM +0100, Michal Hocko wrote:
> > This would require using hierarchical cgroup iterators to iterate over
> 
> It does behave hierarchically.
> 
> > tasks. As per Andy's testing this doesn't seem to be the case. I haven't
> 
> That's not what Andy's testing showed.  What that showed was that
> program in a child can override the one from its ancestor.

My fault, I've misread Andy's test case. I thought that the child group
simply disabled the bpf program and the one from the parent hasn't
executed.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web