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_obj_get_info_by_fd doesn't allow passing structs smaller than sizeof(struct bpf_*_info) #4160

Open
lmb opened this issue Jan 27, 2025 · 2 comments
Assignees
Labels
bug Something isn't working P2 triaged Discussed in a triage meeting
Milestone

Comments

@lmb
Copy link
Collaborator

lmb commented Jan 27, 2025

Describe the bug

Calling bpf_obj_get_info_by_fd with an info_len that is smaller than the corresponding struct bpf_map_info, etc. will cause an error. This is a problem when running a program which was built / linked against an older version of the runtime against a new version where the size of that struct has increased.

The reason for this are checks like the following:

if (*info_size < sizeof(*info)) {
EBPF_LOG_MESSAGE_UINT64_UINT64(
EBPF_TRACELOG_LEVEL_ERROR,
EBPF_TRACELOG_KEYWORD_MAP,
"ebpf_map_get_info buffer too small",
*info_size,
sizeof(*info));
return EBPF_INSUFFICIENT_BUFFER;
}

I think this is also a divergence from upstream libbpf behaviour.

OS information

No response

Steps taken to reproduce bug

You can "simulate" the behaviour like so:

struct bpf_prog_info info; 
uint32_t info_size = sizeof(info) - sizeof(info.pinned_path_count);
bpf_obj_get_info_by_fd(fd, &info, &info_size); // returns an error

Expected behavior

The call to bpf_obj_get_info_by_fd should succeed and fill in all information up to the smaller info_size.

Actual outcome

Return an error.

Additional details

This is the same behaviour as I implemented for the bpf() wrapper:

template <typename T> class ExtensibleStruct
{
private:
void* _source;
size_t _copy_size;
T _temporary;
T* _pointer;
static void
check_tail(_In_reads_bytes_(end) const void* buffer, size_t start, size_t end)
{
const unsigned char* p = (const unsigned char*)buffer + start;
const unsigned char* e = (const unsigned char*)buffer + end;
for (; p < e; p++) {
if ((*p) != 0) {
throw std::runtime_error("Non-zero tail");
}
}
}
public:
ExtensibleStruct(_In_reads_bytes_(size) void* pointer, size_t size) : _source(pointer)
{
if (size >= sizeof(T)) {
// Forward compatibility: allow a larger input as long as the
// unknown fields are all zero.
check_tail(pointer, sizeof(T), size);
_copy_size = 0;
_pointer = (T*)pointer;
} else {
// Backwards compatibility: allow a smaller input by implicitly zeroing all
// missing fields.
memcpy(&_temporary, pointer, size);
_copy_size = size;
_pointer = &_temporary;
}
}
~ExtensibleStruct() { memcpy(_source, &_temporary, _copy_size); }
T*
operator->()
{
return _pointer;
}
T*
operator&()
{
return _pointer;
}
T
operator*()
{
return *_pointer;
}
};

@lmb lmb added the bug Something isn't working label Jan 27, 2025
@lmb
Copy link
Collaborator Author

lmb commented Jan 27, 2025

This really applies to any API which takes a pointer to a struct and a size argument.

@shankarseal
Copy link
Collaborator

The proposal is that the API will attempt to fill the output as much as the buffer passed by the caller. And this should be done for all bpf_* APIs.
One solution would be to use the existing code to fill a local variable and then use memcpy to the caller specified buffer.

@shankarseal shankarseal added the P2 label Jan 27, 2025
@shankarseal shankarseal added this to the 2502 milestone Jan 27, 2025
@shankarseal shankarseal added the triaged Discussed in a triage meeting label Jan 27, 2025
@shankarseal shankarseal modified the milestones: 2502, Backlog, 2501 Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P2 triaged Discussed in a triage meeting
Projects
None yet
Development

No branches or pull requests

3 participants