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

maps: Handle GetValueAndDeleteBatch return code appropiately #157

Conversation

javierhonduco
Copy link
Contributor

@javierhonduco javierhonduco commented Apr 27, 2022

Long story short, some libbpf APIs that don't return pointers, such as the
batch APIs might return -1 when there's a 'partial success'. In the case
of these APIs, with errno=-ENOENT, we know that we read and deleted less than
count entries, but it was still overall sucessful.

This PR fixes this for GetValueAndDeleteBatch only. We aren't addressing
all the other batch calls yet as we haven't used them
yet.

Kemal originally tacked this in #152, but unfortunately
not checking the return code and errno makes the interpretation of return values
bogus. I have traced the kernel code and we never returned EFAULT at least in this
libbpf call.

Test plan

$ make run-dynamic
make -C /home/vagrant/work/libbpfgo libbpfgo-dynamic
make[1]: Entering directory '/home/vagrant/work/libbpfgo'
CC=clang \
        CGO_CFLAGS= \
        CGO_LDFLAGS="-lelf -lz -lbpf" \
        go build .
make[1]: Leaving directory '/home/vagrant/work/libbpfgo'
CC=clang \
        CGO_CFLAGS= \
        CGO_LDFLAGS="-lelf -lz -lbpf" \
        go build -o ./main-dynamic ./main.go
sudo ./run.sh main-dynamic
[*] SUCCESS: all good

Thanks!

cc/ @Sylfrena @v-thakkar @kakkoyun

@CLAassistant
Copy link

CLAassistant commented Apr 27, 2022

CLA assistant check
All committers have signed the CLA.

@javierhonduco javierhonduco marked this pull request as ready for review April 27, 2022 16:51
Copy link
Contributor

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I think we might get away without changing the signatures.
Also, I have an inlined question.

libbpfgo.go Show resolved Hide resolved
libbpfgo.go Outdated Show resolved Hide resolved
@javierhonduco javierhonduco force-pushed the get-and-delete-batch-errorcode-fix branch from 420f323 to 81933ce Compare April 28, 2022 09:33
Long story short, some libbpf APIs that don't return pointers, such as the
batch APIs might return `-1` when there's a 'partial success'. In the case
of these APIs, with errno=ENOENT, we know that we read and deleted less than
`count` entries, but it was still overall sucessful. For more  more background
see the inline comments.

This PR fixes this for `GetValueAndDeleteBatch` only. We aren't addressing
all the other batch calls yet as we haven't used them
yet.

Thanks!
@javierhonduco javierhonduco force-pushed the get-and-delete-batch-errorcode-fix branch from 81933ce to f2b3ab8 Compare April 28, 2022 09:42
@javierhonduco
Copy link
Contributor Author

BTW this PR should fix #147

Copy link
Contributor

@grantseltzer grantseltzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM! Thanks for your help.

I'd really like to make sure each batch API is updated for correctness alongside this. Do you plan on doing so? It's alright if not, I just want to add it to my queue and keep track!

@grantseltzer grantseltzer merged commit fb3d682 into aquasecurity:main Apr 29, 2022
@javierhonduco javierhonduco deleted the get-and-delete-batch-errorcode-fix branch May 3, 2022 07:41
@javierhonduco
Copy link
Contributor Author

@grantseltzer I will probably tackle the DeleteKeyBatch API, will keep you updated! Created #162 to track it

javierhonduco pushed a commit to javierhonduco/parca-agent that referenced this pull request May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this pull request May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this pull request May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this pull request May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326)
to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handle with errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
javierhonduco added a commit to javierhonduco/parca-agent that referenced this pull request May 5, 2022
This PR is based off [@kakkoyun's work](parca-dev#326) to use libbpf(go)'s batch APIs.

**Context**
The main issue we found while working with this API was that they were erroring with `EPERM`.
After some debugging, we realised that libbpf wasn't handling errors in the way we
expected.

The debugging write-up and more context can be found [here](aquasecurity/libbpfgo#159),
and the fix is in [this PR](aquasecurity/libbpfgo#157).

After landing these changes upstream, we pointed to the updated libbpfgo version, as well as added
some [regression tests](parca-dev#381) to ensure that libbpfgo
behaves as expected, and to make it easier in the future to write further compatibility tests.

Note: the rest of the batch APIs error handling is still unfixed. Tracking in aquasecurity/libbpfgo#162.
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.

"operation not permitted" errors for batch operations
5 participants