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

elf: do not remove dots from names #1680

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

lmb
Copy link
Collaborator

@lmb lmb commented Feb 13, 2025

elf: do not remove dots from names

The ELF reader currently strips dots from object names if the kernel doesn't
support them. This is against the goal that the ELF reader should produce
the same result no matter which environment it is run in.

Change the setup around slightly, so that the ELF reader now always allows
dots in names. Instead we strip dots if necessary when making the syscalls.

This will break users which:

* use object names with dots in them
* on kernels which don't support dots
* which are pinned into bppfs

Signed-off-by: Lorenz Bauer <[email protected]>

Revert "features: tolerate EPERM in haveObjName and objNameAllowsDot"

This reverts commit cfd8515687f75ac9e95dcfe75db1e3ef0d8f6601.

Now that the ELF reader doesn't rely on feature tests anymore we can undo
the change to treat EPERM specially.

Signed-off-by: Lorenz Bauer <[email protected]>

@lmb lmb force-pushed the elf-reader-names branch from 89e0865 to 14bd6e2 Compare February 13, 2025 12:27
@lmb lmb added the breaking-change Changes exported API label Feb 13, 2025
@lmb lmb force-pushed the elf-reader-names branch from 14bd6e2 to c7b8bd9 Compare February 13, 2025 18:51
@github-actions github-actions bot removed the breaking-change Changes exported API label Feb 13, 2025
lmb added 2 commits March 10, 2025 16:47
The ELF reader currently strips dots from object names if the kernel
doesn't support them. This is against the goal that the ELF reader
should produce the same result no matter which environment it is
run in.

Change the setup around slightly, so that the ELF reader now always
allows dots in names. Instead we strip dots if necessary when making
the syscalls.

This will break users which:

* use object names with dots in them
* on kernels which don't support dots
* which are pinned into bppfs

Signed-off-by: Lorenz Bauer <[email protected]>
This reverts commit cfd8515.

Now that the ELF reader doesn't rely on feature tests anymore we can
undo the change to treat EPERM specially.

Signed-off-by: Lorenz Bauer <[email protected]>
@lmb lmb force-pushed the elf-reader-names branch from c7b8bd9 to 25bd23f Compare March 10, 2025 16:47
@lmb lmb added the breaking-change Changes exported API label Mar 10, 2025
@lmb lmb marked this pull request as ready for review March 10, 2025 16:52
@lmb lmb requested a review from a team as a code owner March 10, 2025 16:52
@lmb
Copy link
Collaborator Author

lmb commented Mar 24, 2025

@ti-mo any concerns about this one?

Copy link
Contributor

@florianl florianl left a comment

Choose a reason for hiding this comment

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

Not Timo here but this would be a great change 👍

@ti-mo
Copy link
Collaborator

ti-mo commented Mar 25, 2025

My main concern is we should've thought this through before we merged the initial version. 😛 Also, I'd really like to unexport SanitizeName; it should've been an implementation detail, but I understand why we provided it.

Also, I found this: https://sourcegraph.com/github.com/cloudflare/xdpcap/-/blob/hook.go?L23-25. We need to give existing users a workaround if we can. Unfortunately, xdpcap substitutes with _ instead of removing the character, which is different from our default. (IMO another reason to unexport the function)

I have a few Qs:

  • Does bpffs also reject those unsupported characters in file names on older kernels?
  • Could we extend the stripping behaviour to Pin() and LoadPinned*() to make it so the user doesn't need to care? (ignoring corner cases like breaking os.Remove for a second)

@lmb
Copy link
Collaborator Author

lmb commented Mar 26, 2025

As far as I know bpffs doesn't have restrictions on what files can be named (outside of the usual rules which apply to all filesystems). The only limitation is on map and program names as passed to the create syscalls. I think that rule came about because programs end up in /proc/kallsyms, but I'm not sure.

I think we shouldn't sanitize in Pin and LoadPinned since bpffs doesn't have these restrictions.

We need to give existing users a workaround if we can. Unfortunately, xdpcap substitutes with _ instead of removing the character, which is different from our default.

You're saying: this change may break xdpcap because on old kernels SanitizeName won't replace . with _? That's a good point, in the case of xdpcap it's going to be fine I think. Name is set as a debug aid. On old kernels which don't allow dots, a name like foo.bar would become foobar instead of foo_bar.

I'd really like to unexport SanitizeName;

Let's tackle that separately if need be?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes exported API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants