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.

Sign up for GitHub

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

Jump to bottom

lkl: Remove the stat symbol hijack #583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

lkl: Remove the stat symbol hijack #583

wants to merge 1 commit into from
+0 −9

Conversation

timschumi
Copy link

@timschumi timschumi commented Mar 16, 2025 •
edited
Loading

The replacement would be to use __xstat as the implementation instead, but the requirements on the vers parameter make it hard to portably call.

Since the symbol just delegates to the real implementation unconditionally and therefore does not seem to have a deeper purpose, just remove it.

Fixes: #361

@timschumi
Copy link
Author

@thehajime Got any recollection on why the __x line of interfaces was chosen and why the 64-bit variant specifically? It seems like that was additional engineering effort without any noticeable benefit.

@timschumi
Copy link
Author

Erk, not sure what Ubuntu has done to their glibc...

@timschumi timschumi marked this pull request as draft March 16, 2025 18:40
@timschumi timschumi marked this pull request as ready for review March 16, 2025 22:32
@timschumi
Copy link
Author

Turns out aarch64 apparently uses different constants, marking as a draft again.

@timschumi timschumi marked this pull request as draft March 17, 2025 00:01
@timschumi timschumi changed the title lkl: Fix up libhijack stat ABI lkl: Remove the stat symbol hijack Mar 17, 2025
@timschumi timschumi marked this pull request as ready for review March 17, 2025 05:52
@timschumi
Copy link
Author

Removing the symbol fully passes CI, let me know if there is some user I am missing. The only alternative to this approach (as far as I can see) is to update the minimum environment to have at least glibc 2.33, where a non-internal stat symbol got introduced that we could rely on.

tavip
tavip reviewed Mar 18, 2025
View reviewed changes
Copy link
Member

@tavip tavip left a comment

Choose a reason for hiding this comment

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

LGTM, but lets wait for @thehajime to review this as we don't run DPDK tests in CI.

@timschumi
lkl: Remove the stat symbol hijack
5920e09 
We are hijacking `stat`, where the size of the target struct is
dependent on the wordsize. Delegating to `__xstat64` is incorrect, as it
unconditionally uses the stat64 layout and will result in an out-of-bounds
write on 32-bit architectures.

The replacement would be to use `__xstat` as the implementation instead,
but the requirements on the `vers` parameter make it hard to portably
call.

Since the symbol just delegates to the real implementation
unconditionally and therefore does not seem to have a deeper purpose,
just remove it.

Fixes: d4b9b65 ("lkl: update dpdk version to 17.02 from 2.2")
Signed-off-by: Tim Schumacher <[email protected]>
@timschumi
Copy link
Author

No changes made in the force push, had an accidental leftover line in the commit message from squashing.

@thehajime
Copy link
Member

thanks @timschumi for the patch.
sorry but I don't remember what I did in 8 years ago (oh..). my best guess is as we only used DPDK on x86_64, we only tested on the platform while silencing build failures with ifdefs, in the particular part of the code.

the changes looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tavip tavip tavip left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@timschumi @thehajime @tavip