Support context extensions

[amorenoz]

The retis context contains enough information to access common types such as the sk_buff pointer.

Sometimes this information cannot be easily available through the probe's arguments. In such cases, the context needs to be dynamically extended.

Well known types to sk_buff indirection can be implemented in the core so that it can be used in generic probes. However, this might be insufficient for some specific probes, for which a freplaced hook is available.

Note the hook is currently unused, the first user will be #493

[atenart]

I find this quite clever and nice for many reasons! But I have some concerns:

1. This changes the current behavior of supporting getting (and tracking) skbs from any struct nft_traceinfo * or struct nft_pktinfo * enabled function. Now only the targeted probe installed by the nft collector will access that indirect skb (e.g. retis collect -c skb,skb-tracking -p some_nft_function won't work anymore). IMO this is a key feature we want to keep (and extend): indirect accesses.
2. In addition this makes the retrieval of getting such skbs depending on the nft collector being enabled.
3. It makes things a bit scattered: the logic to get the above parameter offsets is in the core while it is (and will be) only
   used by the nft collector now.

Point 2 could be worked out by letting collectors install context hook even if not enabled (there's a slight subtility to get right wrt. the compatibility check collectors make — e.g. not loading the context probe in case the nft collector isn't explicitly enabled and the nft module isn't built-in or loaded).

Point 3 can be easily improved by moving the logic to be inside the collectors. This even raises the question to extend this to all types and let the collectors provide type offsets themselves (extending the logic added here wrt. the getters Retis provides).

Point 1 is more tricky. Context hooks should be added to the right list of probes (user defined, collector defined and core defined). Also multiple collectors could add a context hook to the same probe. I don't really see a problem with the later (a collector operates on his own types). For the former, maybe context hooks should be added to all probes. Or collectors should be able to inspect all probes after all are initialized?

Another way of seeing this would be context hooks are a nice addition for providing a way to do a really specialized thing that only applies to a specific probe; but that doesn't apply to indirect accesses of well known types and here for the nft example.

[amorenoz]

I find this quite clever and nice for many reasons! But I have some concerns:

1. This changes the current behavior of supporting getting (and tracking) skbs from any  `struct nft_traceinfo *` or `struct nft_pktinfo *` enabled function. Now only the targeted probe installed by the `nft` collector will access that indirect skb (e.g. `retis collect -c skb,skb-tracking -p some_nft_function` won't work anymore). IMO this is a key feature we want to keep (and extend): indirect accesses.

Maybe we could have some built-in ctx "hooks" (they would not be hooks anymore) and them a way to freplace a custom one for collector-driven probes?

2. In addition this makes the retrieval of getting such skbs depending on the `nft` collector being enabled.

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem.
I really don't see whey anyone would probe that module without enabling the nft collector.

3. It makes things a bit scattered: the logic to get the above parameter offsets is in the core while it is (and will be) only
   used by the nft collector now.

But it's not in the core anymore, is it? It lives in the nft collector.

Point 2 could be worked out by letting collectors install context hook even if not enabled (there's a slight subtility to get right wrt. the compatibility check collectors make — e.g. not loading the context probe in case the nft collector isn't explicitly enabled and the nft module isn't built-in or loaded).

Point 3 can be easily improved by moving the logic to be inside the collectors. This even raises the question to extend this to all types and let the collectors provide type offsets themselves (extending the logic added here wrt. the getters Retis provides).

Point 1 is more tricky. Context hooks should be added to the right list of probes (user defined, collector defined and core defined). Also multiple collectors could add a context hook to the same probe. I don't really see a problem with the later (a collector operates on his own types). For the former, maybe context hooks should be added to all probes. Or collectors should be able to inspect all probes after all are initialized?

Another way of seeing this would be context hooks are a nice addition for providing a way to do a really specialized thing that only applies to a specific probe; but that doesn't apply to indirect accesses of well known types and here for the nft example.

I think I agree. As I suggested above, we could have a built-in indirect-based skb extraction logic (only outside of retis_get_sk_buff), plus a custom hook.

[atenart]

2. In addition this makes the retrieval of getting such skbs depending on the `nft` collector being enabled.

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem. I really don't see whey anyone would probe that module without enabling the nft collector.

I think doing retis -c skb,ct -p an_nft_func is a valid use case, users don't always want to get the information about the nft actions but only about the state of the skb in there, or even only want to "see" the skb. In the general case I don't think we'd want a strong link between enabling a collector and getting indirect access to a field; that last part should be transparent.

3. It makes things a bit scattered: the logic to get the above parameter offsets is in the core while it is (and will be) only
   used by the nft collector now.

But it's not in the core anymore, is it? It lives in the nft collector.

The offsets are, in retis/src/core/probe/kernel/inspect.rs.

Another way of seeing this would be context hooks are a nice addition for providing a way to do a really specialized thing that only applies to a specific probe; but that doesn't apply to indirect accesses of well known types and here for the nft example.

I think I agree. As I suggested above, we could have a built-in indirect-based skb extraction logic (only outside of retis_get_sk_buff), plus a custom hook.

Maybe we could have some built-in ctx "hooks" (they would not be hooks anymore) and them a way to freplace a custom one for collector-driven probes?

I'm not sure to get the "only outside of retis_get_sk_buff" part.

Except that, if I think I agree. Let me rephrase to be sure: we would keep the indirect access logic for common objects in the core and add context hooks (on targeted probes only) for specialized handling (here providing access to an skb ojbect from a specific ovs probe so that retis_get_sk_buff is aware of it upon later calls)?

[amorenoz]

2. In addition this makes the retrieval of getting such skbs depending on the `nft` collector being enabled.

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem. I really don't see whey anyone would probe that module without enabling the nft collector.

I think doing retis -c skb,ct -p an_nft_func is a valid use case, users don't always want to get the information about the nft actions but only about the state of the skb in there, or even only want to "see" the skb. In the general case I don't think we'd want a strong link between enabling a collector and getting indirect access to a field; that last part should be transparent.

3. It makes things a bit scattered: the logic to get the above parameter offsets is in the core while it is (and will be) only
   used by the nft collector now.

But it's not in the core anymore, is it? It lives in the nft collector.

The offsets are, in retis/src/core/probe/kernel/inspect.rs.

Ok, now I get what you were referring to.

Another way of seeing this would be context hooks are a nice addition for providing a way to do a really specialized thing that only applies to a specific probe; but that doesn't apply to indirect accesses of well known types and here for the nft example.

I think I agree. As I suggested above, we could have a built-in indirect-based skb extraction logic (only outside of retis_get_sk_buff), plus a custom hook.

Maybe we could have some built-in ctx "hooks" (they would not be hooks anymore) and them a way to freplace a custom one for collector-driven probes?

I'm not sure to get the "only outside of retis_get_sk_buff" part.

What I mean is: remove the part of retis_get_sk_buff that currently looks on other arbitrary symbols and performs indirections. It is both unnecessary (no need to run that code multiple times) and confusing IMO (it's the only retis_get_TYPE that does this quirks). Instead, create infrastructure to explicitly update the context once and before filtering is performed and put the built-in ctx-enrichment indirections there. That same infrastructure also calls the ctx-hook if available.

Except that, if I think I agree. Let me rephrase to be sure: we would keep the indirect access logic for common objects in the core and add context hooks (on targeted probes only) for specialized handling (here providing access to an skb ojbect from a specific ovs probe so that retis_get_sk_buff is aware of it upon later calls)?

[atenart]

3. It makes things a bit scattered: the logic to get the above parameter offsets is in the core while it is (and will be) only
   used by the nft collector now.

But it's not in the core anymore, is it? It lives in the nft collector.

The offsets are, in retis/src/core/probe/kernel/inspect.rs.

Ok, now I get what you were referring to.

Side note: I have a plan to make this completely generic; it's just low priority.

Another way of seeing this would be context hooks are a nice addition for providing a way to do a really specialized thing that only applies to a specific probe; but that doesn't apply to indirect accesses of well known types and here for the nft example.

I think I agree. As I suggested above, we could have a built-in indirect-based skb extraction logic (only outside of retis_get_sk_buff), plus a custom hook.

Maybe we could have some built-in ctx "hooks" (they would not be hooks anymore) and them a way to freplace a custom one for collector-driven probes?

I'm not sure to get the "only outside of retis_get_sk_buff" part.

What I mean is: remove the part of retis_get_sk_buff that currently looks on other arbitrary symbols and performs indirections. It is both unnecessary (no need to run that code multiple times) and confusing IMO (it's the only retis_get_TYPE that does this quirks). Instead, create infrastructure to explicitly update the context once and before filtering is performed and put the built-in ctx-enrichment indirections there. That same infrastructure also calls the ctx-hook if available.

So we would make:

• retis_get_sk_buff return either the skb from the probe args or the one found in the indirection logic.
• Make an indirection function, which could be extended on targeted probes.

Right?

That's fine IMO. I think (without having a look at the code) a clean way to handle indirections would be to add extra registers (after REG_MAX) in our existing context arguments and reuse the offsets in our context to point to those. The logic would be unified, and that would allow multi-arg support later (e.g. handling multiple skbs in the same probe).

[amorenoz]

3. It makes things a bit scattered: the logic to get the above parameter offsets is in the core while it is (and will be) only
   used by the nft collector now.

But it's not in the core anymore, is it? It lives in the nft collector.

The offsets are, in retis/src/core/probe/kernel/inspect.rs.

Ok, now I get what you were referring to.

Side note: I have a plan to make this completely generic; it's just low priority.

Another way of seeing this would be context hooks are a nice addition for providing a way to do a really specialized thing that only applies to a specific probe; but that doesn't apply to indirect accesses of well known types and here for the nft example.

I think I agree. As I suggested above, we could have a built-in indirect-based skb extraction logic (only outside of retis_get_sk_buff), plus a custom hook.

Maybe we could have some built-in ctx "hooks" (they would not be hooks anymore) and them a way to freplace a custom one for collector-driven probes?

I'm not sure to get the "only outside of retis_get_sk_buff" part.

What I mean is: remove the part of retis_get_sk_buff that currently looks on other arbitrary symbols and performs indirections. It is both unnecessary (no need to run that code multiple times) and confusing IMO (it's the only retis_get_TYPE that does this quirks). Instead, create infrastructure to explicitly update the context once and before filtering is performed and put the built-in ctx-enrichment indirections there. That same infrastructure also calls the ctx-hook if available.

So we would make:

* `retis_get_sk_buff` return either the skb from the probe args or the one found in the indirection logic.

* Make an indirection function, which could be extended on targeted probes.

Right?

Yes.

That's fine IMO. I think (without having a look at the code) a clean way to handle indirections would be to add extra registers (after REG_MAX) in our existing context arguments and reuse the offsets in our context to point to those. The logic would be unified, and that would allow multi-arg support later (e.g. handling multiple skbs in the same probe).

Yest, that's a nice idea. I'll give it a try

[amorenoz]

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem. I really don't see whey anyone would probe that module without enabling the nft collector.

I think doing retis -c skb,ct -p an_nft_func is a valid use case, users don't always want to get the information about the nft actions but only about the state of the skb in there, or even only want to "see" the skb. In the general case I don't think we'd want a strong link between enabling a collector and getting indirect access to a field; that last part should be transparent.

Note this does not work today.

[atenart]

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem. I really don't see whey anyone would probe that module without enabling the nft collector.

I think doing retis -c skb,ct -p an_nft_func is a valid use case, users don't always want to get the information about the nft actions but only about the state of the skb in there, or even only want to "see" the skb. In the general case I don't think we'd want a strong link between enabling a collector and getting indirect access to a field; that last part should be transparent.

Note this does not work today.

Nice finding. We should probably extend the collector known args with known indirections. E.g. if a collector knows sk_buff, add the current skb indirections.

[amorenoz]

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem. I really don't see whey anyone would probe that module without enabling the nft collector.

I think doing retis -c skb,ct -p an_nft_func is a valid use case, users don't always want to get the information about the nft actions but only about the state of the skb in there, or even only want to "see" the skb. In the general case I don't think we'd want a strong link between enabling a collector and getting indirect access to a field; that last part should be transparent.

Note this does not work today.

Nice finding. We should probably extend the collector known args with known indirections. E.g. if a collector knows sk_buff, add the current skb indirections.

Even so, I think it would still not work given the current implementation of indirection. It is very much focused on __nft_trace_packet, i.e: it required struct nft_traceinfo to be present.

[atenart]

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem. I really don't see whey anyone would probe that module without enabling the nft collector.

I think doing retis -c skb,ct -p an_nft_func is a valid use case, users don't always want to get the information about the nft actions but only about the state of the skb in there, or even only want to "see" the skb. In the general case I don't think we'd want a strong link between enabling a collector and getting indirect access to a field; that last part should be transparent.

Note this does not work today.

Nice finding. We should probably extend the collector known args with known indirections. E.g. if a collector knows sk_buff, add the current skb indirections.

Even so, I think it would still not work given the current implementation of indirection. It is very much focused on __nft_trace_packet, i.e: it required struct nft_traceinfo to be present.

Right, this should be handled too; as part of another effort to add common indirections.

[vlrpl]

Probes with nft_pktinfo as an argument are exclusively in the netfilter subsystem. I really don't see whey anyone would probe that module without enabling the nft collector.

I think doing retis -c skb,ct -p an_nft_func is a valid use case, users don't always want to get the information about the nft actions but only about the state of the skb in there, or even only want to "see" the skb. In the general case I don't think we'd want a strong link between enabling a collector and getting indirect access to a field; that last part should be transparent.

Note this does not work today.

Nice finding. We should probably extend the collector known args with known indirections. E.g. if a collector knows sk_buff, add the current skb indirections.

Even so, I think it would still not work given the current implementation of indirection. It is very much focused on __nft_trace_packet, i.e: it required struct nft_traceinfo to be present.

Yes, it was, mostly because there was no plan/reason to support anything other than what was required to implement rule tracing.

[amorenoz]

The dynamic version of the context extension (ctx hooks) used in #493

[atenart]

This looks pretty good, I guess you can remove "WIP" from the PR title.

[atenart]

LGTM, thanks!