!ioctl: serialize special ioctls outside of kernel #656
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @julianuziemblo, I'm Gemini Code Assist1! 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 everyone, gemini-code-assist here to provide a summary of this pull request. The primary goal of this PR, as indicated by the title and description, is to shift the responsibility of serializing and deserializing complex ioctl arguments (specifically those containing pointers or variable-sized data) from the kernel/common syscall layer to userspace. This makes the kernel's handling of ioctl more transparent and generic. To achieve this, the ioctl syscall proxy is modified to accept an explicit size parameter for the argument data, in addition to the request code and the data pointer itself. This allows userspace libraries (like libphoenix, as mentioned in the description) and device servers to handle the specific packing and unpacking logic based on the request code and the provided size, removing the need for special-case handling within the core syscall implementation.
Highlights
- Modified ioctl syscall signature: The
ioctlsyscall proxy function signature has been changed to include a fourth argument,size_t size, allowing userspace to pass the explicit size of the ioctl argument data. - Added size field to ioctl message structure: The internal
ioctl_in_tstructure used for passing ioctl data within messages has been updated to include asizefield to store the explicit size provided by the userspace caller. - Removed special-case ioctl handling: Specific handling logic and definitions for complex ioctls like
SIOCGIFCONF,SIOCADDRT, andSIOCDELRThave been removed from the commonposixlayer, pushing this serialization/deserialization responsibility to userspace and device servers.
Changelog
Click here to see the changelog
- include/ioctl.h
- Added
unsigned long size;field to theioctl_in_tstruct.
- Added
- posix/posix.c
- Removed definitions for
SIOCGIFCONF,SIOCADDRT,SIOCDELRT, and the_IOCmacro. - Modified
ioctl_packfunction signature to acceptsize_t sizeas a new parameter. - Removed special-case packing logic for
SIOCGIFCONF,SIOCADDRT, andSIOCDELRTfromioctl_pack. - Added
ioctl->size = size;inioctl_packto store the provided size. - Modified
ioctl_processResponsefunction signature to acceptsize_t sizeas a new parameter. - Removed special-case response processing logic for
SIOCGIFCONFfromioctl_processResponse. - Modified
posix_ioctlfunction to retrieve thesizeargument from the user stack (as the 4th argument). - Updated calls to
ioctl_packandioctl_processResponseinposix_ioctlto pass the newsizeargument.
- Removed definitions for
- posix/posix_private.h
- Removed the definitions for
struct ifconfandstruct rtentry, as their special handling is moved out.
- Removed the definitions for
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 kernel's plea,
For data packed, for all to see.
No pointers now, just bytes in line,
Userspace's task, a new design.
Footnotes
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the ioctl handling by moving the serialization/deserialization of special ioctls to userspace. The core change involves adding an explicit size parameter to the sys_ioctl syscall and propagating this size through the internal ioctl processing functions. This allows for variable-sized ioctl payloads to be handled correctly, with userspace taking responsibility for the data structure layout.
The changes are clear and seem to address the stated problem. The removal of kernel-side special ioctl handling logic (like for SIOCGIFCONF) and related definitions is consistent with the goal.
A few points to note:
- Breaking Change: As correctly identified, this is a breaking change to the
sys_ioctlsyscall signature. All callers will need to be updated. - Dependent PRs: The PR description mentions this needs additional PRs to work (e.g., in
libphoenixand device servers). This is crucial for the functionality to be complete. - Documentation: The syscall documentation will need to be updated to reflect the new 4th
sizeargument forioctl.
Overall, the implementation appears robust for the kernel-side changes.
Summary of Findings
- Core Logic Change: The introduction of an explicit
sizeparameter for ioctls, moving serialization to userspace, is well-implemented. The changes inioctl_pack,ioctl_processResponse, andposix_ioctlconsistently use this new size, allowing for flexible payload handling. - Code Cleanup: The removal of now-unnecessary macros (
_IOC,SIOCGIFCONF, etc.) and structure definitions (ifconf,rtentry) fromposix.candposix_private.his clean and aligns with the refactoring goals. - Maintainability Improvement: The addition of a
break;statement in thedefaultcase of theswitchinposix_ioctlis a good practice, improving code robustness against future modifications. - Breaking Change Management: The PR correctly identifies this as a breaking change due to the
sys_ioctlsignature modification. Ensuring dependent components (libphoenix, device servers) are updated is critical. - Documentation Update Required: As noted in the PR description, documentation for the
ioctlsyscall needs to be updated to reflect the newsizeargument. This was not addressed in this PR.
Merge Readiness
The code changes in this pull request are well-implemented for the kernel-side modifications and address the intended goal of serializing special ioctls in userspace. The addition of the break; statement in posix_ioctl is a good improvement for maintainability.
However, before this PR can be merged, the following should be considered:
- Dependent Changes: As stated, this PR requires corresponding changes in
libphoenixand potentially device server implementations. These should ideally be coordinated. - Documentation: The syscall documentation needs to be updated to reflect the new
sys_ioctlsignature. While this might be handled in a separate PR, it's a necessary follow-up.
Given these points, and particularly the dependency on other PRs for full functionality, I would recommend ensuring those related changes are ready or planned. The code itself within this PR appears to be of good quality. I am not authorized to approve pull requests, so further review and approval by authorized maintainers are necessary.
agkaminski
left a comment
agkaminski
left a comment
There was a problem hiding this comment.
LGTM, would like @Darchiv to approve
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
from
c9be3a9 to
ee8f74c
Compare
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
2 times, most recently
from
aaa469e to
53729d1
Compare
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
from
53729d1 to
78e1d5c
Compare
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
from
78e1d5c to
615cb9b
Compare
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
from
615cb9b to
613d51a
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
2 times, most recently
from
56da725 to
77a3310
Compare
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
5 times, most recently
from
cda275f to
57e992f
Compare
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
from
57e992f to
ccd1654
Compare
Unit Test Results1 377 tests - 7 900 841 ✅ - 7 847 8m 48s ⏱️ - 47m 18s For more details on these failures, see this check. Results for commit ccd1654. ± Comparison against base commit e4f9f90. This pull request removes 8131 and adds 231 tests. Note that renamed tests count towards both. |
julianuziemblo
changed the title
For the kernel, the ioctl syscall data should be transparent, so special ioctls (e.g. ones containing a pointer in the input structure) should be serialized/deserialized in userspace. JIRA: RTOS-1014
julianuziemblo
force-pushed
the
julianuziemblo/implement-siocethtool
branch
from
ccd1654 to
c4884cd
Compare
Description
For the kernel, the ioctl syscall data should be transparent, so special ioctls (e.g. ones containing a pointer in the input structure) should be serialized/deserialized in userspace. Additional size parameter is passed to allow for not-const sized ioctl payloads (e.g. for SIOCGIFCONF).
Related (serialization in userspace): phoenix-rtos/libphoenix#414
Motivation and Context
Types of changes
int sys_ioctl(int fildes, unsigned long request, void *val, size_t size)This was done to pass the dynamic size of the whole structure to the kernel without affecting the
requestvariable, 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.How Has This Been Tested?
armv7m7-imxrt106x-evk,ia32-generic-qemuChecklist:
Special treatment