ioctl: add SIOCETHTOOL

[julianuziemblo]

Description

• ioctl: serialize special ioctls in userspace
  There are some special cases where the input to the ioctl syscall is
  a structure with a pointer, which has to be detected and treated
  uniquely by the kernel. This leads to asymmetric behaviour
  (serialization in kernel, but deserialization in userspace) and
  code duplication.
  This change introduces simple packing and unpacking functions, which
  work only for the special cases, allocating memory for the whole
  request to be sent in the syscall and deallocating it when the syscall
  returns.

• ioctl: add SIOCETHTOOL
  SIOCETHTOOL has to be handled specially because of the pointer
  in struct ifreq, where the ethtool structure is saved. The
  sub-structures can have different sizes depending on the always-present
  uint32_t cmd field as well as length/size fields in the structures themselves.

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change):
  • change signature of ioctl syscall proxy function: int sys_ioctl(int fildes, unsigned long request, void *val, size_t size)
    This was done to pass the size of the size of the whole structure to the kernel without affecting the request variable, so we can still match on it in the device server handling the ioctl. This eliminates the need to handle special cases in the kernel: userspace serializes the ioctl, and device server unpacks it.
• Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

• Already covered by automatic testing.
• New test added: ioctl: add special ioctl tests phoenix-rtos-tests#417
• Tested by hand on: armv7m7-imxrt106x-evk, ia32-generic-qemu.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing linter checks and tests passed.
• My changes generate no new compilation warnings for any of the targets.

Special treatment

• This PR needs additional PRs to work:
  • !ioctl: serialize special ioctls outside of kernel phoenix-rtos-kernel#656
  • ioctl: add SIOCETHTOOL phoenix-rtos-lwip#102
• I will merge this PR by myself when appropriate.

[gemini-code-assist]

Hello @julianuziemblo, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello team, gemini-code-assist here to provide a summary of this pull request. This PR introduces support for the SIOCETHTOOL ioctl command and refactors the handling of specific ioctls that involve structures with pointers. The core idea is to move the serialization and deserialization logic for these complex ioctls from the kernel space to the userspace library (libphoenix). This aims to simplify the kernel code, avoid duplication, and provide a more consistent interface for handling these special cases.

Highlights

• Userspace Serialization/Deserialization: The PR introduces a new ioctl-helper module in libphoenix responsible for serializing complex ioctl arguments (like those with embedded pointers or variable-sized data) into a contiguous buffer before the syscall and deserializing the results back into the original structure after the syscall returns. This handles SIOCGIFCONF, SIOCADDRT, SIOCDELRT, and the newly added SIOCETHTOOL.
• Add SIOCETHTOOL Support: Definitions for the SIOCETHTOOL ioctl command and the extensive set of ethtool structures, commands, and constants are added. The ioctl-helper includes logic to correctly serialize and deserialize the various ethtool structures based on the cmd field and internal size/length fields.
• sys_ioctl Signature Change (Breaking): The signature of the sys_ioctl syscall proxy function is changed to include an explicit size_t size argument. This allows the userspace library to pass the total size of the serialized buffer to the kernel, eliminating the need for the kernel to calculate the size based on the request code for special cases.
• Kernel Simplification: The kernel-side ioctl_unpackEx function is simplified by removing the special case handling logic for SIOCGIFCONF, SIOCADDRT, and SIOCDELRT, as this is now managed in userspace. The size of the data to unpack is now taken directly from the new size field in the ioctl_in_t structure (implicitly passed via the new syscall signature).

Changelog

Click here to see the changelog

• include/phoenix/ethtool.h
  • Added a new header file defining ethtool commands, constants, enums, and structures (lines 1-1196).
• include/sys/sockios.h
  • Added the definition for the SIOCETHTOOL ioctl command (line 56).
• sys/ioctl.c
  • Removed includes for sys/sockios.h, net/if.h, and stdlib.h as special case handling is moved to userspace (lines 23-27 removed).
  • Changed how the size is determined in ioctl_unpackEx to use the explicit size argument instead of IOCPARM_LEN (line 46).
  • Removed the special case handling logic for SIOCGIFCONF, SIOCADDRT, and SIOCDELRT (lines 66-89 removed).
• unistd/Makefile
  • Added ioctl-helper.o to the list of objects to be compiled (line 7).
• unistd/file.c
  • Included the new ioctl-helper.h header (line 32).
  • Modified the userspace ioctl wrapper to call ioctl_serialize before sys_ioctl and ioctl_deserialize after, and updated the sys_ioctl external declaration to include the size argument (lines 546, 562-570).
• unistd/ioctl-helper.c
  • Added a new source file implementing ioctl_serialize and ioctl_deserialize for special case ioctls, including detailed logic for various ethtool commands (lines 1-372).
• unistd/ioctl-helper.h
  • Added a new header file declaring the ioctl_serialize and ioctl_deserialize functions (lines 1-33).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

An ioctl call, a tricky feat,
With pointers nested, oh so sweet.
Now userspace takes the load,
Packing data down the road,
Kernel's lighter, can't be beat.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a new mechanism for handling complex ioctl calls by serializing/deserializing data in userspace, which is a good step towards simplifying kernel interfaces. The addition of SIOCETHTOOL is a valuable feature.

While the overall direction is positive, there are several critical and high-priority issues related to memory safety and robustness that need to be addressed before this can be merged. The most significant is in the IOCTL_UNPACK macro, which could lead to dangling pointers in the calling application. Other points include potential NULL dereferences and portability concerns.

Please review the detailed comments below.

Summary of Findings

• Memory Safety: The IOCTL_UNPACK macro in ioctl-helper.c incorrectly reassigns a pointer in the user's structure to point into a temporary buffer that is subsequently freed, leading to a dangling pointer (Critical).
• NULL Pointer Dereference: In sockioctl_packEthtool (ioctl-helper.c), ifr->ifr_data might be dereferenced without a NULL check, potentially causing a crash (High).
• Portability: Use of GCC-specific __builtin_popcountll in ioctl-helper.c may limit portability (Medium).
• Incomplete Logic/Hardcoding: The size calculation for ETHTOOL_PERQUEUE in sockioctl_packEthtool (ioctl-helper.c) is hardcoded for ethtool_coalesce, which might be incorrect for other sub-commands (Medium).
• Robustness: ioctl_packRtEntry (ioctl-helper.c) relies on rt->rt_dev being null-terminated; strlen on an unterminated string is a risk (Medium).
• Code Duplication: There are duplicate macro definitions in include/phoenix/ethtool.h (e.g., ETHTOOL_F_UNSUPPORTED, MAX_NUM_QUEUE). (Not commented inline due to review settings).

Merge Readiness

This pull request makes significant and valuable changes to ioctl handling. However, due to the critical memory safety issue in IOCTL_UNPACK and other high/medium severity concerns (like potential NULL dereferences and hardcoded logic), I must recommend that changes be made to address these issues before merging. Once these are resolved, the PR will be in much better shape. As an AI, I am not authorized to approve pull requests.