Add Test for meta filter on skb device name

sundaram123krishnan commented 3 months ago

Implemented test for meta filter on skb device name and skb inum

Any suggestions are welcome!

sundaram123krishnan commented 3 months ago

Not sure why it fails on ubuntu and centos 8

atenart commented 3 months ago

Not sure why it fails on ubuntu and centos 8

Looks like using meta filtering triggers the following BPF error and probes are not loading: invalid read from stack off -64+0 size 1.

W/o digging to much into this, looks like an issue in the meta filtering logic (bug or old verifier being too picky). @vlrpl could you have a look at this?

vlrpl commented 3 months ago

Not sure why it fails on ubuntu and centos 8

Looks like using meta filtering triggers the following BPF error and probes are not loading: invalid read from stack off -64+0 size 1.

W/o digging to much into this, looks like an issue in the meta filtering logic (bug or old verifier being too picky). @vlrpl could you have a look at this?

yes, it's not related to the PR, but it is probably something to adjust to make the verifier happy. I'll try to find some time to look into it.

In the meantime, some quick general comments about the PR:

First of all, thanks @sundaram123krishnan for working on this :)

I think your second patch can be squashed into the first. Also the linter is not fully happy yet. You can verify this locally with:

python3 -m black --diff tests/test_filter.py

removing --diff you can fix it inline (I suggest you to double check the result anyways if you decide to use inline correction).

The added test is very useful, and indeed uncovered a verification problem during the load, but before closing #58 we should probably consider some more tests. It's ok to add one or a couple for the time being, though. We can incrementally add tests until we reach a point where #58 can be considered done.

Here are some suggestions about possible tests:

packet filters
- as @amorenoz said in #58, L2 and L2+L3 would be nice to have but any test is welcome here
meta
- strings (done in this PR)
- numbers (e.g. see in the doc retis collect -m 'sk_buff.dev.nd_net.net.ns.inum == ...)
- bitfield match sk_buff.pkt_type == ...
- bitfield + mask match (might be the same bitfield simply masked, e.g. sk_buff.pkt_type:0x... == ...
- ideally cast + mask match (e.g. sk_buff._nfct:~0x7:nf_conn.mark that requires ct mark to be set with, e.g. nft)

sundaram123krishnan commented 3 months ago

Thank you for the feedback and guidance. Will make necessary adjustments to address the formatting issues, further more will start working on additional tests as suggested.

sundaram123krishnan commented 3 months ago

Added test for meta filter on inode number

sundaram123krishnan commented 3 months ago

So, i have added tests for filtering of packets based on inum value. It only passes on aarch64 architecture. Not sure, the added tests is correct? Also, i am a bit confused on extracting inum value from the skb device by using the skb:kfree_skb probe. Any help is much appreciated!

sundaram123krishnan commented 3 months ago

When i try to run,

retis collect -m 'sk_buff.dev.nd_net.net.ns.inum == 4026531840' --out /tmp/events.json

I get this output

{
   "common":{
      "smp_id":6,
      "task":{
         "comm":"waybar",
         "pid":1520,
         "tgid":1520
      },
      "timestamp":12454941603790
   },
   "kernel":{
      "probe_type":"raw_tracepoint",
      "symbol":"skb:kfree_skb"
   },
   "skb":{
      "dev":{
         "ifindex":1,
         "name":"lo",
         "rx_ifindex":1
      },
      "ip":{
         "daddr":"::1",
         "ecn":0,
         "len":40,
         "protocol":6,
         "saddr":"::1",
         "ttl":64,
         "v6":{
            "flow_label":276347
         }
      },
      "packet":{
         "capture_len":94,
         "len":94,
         "packet":"AAAAAAAAAAAAAAAAht1gBDd7ACgGQAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAABtC4ZyGk+9w0AAAAAoAL/xAAwAAACBP/EBAIICvioS3QAAAAAAQMDBw=="
      },
      "tcp":{
         "ack_seq":0,
         "doff":10,
         "dport":6600,
         "flags":2,
         "seq":1765734157,
         "sport":46126,
         "window":65476
      }
   },
   "skb-drop":{
      "drop_reason":"NO_SOCKET"
   },
   "skb-tracking":{
      "orig_head":18446614611645826944,
      "skb":18446614612065586920,
      "timestamp":12454941603790
   }
}

amorenoz commented 3 months ago

When i try to run,

retis collect -m 'sk_buff.dev.nd_net.net.ns.inum == 4026531840' --out /tmp/events.json

I don't think hardcoding the inode number is going to work every time. Where did you get that number from? and how do you know it'll always be that number?

vlrpl commented 3 months ago

When i try to run,
retis collect -m 'sk_buff.dev.nd_net.net.ns.inum == 4026531840' --out /tmp/events.json
I don't think hardcoding the inode number is going to work every time. Where did you get that number from? and how do you know it'll always be that number?

I agree with @amorenoz.

A quick way to retrieve it could be something like something like (inside the ns): readlink /proc/self/ns/net

Also, 4026531840 should be inum for init_net in your case, so the filter might match other packets making the test flaky (unless you narrow it down by adding some packet filter).

sundaram123krishnan commented 3 months ago

Ok, will look into it

sundaram123krishnan commented 3 months ago

I got this value, by entering the following command:

 sudo lsns -t net

sundaram123krishnan commented 3 months ago

Quick update on the PR: As @vlrpl suggested, I added readlink to retrieve the inum value dynamically instead of hardcoding it. Further more, it passes all tests

sundaram123krishnan commented 3 months ago

Any update on this pr? @amorenoz

vlrpl commented 2 months ago

Any update on this pr? @amorenoz

sorry for the late reply. Vacations don't help in this matter (some are ongoing for some of us and some will be :)

That being said, before completing the review, cleaning up the PR a little is required. I think this pr has to contain two commits:

Add test for meta filter on skb device name
Add test for meta filter on skb inum

Incremental changes (lints or format) should be part of the same logical change. So, please, squash the incremental changes in those patches. This has the bonus of easing the review process.

The two resulting patches have to include the Signed-off-by trailer (see code contribution guidelines for further details).

Also, please update the PR description removing the "Closes #58" as more tests are needed to close it.

sundaram123krishnan commented 2 months ago

Really sry for the inconvinience @vlrpl @amorenoz i messed some things in my git. So, opened another pr regarding this, addressing all the changes that u mentioned.

sundaram123krishnan commented 2 months ago

As the main branch was updated twice after my pr, i found it difficult to squash commits

retis-org / retis

Add Test for meta filter on skb device name #420