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

program: Add TestWithContext #1042

Closed

Conversation

jschwinger233
Copy link
Member

This commit adds the TestWithContext function, which allows the acceptance of customized context for eBPF tests and returns the context modified by the eBPF program.

The input context will be cast to a __sk_buff struct for the tc program. For those who want to pass an skb with a special skb->mark or check the returned skb's metadata, TestWithContext can be useful for that purpose.

Signed-off-by: Zhichuan Liang gray.liang@isovalent.com

This commit adds the TestWithContext function, which allows the
acceptance of customized context for eBPF tests and returns the context
modified by the eBPF program.

The input context will be cast to a `__sk_buff` struct for the tc
program. For those who want to pass an skb with a special skb->mark or
check the returned skb's metadata, TestWithContext can be useful for
that purpose.

Signed-off-by: gray <gray.liang@isovalent.com>
@ti-mo
Copy link
Collaborator

ti-mo commented May 25, 2023

@jschwinger233 Does Program.Run() with setting the appropriate RunOptions not do what you need?

@jschwinger233
Copy link
Member Author

@ti-mo Thank you for reply. I did notice Run function, my concern is this special logic for outputPad at:

ebpf/prog.go

Lines 562 to 571 in 211b0e6

func (p *Program) Test(in []byte) (uint32, []byte, error) {
// Older kernels ignore the dataSizeOut argument when copying to user space.
// Combined with things like bpf_xdp_adjust_head() we don't really know what the final
// size will be. Hence we allocate an output buffer which we hope will always be large
// enough, and panic if the kernel wrote past the end of the allocation.
// See https://patchwork.ozlabs.org/cover/1006822/
var out []byte
if len(in) > 0 {
out = make([]byte, len(in)+outputPad)
}

Calling Run() directly requires to expose that magic pad to user, and I assume this is something internal that we'd like to hide from caller?

@lmb
Copy link
Collaborator

lmb commented May 25, 2023

The problem was fixed in torvalds/linux@b5a36b1 aka 5.0-rc1 if I read things right. You also need to call skb_adjust_head or similar to trigger it, with a non-nil DataOut. Does that apply in your case?

Do you want to call this from production code or from tests? If it's just from tests I'd suggest we just turn the panic into an error, and have the caller deal with it. For production we might need to solve this in run() instead.

@jschwinger233
Copy link
Member Author

jschwinger233 commented May 26, 2023

@lmb I'm adding BPF test cases for cilium's IPSec feature, which requires me to check skb->mark after the execution of a BPF program, please see the draft PR: cilium/cilium#25699

In the PR above, I have to define a function runBpfProgram at test/bpf_tests/bpf_test.go, where dataOut = make([]byte, len(data)+256+2) looks magic. Maybe I can leave some comments there to explain the reason? What would you suggest?

@lmb
Copy link
Collaborator

lmb commented May 30, 2023

where dataOut = make([]byte, len(data)+256+2) looks magic.

Maybe just allocate a maximally sized dataOut? Seems like that's what you're already doing for data.

@jschwinger233
Copy link
Member Author

Maybe just allocate a maximally sized dataOut

Sounds good, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants