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

bpf: Return better error codes from hooked syscalls #22965

Merged
merged 1 commit into from Jan 11, 2023

Conversation

gentoo-root
Copy link
Contributor

When a syscall is rejected in BPF, by default the error code returned is -EPERM. Set more appropriate codes explicitly in the remaining places.

Signed-off-by: Maxim Mikityanskiy maxim@isovalent.com

Return better error codes from hooked syscalls, such as connect() and bind().

@gentoo-root gentoo-root added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jan 6, 2023
@gentoo-root gentoo-root requested a review from a team as a code owner January 6, 2023 19:13
@gentoo-root gentoo-root requested a review from jibi January 6, 2023 19:14
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 6, 2023
@jibi
Copy link
Member

jibi commented Jan 9, 2023

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.9' hit: #22578 (98.61% similarity)

@jibi jibi added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Jan 9, 2023
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Code changes look good to me, but could you explain the rationale behind the two error codes?

@gentoo-root
Copy link
Contributor Author

Code changes look good to me, but could you explain the rationale behind the two error codes?

  1. Error codes are chosen according to the man pages of the relevant syscalls, from the list of return codes that can already be returned.
  2. ECONNREFUSED when there is no health service to connect. ENOBUFS when map_update_elem returned an error, which, in my understanding, in this code can only happen when The number of elements in the map reached the *max_entries* limit specified at map creation time, i.e. it's an internal error of the implementation; the closest I could find among the error codes of bind() was ENOBUFS: Insufficient resources were available to complete the call.

@pchaigno
Copy link
Member

Could you add that information to the commit description?

When a syscall is rejected in BPF, by default the error code returned is
-EPERM. Set more appropriate codes explicitly in the remaining places.

The new error codes are chosen according to the documented possible
return values of connect() and bind() syscalls in their corresponding
man pages.

When there is no health service to connect, ECONNREFUSED is returned.

When map_update_elem returns an error (should only happen if max_entries
is exceeded), ENOBUFS is returned, which is the closest error from the
man page that indicates some internal error of the implementation:
`Insufficient resources were available to complete the call`.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
@gentoo-root
Copy link
Contributor Author

Could you add that information to the commit description?

Done.

@jibi
Copy link
Member

jibi commented Jan 10, 2023

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 10, 2023
@aditighag aditighag merged commit 47eae08 into cilium:master Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants