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

Reorder structure for compatibility with linux-6.0 #346

Closed
wants to merge 1 commit into from
Closed

Reorder structure for compatibility with linux-6.0 #346

wants to merge 1 commit into from

Conversation

cdluminate
Copy link
Contributor

Copy link
Contributor

@kumpera kumpera left a comment

Choose a reason for hiding this comment

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

LGTM

@facebook-github-bot
Copy link

@fduwjj has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link

@fduwjj merged this pull request in 1090929.

fduwjj added a commit that referenced this pull request Jan 18, 2023
@maurimo
Copy link

maurimo commented Jan 24, 2023

How can this be correct? the order (req first, the buffer after in the struct) is indeed to make space for a zero-sized array at the end of req. If you put req at the end then there is not enough memory and buffer might overflow?
See #348 for an alternative fix to this issue.

@Flamefire
Copy link

How can this be correct? the order (req first, the buffer after in the struct) is indeed to make space for a zero-sized array at the end of req. If you put req at the end then there is not enough memory and buffer might overflow? See #348 for an alternative fix to this issue.

This change is indeed wrong. However it doesn't overflow the buffer (the size is still the same & link_mode_data is never touched) it will "just" not work at all: ecmd.req.cmd = ETHTOOL_GLINKSETTINGS; actually sets an entry in link_mode_masks (of ethtool_link_settings) and leaves the cmd zeroed which I guess in practice means the ioctl will always return an error.

Flamefire added a commit to Flamefire/gloo that referenced this pull request Mar 21, 2024
The `SIOCETHTOOL` IOCTL expects a pointer to an instance
of `ethtool_link_settings`.
E.g. it will read the `cmd` member to determine what to do.
However facebookincubator#346 reordered the memory layout of the pointer such that
actually and array of `__32` values (zeroed out) is passed.
Hence the IOCTL will either fail because an invalid command (`cmd=0`)
is passed or the values read later by e.g.
`ecmd.req.link_mode_masks_nwords` are something completely different.

So `ethtool_link_settings` has to come before the (stack) memory used in
the flexible array at the end of this struct.
To avoid GCC warnigns/errors (see facebookincubator#345) an union is used that provides
the current struct (i.e. with wrong order) and an access the actually
used `struct ethtool_link_settings` at the top.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fails to build against linux 6.0.3
5 participants