Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure compiler_meta__->drop starts out false #4722

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

vbnogueira
Copy link
Contributor

We found scenarios where the skb->cb is not zeroed out before the bpf
program is called. Since compiler_meta__ = skb->cb, there is an issue in
some specific scenarios. For example, since the bpf code looks at
compiler_meta__->drop to tell whether the packet should be dropped or not,
if by any chance compiler_meta__->drop is initialised to true, the
program will mistakenly drop the packet. To solve this, we are
initialising compiler_meta__->drop to false before doing any
processing on compiler_meta__->drop.

@vbnogueira
Copy link
Contributor Author

@komaljai @Sosutha please take a look when you have some time

@fruffy fruffy added p4tc Topics related to the P4-TC back end run-sanitizer Use this tag to run a Clang+Sanitzers CI run. labels Jun 13, 2024
@vbnogueira
Copy link
Contributor Author

@komaljai please take another look when you have some time

@vbnogueira vbnogueira force-pushed the init_drop branch 2 times, most recently from e10c514 to 0168e14 Compare June 18, 2024 16:26
@vbnogueira
Copy link
Contributor Author

@komaljai please take another look when you have some time

So, tested a bit more your suggestion, that is, initialising compiler_meta__->drop to false only when compiler_meta__->recirculated is false and there are cases where it doesn't work
Essentially because in such cases compiler_meta__->recirculated (its memory area in skb->cb) is also initialised with rubbish by the bpf program caller
So I had to revert the change and initialise compiler_meta__->drop to false unconditionally
Since I don't think a scenario where we recirculate with drop set makes much sense (since I believe we should just drop instead of recirculating in that case), I think initialising drop unconditionally should work fine

@komaljai
Copy link
Contributor

komaljai commented Jun 19, 2024

I was referring to struct pna_pre_input_metadata_t->pass field in pna.p4 file. This will give the current pass number and if it is pass 1, then we should initialize the compiler_meta__->drop value to false.
In tc pna.p4 file, we should add PreControlT control block, similar to standard pna.p4 file.

@vbnogueira
Copy link
Contributor Author

I was referring to struct pna_pre_input_metadata_t->pass field in pna.p4 file. This will give the current pass number and if it is pass 1, then we should initialize the compiler_meta__->drop value to false. In tc pna.p4 file, we should add PreControlT control block, similar to standard pna.p4 file.

I see, but if pna_pre_input_metadata_t->pass will come from skb->cb, as does compiler_meta__->recirculated, I believe it would suffer from the same issue (being initialised with rubbish)
That is, of course, if I am understanding this correctly
Also it seems like they removed PreControl from the PNA spec (as of version 0.7)

@Sosutha
Copy link
Contributor

Sosutha commented Jun 19, 2024

Looks fine

@fruffy
Copy link
Collaborator

fruffy commented Jun 19, 2024

Also it seems like they removed PreControl from the PNA spec (as of version 0.7)

This discussion on the customizations of pna.p4 flavors might be relevant. @jafingerhut Do you recall the reason why PreControl was removed?

@jafingerhut
Copy link
Contributor

The reason that PreControl was introduced in the first place was during a long back-and-forth discussion (some of it between me at different times :-) on how to implement decryption in PNA in a way that hopefully multiple NICs could do, including the Intel IPU. Later around late 2022 or early 2023 we found a different way that does not require the PreControl, and seemed cleaner to everyone involved, so it was removed.

@komaljai
Copy link
Contributor

@vbnogueira Could you rebase the PR?

@vbnogueira
Copy link
Contributor Author

@vbnogueira Could you rebase the PR?

Done

We found scenarios where the skb->cb is not zeroed out before the bpf
program is called. Since compiler_meta__ = skb->cb, there is an issue in
some specific scenarios. For example, since the bpf code looks at
compiler_meta__->drop to tell whether the packet should be dropped or not,
if by any chance compiler_meta__->drop is initialised to true, the
program will mistakenly drop the packet. To solve this, we are
initialising compiler_meta__->drop to false before doing any
processing on compiler_meta__->drop.
@komaljai komaljai added this pull request to the merge queue Jun 24, 2024
Merged via the queue into p4lang:main with commit f781e0b Jun 24, 2024
16 checks passed
@vbnogueira vbnogueira deleted the init_drop branch June 25, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4tc Topics related to the P4-TC back end run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants