Override default socket policy per cgroup

Override default socket policy per cgroup

[sdf]

Let's say I want to set some default sk_priority for all sockets in a
specific cgroup. I can do it right now using cgroup/sock_create, but it
applies only to AF_INET{,6} sockets. I'd like to do the same for raw
(AF_PACKET) sockets and cgroup/sock_create doesn't trigger for them :-(

(1) My naive approach would be to add another cgroup/sock_post_create
which runs late from __sock_create and triggers on everything.

(2) Another approach might be to move BPF_CGROUP_RUN_PROG_INET_SOCK and
make it work with AF_PACKET. This might be not 100% backwards compatible
but I'd assume that most users should look at the socket family before
doing anything. (in this case it feels like we can extend
sock_bind/release for af_packets as well, just for accounting purposes,
without any way to override the target ifindex).

(3) I've also tried to play with fentry/security_socket_post_create, but
it doesn't look like I can change kernel data from the tracing context.
fentry is also global and I'd like to get to cgroup-local-storage.

(I don't want to get involved in per-packet processing here, so I'm
looking at something I can do once at socket creation).

Any suggestions? Anything I'm missing? I'm leaning towards (2), maybe we
can extend existing socket create/bind/release for af_packet? Having
another set (1) doesn't make sense.

Re: Override default socket policy per cgroup

[Martin KaFai Lau]

On Wed, Feb 09, 2022 at 09:03:45AM -0800, sdf@google.com wrote:
> Let's say I want to set some default sk_priority for all sockets in a
> specific cgroup. I can do it right now using cgroup/sock_create, but it
> applies only to AF_INET{,6} sockets. I'd like to do the same for raw
> (AF_PACKET) sockets and cgroup/sock_create doesn't trigger for them :-(
Other than AF_PACKET and INET[6], do you have use cases for other families?

> (1) My naive approach would be to add another cgroup/sock_post_create
> which runs late from __sock_create and triggers on everything.
> 
> (2) Another approach might be to move BPF_CGROUP_RUN_PROG_INET_SOCK and
> make it work with AF_PACKET. This might be not 100% backwards compatible
> but I'd assume that most users should look at the socket family before
> doing anything. (in this case it feels like we can extend
> sock_bind/release for af_packets as well, just for accounting purposes,
> without any way to override the target ifindex).
If adding a hook at __sock_create, I think having a new CGROUP_POST_SOCK_CREATE
may be better instead of messing with the current inet assumption
in CGROUP_'INET'_SOCK_CREATE.  Running all CGROUP_*_SOCK_CREATE at
__sock_create could be a nice cleanup such that a few lines can be
removed from inet[6]_create but an extra family check will be needed.

The bpf prog has both bpf_sock->family and bpf_sock->protocol field to
check with, so it should be able to decide the sk type if it is run
at __sock_create.  All bpf_sock fields should make sense or at least 0
to all families (?), please check.

For af_packet bind, the ip[46]/port probably won't be useful?  What
the bpf prog will need?

Re: Override default socket policy per cgroup

[sdf]

On 02/09, Martin KaFai Lau wrote:
> On Wed, Feb 09, 2022 at 09:03:45AM -0800, sdf@google.com wrote:
> > Let's say I want to set some default sk_priority for all sockets in a
> > specific cgroup. I can do it right now using cgroup/sock_create, but it
> > applies only to AF_INET{,6} sockets. I'd like to do the same for raw
> > (AF_PACKET) sockets and cgroup/sock_create doesn't trigger for them :-(
> Other than AF_PACKET and INET[6], do you have use cases for other  
> families?

No, I only need AF_PACKET for now. But I feel like we should create
a more extensible hook point this time (if we go this route).

> > (1) My naive approach would be to add another cgroup/sock_post_create
> > which runs late from __sock_create and triggers on everything.
> >
> > (2) Another approach might be to move BPF_CGROUP_RUN_PROG_INET_SOCK and
> > make it work with AF_PACKET. This might be not 100% backwards compatible
> > but I'd assume that most users should look at the socket family before
> > doing anything. (in this case it feels like we can extend
> > sock_bind/release for af_packets as well, just for accounting purposes,
> > without any way to override the target ifindex).
> If adding a hook at __sock_create, I think having a new  
> CGROUP_POST_SOCK_CREATE
> may be better instead of messing with the current inet assumption
> in CGROUP_'INET'_SOCK_CREATE.  Running all CGROUP_*_SOCK_CREATE at
> __sock_create could be a nice cleanup such that a few lines can be
> removed from inet[6]_create but an extra family check will be needed.

SG. Hopefully I can at least reuse exiting progtype and just introduce
new hook point in __sock_create.

> The bpf prog has both bpf_sock->family and bpf_sock->protocol field to
> check with, so it should be able to decide the sk type if it is run
> at __sock_create.  All bpf_sock fields should make sense or at least 0
> to all families (?), please check.

Yeah, that's what I think as well, existing bpf_sock should work
as is (it might show empty ip/port for af_packet), but I'll do verify
that.

> For af_packet bind, the ip[46]/port probably won't be useful?  What
> the bpf prog will need?

For AF_PACKET bind we would need new ifindex and new protocol. I was  
thinking
maybe new bpf_packet_sock type+helper to convert from bpf_sock is the
way to go here.

For AF_PACKET bind we actually have another use-case where I think
generic bind hook might be helpful. I have a working prototype with  
fmod_ret,
but feels like per-cgroup hook is better (let's me access cgroup local
storage):
We'd like to have a cgroup-enforced TX-only form of raw socket (grant
CAP_NET_RAW+restrict RX path). For AF_INET{,6} it means allow only
socket(AF_INET{,6}, SOCK_RAW, IPPROTO_RAW); that's easily enforcible with
the current hooks. For AF_PACKET it means allow only
socket(AF_PACKET, SOCK_RAW, 0 == ETH_P_NONE) and prohibit bind to  
protocol !=
0.

Re: Override default socket policy per cgroup

[Alexei Starovoitov]

On Wed, Feb 9, 2022 at 1:51 PM <sdf@google.com> wrote:
>
> On 02/09, Martin KaFai Lau wrote:
> > On Wed, Feb 09, 2022 at 09:03:45AM -0800, sdf@google.com wrote:
> > > Let's say I want to set some default sk_priority for all sockets in a
> > > specific cgroup. I can do it right now using cgroup/sock_create, but it
> > > applies only to AF_INET{,6} sockets. I'd like to do the same for raw
> > > (AF_PACKET) sockets and cgroup/sock_create doesn't trigger for them :-(
> > Other than AF_PACKET and INET[6], do you have use cases for other
> > families?
>
> No, I only need AF_PACKET for now. But I feel like we should create
> a more extensible hook point this time (if we go this route).
>
> > > (1) My naive approach would be to add another cgroup/sock_post_create
> > > which runs late from __sock_create and triggers on everything.
> > >
> > > (2) Another approach might be to move BPF_CGROUP_RUN_PROG_INET_SOCK and
> > > make it work with AF_PACKET. This might be not 100% backwards compatible
> > > but I'd assume that most users should look at the socket family before
> > > doing anything. (in this case it feels like we can extend
> > > sock_bind/release for af_packets as well, just for accounting purposes,
> > > without any way to override the target ifindex).
> > If adding a hook at __sock_create, I think having a new
> > CGROUP_POST_SOCK_CREATE
> > may be better instead of messing with the current inet assumption
> > in CGROUP_'INET'_SOCK_CREATE.  Running all CGROUP_*_SOCK_CREATE at
> > __sock_create could be a nice cleanup such that a few lines can be
> > removed from inet[6]_create but an extra family check will be needed.
>
> SG. Hopefully I can at least reuse exiting progtype and just introduce
> new hook point in __sock_create.

Can you take a look at what it would take to add cgroup scope
to bpf_lsm ?
__sock_create() already has
security_socket_create and security_socket_post_create
in the right places.

bpf_lsm cannot write directly into PTR_TO_BTF_ID like the 1st 'sock' pointer.
We can whitelist the write for certain cases.
Maybe prototype it with bpf_lsm and use
bpf_current_task_under_cgroup() helper to limit the scope
before implementing cgroup-scoped bpf_lsm?

There were cases in the past where bpf_lsm hook was in the ideal
spot, but lack of cgroup scoping was a show stopper.
This use case is another example and motivation to extend
what bpf can do with lsm hooks. That's better than
adding a new bpf_cgroup hook in the same location.

Re: Override default socket policy per cgroup

[sdf]

On 02/09, Alexei Starovoitov wrote:
> On Wed, Feb 9, 2022 at 1:51 PM <sdf@google.com> wrote:
> >
> > On 02/09, Martin KaFai Lau wrote:
> > > On Wed, Feb 09, 2022 at 09:03:45AM -0800, sdf@google.com wrote:
> > > > Let's say I want to set some default sk_priority for all sockets in  
> a
> > > > specific cgroup. I can do it right now using cgroup/sock_create,  
> but it
> > > > applies only to AF_INET{,6} sockets. I'd like to do the same for raw
> > > > (AF_PACKET) sockets and cgroup/sock_create doesn't trigger for  
> them :-(
> > > Other than AF_PACKET and INET[6], do you have use cases for other
> > > families?
> >
> > No, I only need AF_PACKET for now. But I feel like we should create
> > a more extensible hook point this time (if we go this route).
> >
> > > > (1) My naive approach would be to add another  
> cgroup/sock_post_create
> > > > which runs late from __sock_create and triggers on everything.
> > > >
> > > > (2) Another approach might be to move BPF_CGROUP_RUN_PROG_INET_SOCK  
> and
> > > > make it work with AF_PACKET. This might be not 100% backwards  
> compatible
> > > > but I'd assume that most users should look at the socket family  
> before
> > > > doing anything. (in this case it feels like we can extend
> > > > sock_bind/release for af_packets as well, just for accounting  
> purposes,
> > > > without any way to override the target ifindex).
> > > If adding a hook at __sock_create, I think having a new
> > > CGROUP_POST_SOCK_CREATE
> > > may be better instead of messing with the current inet assumption
> > > in CGROUP_'INET'_SOCK_CREATE.  Running all CGROUP_*_SOCK_CREATE at
> > > __sock_create could be a nice cleanup such that a few lines can be
> > > removed from inet[6]_create but an extra family check will be needed.
> >
> > SG. Hopefully I can at least reuse exiting progtype and just introduce
> > new hook point in __sock_create.

> Can you take a look at what it would take to add cgroup scope
> to bpf_lsm ?
> __sock_create() already has
> security_socket_create and security_socket_post_create
> in the right places.

> bpf_lsm cannot write directly into PTR_TO_BTF_ID like the 1st 'sock'  
> pointer.
> We can whitelist the write for certain cases.
> Maybe prototype it with bpf_lsm and use
> bpf_current_task_under_cgroup() helper to limit the scope
> before implementing cgroup-scoped bpf_lsm?

> There were cases in the past where bpf_lsm hook was in the ideal
> spot, but lack of cgroup scoping was a show stopper.
> This use case is another example and motivation to extend
> what bpf can do with lsm hooks. That's better than
> adding a new bpf_cgroup hook in the same location.

Cool, if you think we can whitelist some writes in lsm/fentry that
might be a perfect solution (especially if it gets per-cgroup scope).
I'll try to look in that, thanks!

Re: Override default socket policy per cgroup

[Martin KaFai Lau]

On Wed, Feb 09, 2022 at 01:51:38PM -0800, sdf@google.com wrote:
> On 02/09, Martin KaFai Lau wrote:
> > On Wed, Feb 09, 2022 at 09:03:45AM -0800, sdf@google.com wrote:
> > > Let's say I want to set some default sk_priority for all sockets in a
> > > specific cgroup. I can do it right now using cgroup/sock_create, but it
> > > applies only to AF_INET{,6} sockets. I'd like to do the same for raw
> > > (AF_PACKET) sockets and cgroup/sock_create doesn't trigger for them :-(
> > Other than AF_PACKET and INET[6], do you have use cases for other
> > families?
> 
> No, I only need AF_PACKET for now. But I feel like we should create
> a more extensible hook point this time (if we go this route).
> 
> > > (1) My naive approach would be to add another cgroup/sock_post_create
> > > which runs late from __sock_create and triggers on everything.
> > >
> > > (2) Another approach might be to move BPF_CGROUP_RUN_PROG_INET_SOCK and
> > > make it work with AF_PACKET. This might be not 100% backwards compatible
> > > but I'd assume that most users should look at the socket family before
> > > doing anything. (in this case it feels like we can extend
> > > sock_bind/release for af_packets as well, just for accounting purposes,
> > > without any way to override the target ifindex).
> > If adding a hook at __sock_create, I think having a new
> > CGROUP_POST_SOCK_CREATE
> > may be better instead of messing with the current inet assumption
> > in CGROUP_'INET'_SOCK_CREATE.  Running all CGROUP_*_SOCK_CREATE at
> > __sock_create could be a nice cleanup such that a few lines can be
> > removed from inet[6]_create but an extra family check will be needed.
> 
> SG. Hopefully I can at least reuse exiting progtype and just introduce
> new hook point in __sock_create.
> 
> > The bpf prog has both bpf_sock->family and bpf_sock->protocol field to
> > check with, so it should be able to decide the sk type if it is run
> > at __sock_create.  All bpf_sock fields should make sense or at least 0
> > to all families (?), please check.
> 
> Yeah, that's what I think as well, existing bpf_sock should work
> as is (it might show empty ip/port for af_packet), but I'll do verify
> that.
> 
> > For af_packet bind, the ip[46]/port probably won't be useful?  What
> > the bpf prog will need?
> 
> For AF_PACKET bind we would need new ifindex and new protocol. I was
> thinking
> maybe new bpf_packet_sock type+helper to convert from bpf_sock is the
> way to go here.
Right, should follow the existing bpf_skc_to_*() and
RET_PTR_TO_BTF_ID_OR_NULL pattern to return a 'struct packet_sock *'.

> For AF_PACKET bind we actually have another use-case where I think
> generic bind hook might be helpful. I have a working prototype with
> fmod_ret,
> but feels like per-cgroup hook is better (let's me access cgroup local
> storage):
> We'd like to have a cgroup-enforced TX-only form of raw socket (grant
> CAP_NET_RAW+restrict RX path). For AF_INET{,6} it means allow only
> socket(AF_INET{,6}, SOCK_RAW, IPPROTO_RAW); that's easily enforcible with
> the current hooks. For AF_PACKET it means allow only
> socket(AF_PACKET, SOCK_RAW, 0 == ETH_P_NONE) and prohibit bind to protocol
> != 0.
Meaning a generic hook for bind also?
hmm... yeah, instead of adding a new one for AF_PACKET, adding a generic
one may be more useful.
Just noticed there are INET4_POST_BIND and INET6_POST_BIND
instead of one INET_POST_BIND.  It may be worth checking if it was due to some
bummer in the sock.  A quick look seems to be fine, the addrs in the sock are
not overlapped in a union.