lib/9pfs: Return ENOTSUP on ioctl requests except for FIONBIO #1098

StefanJum · 2023-09-16T11:15:22Z

Currently 9pfs returns 0 for any ioctl request (except for TIOCGWINSZ), which leads to multiple cases of files being interpreted as terminals. You can see this problem when running python scripts, since python uses ioctl calls to check if it should open up the interactive interpretor or execute a script.

Same thing could happen for other applications that rely on ioctl operations being properly executed, so the best way to go would be return EINVAL (as is used to happen before commit 28d0edf).

If the ioctl call always return EINVAL, the Ruby binary compatibility application will fail, since it tries to set O_ASYNC and breaks on error, even if it does not directly depends on that, so for now add a special case for that.

So as an overview:

current implementation (staging branch) -> binary compatibility python3 scripts (using glibc as the libc, since the musl case is covered by the TIOCGWINSZ special case) fails, since it thinks it runs inside a terminal.
The same could happen for any application that expects the ioctl request to be properly handeled.
ruby works in binary compatibility mode.
ioctl always returns EINVAL (as it should, since we do not handle the ioctl requests) -> python3 scripts work fine, ruby fails in binary compatibility mode, since it tries to set O_ASYNC (even if it works fine without it).
the implementation in this PR -> both python3 and ruby work fine, nothing else seems to currently break, even if adding the special case for FIONBIO is very hackish.

Prerequisite checklist

Read the contribution guidelines regarding submitting new changes to the project;
Tested your changes against relevant architectures and platforms;
Ran the checkpatch.uk on your commit series before opening this PR;
Updated relevant documentation.

Base target

Architecture(s): N/A
Platform(s): N/A
Application(s): python3 bincompat, ruby bincompat

Currently `9pfs` returns 0 for any `ioctl` request (except for `TIOCGWINSZ`), which leads to multiple cases of files being interpreted as terminals. You can see this problem when running python scripts, since python uses `ioctl` calls to check if it should open up the interactive interpretor or execute a script. Same thing could happen for other applications that rely on `ioctl` operations being properly executed, so the best way to go would be return `ENOTSUP` (as is used to happen before commit 28d0edf). If the `ioctl` call always return `ENOTSUP`, the Ruby binary compatibility application will fail, since it tries to set O_ASYNC and breaks on error, even if it does not directly depends on that, so for now add a special case for that. Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>

StefanJum · 2023-09-20T20:44:46Z

Updated as discussed in the application compatibility meeting @razvand @skuenzer

lib/9pfs/9pfs_vnops.c

razvand · 2023-09-27T04:40:06Z

Thanks, @StefanJum . This looks OK to me. I'm waiting for the final review, accept from @skuenzer and then I'll add my review tag.

lib/9pfs/9pfs_vnops.c

lib/vfscore/include/vfscore/vnode.h

razvand · 2023-10-02T06:32:41Z

@StefanJum , @mogasergiu , this looks fine with me. I'll wait for @skuenzer's confirmation that this is OK and then I'll add my reviewer tag.

mogasergiu

I would also mention the fact that you introduced the new IOCTL macro's in the commit message. But other than that, LGTM! 👍

Reviewed-by: Sergiu Moga sergiu@unikraft.io

StefanJum · 2023-10-03T13:31:31Z

Added the macro description in the commit message.

lib/vfscore/include/vfscore/vnode.h

Some applications (e.g. python) will use `ioctl()` calls to check if an interactive interpretor should be started or if a file should be read. `9pfs` should respond to all terminal-related calls with an `ENOTTY` error, so applications know that they are not running in an interactive environment. In order to detect the ioctl request type, add a new macro, `IOCTL_CMD_ISTYPE(cmd, type)`, which will check the corresponding bytes from the request number (an enhanced version of the Linux `_IOC_TYPE`). Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>

mogasergiu · 2023-10-03T14:09:20Z

lib/vfscore/include/vfscore/vnode.h

+#define IOCTL_CMD_ISTYPE(cmd, type)			\
+		((cmd & (IOCTL_CMD_TYPE_MASK)) ==	\
+		 (((type) << IOCTL_CMD_TYPE_SHIFT) &	\
+		  IOCTL_CMD_TYPE_MASK))


Bellísimo 💅👌

skuenzer

Thanks for your work.

Approved-by: Simon Kuenzer simon@unikraft.io

Currently `9pfs` returns 0 for any `ioctl` request (except for `TIOCGWINSZ`), which leads to multiple cases of files being interpreted as terminals. You can see this problem when running python scripts, since python uses `ioctl` calls to check if it should open up the interactive interpretor or execute a script. Same thing could happen for other applications that rely on `ioctl` operations being properly executed, so the best way to go would be return `ENOTSUP` (as is used to happen before commit 28d0edf). If the `ioctl` call always return `ENOTSUP`, the Ruby binary compatibility application will fail, since it tries to set O_ASYNC and breaks on error, even if it does not directly depends on that, so for now add a special case for that. Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Sergiu Moga <sergiu@unikraft.io> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #1098

Some applications (e.g. python) will use `ioctl()` calls to check if an interactive interpretor should be started or if a file should be read. `9pfs` should respond to all terminal-related calls with an `ENOTTY` error, so applications know that they are not running in an interactive environment. In order to detect the ioctl request type, add a new macro, `IOCTL_CMD_ISTYPE(cmd, type)`, which will check the corresponding bytes from the request number (an enhanced version of the Linux `_IOC_TYPE`). Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Sergiu Moga <sergiu@unikraft.io> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #1098

Currently `9pfs` returns 0 for any `ioctl` request (except for `TIOCGWINSZ`), which leads to multiple cases of files being interpreted as terminals. You can see this problem when running python scripts, since python uses `ioctl` calls to check if it should open up the interactive interpretor or execute a script. Same thing could happen for other applications that rely on `ioctl` operations being properly executed, so the best way to go would be return `ENOTSUP` (as is used to happen before commit 28d0edf). If the `ioctl` call always return `ENOTSUP`, the Ruby binary compatibility application will fail, since it tries to set O_ASYNC and breaks on error, even if it does not directly depends on that, so for now add a special case for that. Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Sergiu Moga <sergiu@unikraft.io> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #1098

Some applications (e.g. python) will use `ioctl()` calls to check if an interactive interpretor should be started or if a file should be read. `9pfs` should respond to all terminal-related calls with an `ENOTTY` error, so applications know that they are not running in an interactive environment. In order to detect the ioctl request type, add a new macro, `IOCTL_CMD_ISTYPE(cmd, type)`, which will check the corresponding bytes from the request number (an enhanced version of the Linux `_IOC_TYPE`). Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Sergiu Moga <sergiu@unikraft.io> Approved-by: Simon Kuenzer <simon@unikraft.io> GitHub-Closes: #1098

Currently ramfs returns EINVAL for any ioctl request, which leads to the Ruby binary compatibility application to fail, since it tries to set `O_ASYNC` and breaks on error, even if it does not directly depends on it being set. Add a special case to treat `O_ASYNC`, return `ENOTSUP` by default, and return `ENOTTY` on every `ioctl()` call related to a terminal. The implementation is similar to what we currently have in `lib/9pfs/`, introduced by unikraft/unikraft#1098 GitHub-Fixes: unikraft#1186 Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>

Currently ramfs returns EINVAL for any ioctl request, which leads to the Ruby binary compatibility application to fail, since it tries to set `O_ASYNC` and breaks on error, even if it does not directly depends on it being set. Add a special case to treat `O_ASYNC`, return `ENOTSUP` by default, and return `ENOTTY` on every `ioctl()` call related to a terminal. The implementation is similar to what we currently have in `lib/9pfs/`, introduced by unikraft#1098 GitHub-Fixes: unikraft#1186 Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>

Currently ramfs returns EINVAL for any ioctl request, which leads to the Ruby binary compatibility application to fail, since it tries to set `O_ASYNC` and breaks on error, even if it does not directly depends on it being set. Add a special case to treat `O_ASYNC`, return `ENOTSUP` by default, and return `ENOTTY` on every `ioctl()` call related to a terminal. The implementation is similar to what we currently have in `lib/9pfs/`, introduced by #1098 GitHub-Fixes: #1186 Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com>

Currently ramfs returns EINVAL for any ioctl request, which leads to the Ruby binary compatibility application to fail, since it tries to set `O_ASYNC` and breaks on error, even if it does not directly depends on it being set. Add a special case to treat `O_ASYNC`, return `ENOTSUP` by default, and return `ENOTTY` on every `ioctl()` call related to a terminal. The implementation is similar to what we currently have in `lib/9pfs/`, introduced by #1098 Signed-off-by: Stefan Jumarea <stefanjumarea02@gmail.com> Reviewed-by: Razvan Deaconescu <razvand@unikraft.io> Approved-by: Razvan Deaconescu <razvand@unikraft.io> GitHub-Fixes: #1186 GitHub-Closes: #1202

StefanJum added kind/bug Something isn't working kind/quick-fix Issue is a quick fix lib/9pfs 9p client bug/runtime Bug occurs during runtime labels Sep 16, 2023

StefanJum requested review from razvand and mogasergiu September 16, 2023 11:15

StefanJum requested a review from a team as a code owner September 16, 2023 11:15

StefanJum assigned marcrittinghaus Sep 16, 2023

StefanJum removed the request for review from a team September 16, 2023 11:15

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from efd0e7e to e068e51 Compare September 16, 2023 11:18

StefanJum changed the title ~~lib/9pfs: Return ENOTTY on ioctl tty requests~~ lib/9pfs: Return EINVAL on ioctl requests except for FIONBIO Sep 16, 2023

StefanJum force-pushed the StefanJum/add-ioctl-notty branch 2 times, most recently from fb43a70 to 524d0fb Compare September 20, 2023 20:40

StefanJum changed the title ~~lib/9pfs: Return EINVAL on ioctl requests except for FIONBIO~~ lib/9pfs: Return ENOTSUP on ioctl requests except for FIONBIO Sep 20, 2023

mogasergiu reviewed Sep 21, 2023

View reviewed changes

lib/9pfs/9pfs_vnops.c Outdated Show resolved Hide resolved

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from 524d0fb to dc15c69 Compare September 23, 2023 11:37

StefanJum requested a review from mogasergiu September 23, 2023 11:38

razvand assigned skuenzer and unassigned marcrittinghaus Sep 27, 2023

razvand added this to the v0.14.1 (Prometheus) milestone Sep 27, 2023

razvand requested a review from John-Ted September 27, 2023 04:52

mogasergiu reviewed Sep 27, 2023

View reviewed changes

lib/9pfs/9pfs_vnops.c Outdated Show resolved Hide resolved

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from dc15c69 to 0725c8e Compare September 27, 2023 16:12

StefanJum requested a review from mogasergiu September 27, 2023 16:16

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from 0725c8e to 34b4f9e Compare September 27, 2023 17:13

skuenzer reviewed Sep 28, 2023

View reviewed changes

lib/9pfs/9pfs_vnops.c Outdated Show resolved Hide resolved

lib/vfscore/include/vfscore/vnode.h Outdated Show resolved Hide resolved

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from 34b4f9e to 19eb928 Compare September 29, 2023 04:38

StefanJum requested a review from skuenzer September 29, 2023 04:39

mogasergiu reviewed Sep 29, 2023

View reviewed changes

lib/vfscore/include/vfscore/vnode.h Outdated Show resolved Hide resolved

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from 19eb928 to f5a8dcc Compare October 1, 2023 17:18

StefanJum requested a review from mogasergiu October 1, 2023 17:19

mogasergiu approved these changes Oct 2, 2023

View reviewed changes

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from f5a8dcc to 4b78fd0 Compare October 3, 2023 13:31

mogasergiu reviewed Oct 3, 2023

View reviewed changes

lib/vfscore/include/vfscore/vnode.h Outdated Show resolved Hide resolved

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from 4b78fd0 to a70f09d Compare October 3, 2023 13:58

mogasergiu reviewed Oct 3, 2023

View reviewed changes

lib/vfscore/include/vfscore/vnode.h Outdated Show resolved Hide resolved

StefanJum force-pushed the StefanJum/add-ioctl-notty branch from a70f09d to b9f90d0 Compare October 3, 2023 14:07

mogasergiu reviewed Oct 3, 2023

View reviewed changes

skuenzer approved these changes Oct 4, 2023

View reviewed changes

razvand merged commit 383237c into unikraft:staging Oct 4, 2023
9 checks passed

razvand deleted the StefanJum/add-ioctl-notty branch October 4, 2023 23:17

nderjung modified the milestones: v0.14.1 (Prometheus), v0.15.0 (Pandora) Oct 23, 2023

StefanJum mentioned this pull request Dec 12, 2023

lib/ramfs: Return proper errors on ioctl calls #1202

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/9pfs: Return ENOTSUP on ioctl requests except for FIONBIO #1098

lib/9pfs: Return ENOTSUP on ioctl requests except for FIONBIO #1098

StefanJum commented Sep 16, 2023

StefanJum commented Sep 20, 2023

razvand commented Sep 27, 2023

razvand commented Oct 2, 2023

mogasergiu left a comment

StefanJum commented Oct 3, 2023

mogasergiu Oct 3, 2023

skuenzer Oct 4, 2023

skuenzer left a comment

lib/9pfs: Return ENOTSUP on ioctl requests except for FIONBIO #1098

lib/9pfs: Return ENOTSUP on ioctl requests except for FIONBIO #1098

Conversation

StefanJum commented Sep 16, 2023

Prerequisite checklist

Base target

StefanJum commented Sep 20, 2023

razvand commented Sep 27, 2023

razvand commented Oct 2, 2023

mogasergiu left a comment

Choose a reason for hiding this comment

StefanJum commented Oct 3, 2023

mogasergiu Oct 3, 2023

Choose a reason for hiding this comment

skuenzer Oct 4, 2023

Choose a reason for hiding this comment

skuenzer left a comment

Choose a reason for hiding this comment