Skip to content

Reorder structure for compatibility with linux-6.0 #346

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

@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 pytorch#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 pytorch#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.
# for free to join this conversation on GitHub. Already have an account? # 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