bpfilter icon indicating copy to clipboard operation
bpfilter copied to clipboard
verified publisher icon facebook

Allow fetching rules and counters via bfcli

Open ryantimwilson opened this issue 1 year ago • 13 comments

bpfilter daemon keeps a list of active rules and it also writes counters to BPF maps. Sure, we can access the rules/counters via bpftool - counter maps are named bf_cmap_XXX - but it is more convenient to use an API.

$ sudo bpftool map dump name bf_cmap_000201
[{
        "key": 0,
        "value": {
            "packets": 71220,
            "bytes": 112981553
        }
    },{
        "key": 1,
        "value": {
            "packets": 71288,
            "bytes": 112993982
        }
    }

To do this, we need to implement the BF_REQ_GET_RULES and BF_REQ_GET_RULES APIs in bpfilter backend and fetch the results via bfcli.

bfcli API could look like (where chain/rule are optional):

$ bfcli get-rules <chain> <rule>
$ bfcli get-counters <chain> <rule>

Note bfcli would need to introduce subcommands as bfcli is only used for setting rules now.

ryantimwilson avatar Oct 28 '24 15:10 ryantimwilson

Some thoughts on this:

  • BF_REQ_GET_RULES API is as old as this repository, the daemon changed a lot since then, you might have to modify that logic a bit.
  • bfcli get-rules would be a problem on its own: there is no logic to convert the ruleset back to the format used by bfcli. Maybe it would be worth to piggy-back on bf_xxx_dump() logic? You can print almost anything with bf_xxx_dump(), except for a matcher's payload (but that's easily solvable).
  • bfcli get-counters: nft prints the counters with the rule's definition, that could be a way to do it.

qdeslandes avatar Oct 28 '24 16:10 qdeslandes

Tommy Unger is working on adding get-counters functionality as part of his ramp-up!

ryantimwilson avatar Nov 25 '24 21:11 ryantimwilson

I have a few noob questions here. I'm currently working on get-counters because we agreed that get-rules will require further conversation.

  1. Could it be that we actually want <chain> and <rule> to be required arguments to get-counters? If <chain> and <rule> are optional, what is the expected functionality when one, the other, or both are omitted? Perhaps omitting both shows counters from all chains and all rules. Omitting <rule> shows all counters from all rules in the given <chain>. Omitting <chain> shows all counters corresponding to matching rules across all chains (is this useful?).
  2. Is there an existing bfcli request this get-counters request can extend/piggyback on? After looking it seems like the answer may be "no;" the closest analog I could find was bf_ipt_get_info(). Even if that were usable, we would want to implement a new function using BF_FRONT_CLI, right?
  3. Do I have it right that implementing get-counters as a sub-command is best done by modifying the bfcli parser?

Hope these questions even make sense. I'm only beginning to understand this (very cool) system!

tommy-u avatar Jan 09 '25 01:01 tommy-u

I think I've more or less resolved 2 above. I'm currently working on reading a value out of an existing map. I see bf_map_new() generating file paths for maps like /sys/fs/bpf/bf_cmap_030200. I expected to find a psudo-file there that existed for the lifetime of the bpfilter daemon (after receiving a rule from the CLI), but I don't see any. I assumed this would be the primary handle for interacting with existing maps from userspace (e.g. you'd get a fd using it).

Perhaps it: 1) only lives there transiently, 2) is never actually created, or 3) I need to enable some configuration to show these files.

Currently puzzling on that, will report back if I get to the bottom of it.

tommy-u avatar Jan 17 '25 22:01 tommy-u

Could it be that we actually want and to be required arguments to get-counters? If and are optional, what is the expected functionality when one, the other, or both are omitted? Perhaps omitting both shows counters from all chains and all rules. Omitting shows all counters from all rules in the given . Omitting shows all counters corresponding to matching rules across all chains (is this useful?).

One issue I see with get-counters is that counters only make sense when associated with their rule. I.e. bfcli get-counters $MYCHAIN $MYRULE requires the user to know the chain's name or ID, and the rule's ID, which they can't as get-rules doesn't exist.

It would make more sense to start with a high-level command to fetch the whole ruleset (including the counters), i.e.:

$ bfcli ruleset
chain BF_HOOK_XDP{attach=no,ifindex=2,name=my_xdp_program} policy ACCEPT
    rule
        meta.dport eq 22
        counter 12412 packets 1249120 bytes
        CONTINUE
    rule
        meta.dport not 22
        counter 12 packets 1841 bytes
        CONTINUE

A modified version of bf_chain_dump() (bf_chain_print()?) could be used to print a chain (and a rule) as above, including the counters.

Is there an existing bfcli request this get-counters request can extend/piggyback on? After looking it seems like the answer may be "no;" the closest analog I could find was bf_ipt_get_info().

Following the example above, bf_request_cmd should include BF_REQ_GET_RULESET to retrieve the whole ruleset.

Even if that were usable, we would want to implement a new function using BF_FRONT_CLI, right?

You're right, this new command should be supported specifically for BF_FRONT_CLI.

Do I have it right that implementing get-counters as a sub-command is best done by modifying the bfcli parser?

You would have to modify the arguments' parser, not the Bison parser to support subparser with argp.

Currently, bfcli is only used to define a ruleset and doesn't support subcommands. I would suggest the following plan:

  • [ ] Move the ruleset definition behind a subcommand, e.g. bfcli ruleset set ..., which supports the current ruleset definition options (i.e. --file and --str)
  • [ ] Introduce a new command to fetch the ruleset: bfcli ruleset get (with get optional, ideally)

I think I've more or less resolved 2 above. I'm currently working on reading a value out of an existing map.

I see you started working on this, and I'm changing the requirements and expectations on the fly, sorry about that :/

I see bf_map_new() generating file paths for maps like /sys/fs/bpf/bf_cmap_030200. I expected to find a psudo-file there that existed for the lifetime of the bpfilter daemon (after receiving a rule from the CLI), but I don't see any. I assumed this would be the primary handle for interacting with existing maps from userspace (e.g. you'd get a fd using it).

When creating a map, bpfilter will always associate a path to it. The map will be pinned to this path unless the daemon is run in transient mode (--transient). I assume you run the daemon with --transient? That would explain why the path is empty.

Regarding this task, it's better if bfcli goes through the daemon directly, instead of reading from a map, as the layout in the map is only known by the daemon.

qdeslandes avatar Jan 21 '25 21:01 qdeslandes

UPDATE EDIT: I have since found bf_list_marsh(), which I believe makes the former approach easy. Feel free to disregard.

I have a question about the marshaled data with which the daemon responds to the CLI. Is there a preference for returning a list of chains over, say, a cgen (or ctx for that matter)? Only asking because it seems the former will require writing new marshaling/unmarshaling code while it is already done for the latter.

Thanks!

tommy-u avatar Feb 04 '25 22:02 tommy-u

I'm not quite there yet, but an issue I see coming for ruleset get has to do with providing the per-rule counters. I believe this information is not held within the chains, so it does not seem there is a direct way to provide these to the CLI. I'm curious if I'm missing something simple here.

If I have that right, It might take extra work to marshal them into the daemon's response. Or perhaps ruleset get can provide the rules and indices for the CLI user to use to access the counters using get-counters.

tommy-u avatar Feb 05 '25 18:02 tommy-u

@tommy-u

UPDATE EDIT: I have since found bf_list_marsh(), which I believe makes the former approach easy. Feel free to disregard.

I have a question about the marshaled data with which the daemon responds to the CLI. Is there a preference for returning a list of chains over, say, a cgen (or ctx for that matter)? Only asking because it seems the former will require writing new marshaling/unmarshaling code while it is already done for the latter.

That's right, it should help to serialise a whole list at once. bf_list_unmarsh() doesn't exist, though!

I'm not quite there yet, but an issue I see coming for ruleset get has to do with providing the per-rule counters. I believe this information is not held within the chains, so it does not seem there is a direct way to provide these to the CLI. I'm curious if I'm missing something simple here.

If I have that right, It might take extra work to marshal them into the daemon's response. Or perhaps ruleset get can provide the rules and indices for the CLI user to use to access the counters using get-counters.

You're right. But that's fine by me, the chains are static, but the counters are dynamic. bfcli ruleset get would return the entire ruleset, but not the counters. That's the first step.

Then, bfcli ruleset get could provide something like --with-counters flag, in which case it would request the counters (e.g. BF_REQ_COUNTERS_GET) before printing the ruleset + the counters (similarly to nftables). The response from the daemon could be serialized from:

main bf_marsh
    bf_marsh of a list of counters for chain #1
        bf_marsh for the counters of rule #1 in chain #1
        [...]
    bf_marsh of a list of counters for chain #2
        [...]

qdeslandes avatar Feb 05 '25 19:02 qdeslandes

Hey, I've got a prototype working for ruleset get. The output currently looks something like the following:

sudo bfcli ruleset get
chain BF_HOOK_NF_LOCAL_IN{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes
                ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                counters: yes
                ACCEPT
chain BF_HOOK_NF_LOCAL_OUT{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes
                ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x03 0xff 0xff 0xff 0xff 
                        meta.dport eq debug  : 0x16 0x00 
                counters: yes
                ACCEPT

One concern is how to display the payload information for the matchers. It seems that some instances (like matching a source address) would want dot separated hex values, while others (like matching a port) would want a decimal value. Is there existing support for this disambiguation?

Also the debug : looks bad because I'm using bf_dump_hex() directly and should probably write my own version.

tommy-u avatar Feb 12 '25 19:02 tommy-u

Hey Quentin, I think I'm tying myself up a bit trying to do something simple.

My first approach to the daemon returning the list of chains was to produce a bf_list of bf_chains, then use bf_list_marsh(), then pass the pointer to the marsh data to bf_response_new_success(). This worked fine the first time, but the chains were freed by cleanup_bf_list. A quick hack to avoid this was passing a nop free function when creating the list.

I'm working on a better solution. The idea is to create a list of bf_marsh objects which are marshaled versions of the chains. The trick is that we still have to marshal the list, even though the nodes themselves are already marshaled.

Is there a nop marshaling function? I tried to write a very trivial one, but I'm tripping asserts with it. An alternative approach you'd recommend?

tommy-u avatar Feb 13 '25 22:02 tommy-u

@tommy-u

This prototype looks good, thanks for doing this!

One concern is how to display the payload information for the matchers. It seems that some instances (like matching a source address) would want dot separated hex values, while others (like matching a port) would want a decimal value. Is there existing support for this disambiguation?

I think it's about time to implement callbacks for the matcher, i.e. handle matchers more generically. That's not in the scope of this task though, for now you can print the hex value of the matcher's payload.

My first approach to the daemon returning the list of chains was to produce a bf_list of bf_chains, then use bf_list_marsh(), then pass the pointer to the marsh data to bf_response_new_success(). This worked fine the first time, but the chains were freed by cleanup_bf_list. A quick hack to avoid this was passing a nop free function when creating the list.

Why aren't you happy with this solution? The list is used as a container to serialize the chains, it's not meant to own the chain, so it should not free them. Hence, passing a nop free function is perfectly acceptable!

qdeslandes avatar Feb 14 '25 09:02 qdeslandes

Thanks for pointing me in the right direction up there^

I finished prototyping a version of this "get counters" functionality that has the daemon send the chains and the counter values back to the CLI. The CLI is exercised like: sudo bfcli ruleset get [--with-counters] It uses the approach we discussed, returning an object that could be represented by: marsh( marsh( chain list ), marsh( counters ) ). The CLI can print with or without the counters.

A few notes:

  • I printed all the counters on the same lines after the counters: <yes/no> entry in the rules. I thought it would be good to avoid creating new lines when possible so the display isn't shifting around for the user (as they switch between using the flag or not).
  • As we discussed, the daemon will return a (zeroed) counter even if a rule doesn't have counters enabled. Perhaps a small improvement here would be to drop the printing of 0 bytes 0 packets when we are in the counters: no case.
  • I added a chain counters: line with hard coded yes because I assume every chain is initialized with these values. Happy to keep them or drop them; I thought a developer would want to see them. It's a little odd to see these after the per rule printouts. I could move them after the primary chain line if that would be more sane.
  • The "debug :" printout is because I'm reusing bf_dump_hex(). I didn't dig into those macros far enough to tell if the debug prefix could be configured out. I did make a bf_opts_set_verbose(BF_VERBOSE_DEBUG) call in the CLI so that bf_dump_hex() would print anything at all. Perhaps you have a suggestion here, or I could just duplicate the functionality.

With counters:

$ sudo bfcli ruleset get --with-counters
chain BF_HOOK_NF_LOCAL_IN{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes, 0 bytes 0 packets
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                counters: yes, 0 bytes 0 packets
                verdict: ACCEPT
        chain counters: yes, policy: 21053715 bytes 35648 packets, error: 0 bytes 0 packets

chain BF_HOOK_NF_LOCAL_OUT{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes, 0 bytes 0 packets
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x03 0xff 0xff 0xff 0xff 
                        meta.dport eq debug  : 0x16 0x00 
                counters: yes, 16215423 bytes 34923 packets
                verdict: ACCEPT
        chain counters: yes, policy: 0 bytes 0 packets, error: 0 bytes 0 packets

Without counters:

$ sudo bfcli ruleset get
chain BF_HOOK_NF_LOCAL_IN{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                counters: yes
                verdict: ACCEPT
        chain counters: yes

chain BF_HOOK_NF_LOCAL_OUT{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x03 0xff 0xff 0xff 0xff 
                        meta.dport eq debug  : 0x16 0x00 
                counters: yes
                verdict: ACCEPT
        chain counters: yes

I'm happy to make any functional or aesthetic changes you may have in mind :)

tommy-u avatar Feb 19 '25 22:02 tommy-u

Thanks for pointing me in the right direction up there^

I finished prototyping a version of this "get counters" functionality that has the daemon send the chains and the counter values back to the CLI. The CLI is exercised like: sudo bfcli ruleset get [--with-counters] It uses the approach we discussed, returning an object that could be represented by: marsh( marsh( chain list ), marsh( counters ) ). The CLI can print with or without the counters.

That's great, this is gonna be very useful!

A few notes:

  • I printed all the counters on the same lines after the counters: <yes/no> entry in the rules. I thought it would be good to avoid creating new lines when possible so the display isn't shifting around for the user (as they switch between using the flag or not).

I suggest removing the "yes" or "no" after counters, such as:

chain BF_HOOK_NF_LOCAL_IN{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters 0 bytes 0 packets
                verdict: ACCEPT

When defining a rule, only the keyword is required to request the counters.

  • As we discussed, the daemon will return a (zeroed) counter even if a rule doesn't have counters enabled. Perhaps a small improvement here would be to drop the printing of 0 bytes 0 packets when we are in the counters: no case.

Yes, remove the counters: no altogether.

  • I added a chain counters: line with hard coded yes because I assume every chain is initialized with these values. Happy to keep them or drop them; I thought a developer would want to see them. It's a little odd to see these after the per rule printouts. I could move them after the primary chain line if that would be more sane.

I wonder if that would make the chain line too long?

chain BF_HOOK_NF_LOCAL_IN{attach=yes,ifindex=0,name=(null)} policy: ACCEPT counters 0 bytes 0 packets
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes
                verdict: ACCEPT
  • The "debug :" printout is because I'm reusing bf_dump_hex(). I didn't dig into those macros far enough to tell if the debug prefix could be configured out. I did make a bf_opts_set_verbose(BF_VERBOSE_DEBUG) call in the CLI so that bf_dump_hex() would print anything at all. Perhaps you have a suggestion here, or I could just duplicate the functionality.

How do you print the rules and chains? Without a xxx_dump() function? It could be simpler to open a PR now that you have a working prototype, it's gonna be easier for the next steps.

With counters:

$ sudo bfcli ruleset get --with-counters
chain BF_HOOK_NF_LOCAL_IN{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes, 0 bytes 0 packets
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                counters: yes, 0 bytes 0 packets
                verdict: ACCEPT
        chain counters: yes, policy: 21053715 bytes 35648 packets, error: 0 bytes 0 packets

chain BF_HOOK_NF_LOCAL_OUT{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes, 0 bytes 0 packets
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x03 0xff 0xff 0xff 0xff 
                        meta.dport eq debug  : 0x16 0x00 
                counters: yes, 16215423 bytes 34923 packets
                verdict: ACCEPT
        chain counters: yes, policy: 0 bytes 0 packets, error: 0 bytes 0 packets

Without counters:

$ sudo bfcli ruleset get
chain BF_HOOK_NF_LOCAL_IN{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                counters: yes
                verdict: ACCEPT
        chain counters: yes

chain BF_HOOK_NF_LOCAL_OUT{attach=yes,ifindex=0,name=(null)} policy: ACCEPT
        rule: 0
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x01 0xff 0xff 0xff 0xff 
                counters: yes
                verdict: ACCEPT
        rule: 1
                matcher(s):
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x02 0xff 0xff 0xff 0xff 
                        ip4.saddr eq debug  : 0x0a 0x00 0x00 0x03 0xff 0xff 0xff 0xff 
                        meta.dport eq debug  : 0x16 0x00 
                counters: yes
                verdict: ACCEPT
        chain counters: yes

I'm happy to make any functional or aesthetic changes you may have in mind :)

A few things I see that could be improved:

  • Do not print the unused hook options
  • Try to print the ruleset the way it's fed bfcli
  • Remove the debug : prefix

qdeslandes avatar Feb 20 '25 19:02 qdeslandes