Don't check endieness on the host #139
Assignees
Labels
Milestone
Comments
anubhabMajumdar
added
type/bug
|
@hainenber can you comment here if you want this issue assigned to you also? |
|
Yes, please :D |
rbtr
added
priority/2
|
What we want is to handle host to network conversion in bpf code and not handle it in userspace, making the code more efficient. Ideally, we won't need Some references for issue owner to consider:
|
|
Oh that wasn't what I expect at first 😆 . I was just thinking the conversion is already somewhere in eBPF codebase. Thanks for the lead, I'll try getting my hands wet on this one as I didn't work on low-level code before :D |
|
Hand-rolled byte order swap for IPv4 (32-bit addresses): |
@rectified95 maybe you can pick this up now? 🙂 |
|
just want to dust off this issue, when we are getting the src and dst ips from the iphdr |
|
@nddq Yes, those values are always big-endian and need to be converted to little-endian so they can be used by the hosts. I'm working on this now. The question is whether doing this simple operation in bpf is much faster than in Go, but it def wouldn't hurt. |
7 tasks
rectified95
added a commit
to rectified95/retina
that referenced
this issue
Jun 5, 2024
Signed-off-by: Igor Klemenski <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this issue
Jun 7, 2024
# Description
Function `determineEndian()` always returns the same value since the
host byte order is always little-endian, so it can be skipped.
What's more, the byte order of IP address values passed by eBPF programs
doesn't need to be changed, because Golang's `net.IP` type expects
big-endian byte arrays.
**Details:**
We always enter this if-branch `binary.LittleEndian.PutUint32(ip, nn)`
which looks like it performs conversion TO little-endian, while it
actually expects to receive input already IN little-endian.
Note, the input number's bytes are kept in the same order - this
function merely converts a uint32 to a byte array. The result is we
convert a big-endian number to a big-endian byte array.
```
func (littleEndian) PutUint32(b []byte, v uint32) {
_ = b[3] // early bounds check to guarantee safety of writes below
b[0] = byte(v)
b[1] = byte(v >> 8)
b[2] = byte(v >> 16)
b[3] = byte(v >> 24)
}
```
Crucially, `net.IP` expects big-endian byte arrays. For example, IP
`15.14.0.0` (`0x0F0E0000` in host order, and `0x00000E0F` in network
order) should be passed in as `[0F, 0E, 00, 00]` (most significant byte
at the lowest memory address) and so doesn't need inverting. Example
here: https://play.golang.com/p/ykiP6mLB_gG
In summary, we currently do not perform byte order conversion and
shouldn't start to, either in GO or eBPF code.
## Related Issue
Fixes #139
This PR looks almost identical to
#161 which was rejected because
it didn't include byte order conversion in eBPF, but turns out that's
not needed. It also uses the correct uint-to-array conversion functions.
## Checklist
- [x] I have read the [contributing
documentation](https://retina.sh/docs/contributing).
- [x] I signed and signed-off the commits (`git commit -S -s ...`). See
[this
documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification)
on signing commits.
- [x] I have correctly attributed the author(s) of the code.
- [x] I have tested the changes locally.
- [x] I have followed the project's style guidelines.
- [x] I have updated the documentation, if necessary.
- [x] I have added tests, if applicable.
Signed-off-by: Igor Klemenski <[email protected]>
Carpe-Wang
pushed a commit
to Carpe-Wang/retina
that referenced
this issue
Jun 11, 2024
…microsoft#443) Function `determineEndian()` always returns the same value since the host byte order is always little-endian, so it can be skipped. What's more, the byte order of IP address values passed by eBPF programs doesn't need to be changed, because Golang's `net.IP` type expects big-endian byte arrays. **Details:** We always enter this if-branch `binary.LittleEndian.PutUint32(ip, nn)` which looks like it performs conversion TO little-endian, while it actually expects to receive input already IN little-endian. Note, the input number's bytes are kept in the same order - this function merely converts a uint32 to a byte array. The result is we convert a big-endian number to a big-endian byte array. ``` func (littleEndian) PutUint32(b []byte, v uint32) { _ = b[3] // early bounds check to guarantee safety of writes below b[0] = byte(v) b[1] = byte(v >> 8) b[2] = byte(v >> 16) b[3] = byte(v >> 24) } ``` Crucially, `net.IP` expects big-endian byte arrays. For example, IP `15.14.0.0` (`0x0F0E0000` in host order, and `0x00000E0F` in network order) should be passed in as `[0F, 0E, 00, 00]` (most significant byte at the lowest memory address) and so doesn't need inverting. Example here: https://play.golang.com/p/ykiP6mLB_gG In summary, we currently do not perform byte order conversion and shouldn't start to, either in GO or eBPF code. Fixes microsoft#139 This PR looks almost identical to microsoft#161 which was rejected because it didn't include byte order conversion in eBPF, but turns out that's not needed. It also uses the correct uint-to-array conversion functions. - [x] I have read the [contributing documentation](https://retina.sh/docs/contributing). - [x] I signed and signed-off the commits (`git commit -S -s ...`). See [this documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification) on signing commits. - [x] I have correctly attributed the author(s) of the code. - [x] I have tested the changes locally. - [x] I have followed the project's style guidelines. - [x] I have updated the documentation, if necessary. - [x] I have added tests, if applicable. Signed-off-by: Igor Klemenski <[email protected]> Signed-off-by: Carpe-Wang <[email protected]>
matmerr
pushed a commit
to matmerr/retina
that referenced
this issue
Jul 3, 2024
…microsoft#443) # Description Function `determineEndian()` always returns the same value since the host byte order is always little-endian, so it can be skipped. What's more, the byte order of IP address values passed by eBPF programs doesn't need to be changed, because Golang's `net.IP` type expects big-endian byte arrays. **Details:** We always enter this if-branch `binary.LittleEndian.PutUint32(ip, nn)` which looks like it performs conversion TO little-endian, while it actually expects to receive input already IN little-endian. Note, the input number's bytes are kept in the same order - this function merely converts a uint32 to a byte array. The result is we convert a big-endian number to a big-endian byte array. ``` func (littleEndian) PutUint32(b []byte, v uint32) { _ = b[3] // early bounds check to guarantee safety of writes below b[0] = byte(v) b[1] = byte(v >> 8) b[2] = byte(v >> 16) b[3] = byte(v >> 24) } ``` Crucially, `net.IP` expects big-endian byte arrays. For example, IP `15.14.0.0` (`0x0F0E0000` in host order, and `0x00000E0F` in network order) should be passed in as `[0F, 0E, 00, 00]` (most significant byte at the lowest memory address) and so doesn't need inverting. Example here: https://play.golang.com/p/ykiP6mLB_gG In summary, we currently do not perform byte order conversion and shouldn't start to, either in GO or eBPF code. ## Related Issue Fixes microsoft#139 This PR looks almost identical to microsoft#161 which was rejected because it didn't include byte order conversion in eBPF, but turns out that's not needed. It also uses the correct uint-to-array conversion functions. ## Checklist - [x] I have read the [contributing documentation](https://retina.sh/docs/contributing). - [x] I signed and signed-off the commits (`git commit -S -s ...`). See [this documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification) on signing commits. - [x] I have correctly attributed the author(s) of the code. - [x] I have tested the changes locally. - [x] I have followed the project's style guidelines. - [x] I have updated the documentation, if necessary. - [x] I have added tests, if applicable. Signed-off-by: Igor Klemenski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
Currently Retina checks host endianness to enrich port/IP addresses. That is not correct, network is big-endian. Handle conversion in bpf code.
To Reproduce
Ref:
retina/pkg/utils/utils_linux.go
Line 78 in 61e1152
Expected behavior
Network is big-endian. Handle conversion as such bpf code.
Platform (please complete the following information):
The text was updated successfully, but these errors were encountered: