Conversation
mz-pdm
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc,
slp and
tylerfanelli
as code owners
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing and mounting tmpfs filesystems defined in a JSON configuration. It introduces helper functions for JSON navigation and mount point validation. Feedback suggests enhancing the JSON parser's robustness, eliminating redundant helper functions in favor of existing ones, and ensuring error output is sent to stderr.
mz-pdm
force-pushed
the
pdm-tmpfs
branch
2 times, most recently
from
9f4cf40 to
44df048
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces logic to parse and mount tmpfs volumes from a configuration file in the init process. The changes include new helper functions for JSON parsing and mount point verification. Several critical issues were identified during the review: the JSON token indexing logic in config_parse_mounts is incorrect and could lead to crashes, the sscanf approach for parsing /proc/mounts fails to handle escaped characters, and there is a memory leak for the config_tmpfs string. Additionally, it is recommended to add the MS_NODEV flag when mounting the tmpfs for better security.
| config_parse_file(&config_argv, &config_workdir, &config_tmpfs, | ||
| config_file); | ||
|
|
||
| #if __FreeBSD__ | ||
| if (config_file_mounted) { | ||
| unmount_config_iso(); | ||
| } | ||
| #endif | ||
|
|
||
| if (config_tmpfs) { | ||
| if (mkdir(config_tmpfs, 0755) < 0 && errno != EEXIST) { | ||
| perror("mkdir for tmpfs"); | ||
| exit(-1); | ||
| } | ||
|
|
||
| /* TODO: Honour mount flags from the config file. Most notably, | ||
| * tmpcopyup is set by Podman by default, requesting copying the files | ||
| * present in the original directory, e.g. from the image. */ | ||
| if (mount("tmpfs", config_tmpfs, "tmpfs", | ||
| MS_NOEXEC | MS_NOSUID | MS_RELATIME, NULL) < 0) { | ||
| perror("mount for tmpfs"); | ||
| exit(-1); | ||
| } | ||
| } |
There was a problem hiding this comment.
The config_tmpfs string is allocated via malloc (inside config_parse_string called by config_parse_mounts) but is never freed in main. This results in a small memory leak.
References
- Ensure all dynamically allocated memory is properly freed to avoid leaks, especially in long-running processes or entry points.
There was a problem hiding this comment.
True but it is consistent with handling the other config_* variables passed to config_parse_file. Maybe we should free all of them (in a separate change)?
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for parsing and mounting tmpfs filesystems specified in the container configuration. It includes new utility functions for JSON token skipping and mount point verification via /proc/mounts. Review feedback highlights several areas for improvement: ensuring the 'mounts' JSON token is validated as an array, increasing buffer sizes for filesystem paths to avoid truncation, and refining the JSON parsing logic to handle complex values correctly using the new skip utility. There is also a suggestion to relax the validation of the 'source' field for tmpfs mounts to better align with OCI specifications.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to parse tmpfs mount specifications from a JSON configuration and perform the corresponding mount operations. It includes new logic for JSON token skipping and mount point detection via /proc/mounts. The review feedback suggests increasing buffer sizes for path handling to prevent truncation and improving the robustness of directory creation for nested paths.
Podman's --tmpfs command line option is currently ignored by libkrun. This means the host tmpfs, as arranged by crun, is auto-mounted as a virtiofs when first accessing the given directory. Having such a tmpfs adds some overhead when accessing it and reduces the VM isolation. Let's add a limited support for mount the given directory natively. This is implemented by looking at `mounts' in /.krun_config.json and mounting the first matching tmpfs directory there, if any. "Matching" means it's a tmpfs entry and the directory is not yet mounted. The mount options are ignored. We refuse to mount already mounted directories; this is because tmpfs mounts can be requested by Podman itself also for other directories, for example /dev, which libkrun already mounts itself. The mount check is implemented by looking at /proc/mounts; using stat/lstat would result in triggering the Podman's automount. The mount directory must already exist, which is the case with Podman's --tmpfs. This commit implements just the most basic easy part and is not fully compliant. Possible future improvements are: - Copy the files from the original directory to the tmpfs. This is requested by default from Podman (tmpcopyup option). It's not implemented right now because the copying is non-trivial and it's better to leave that for Rust implementation of init. - Create the mount directory if it doesn't exist. - Honour the other mount options. - Honour other mounts from the configuration. Fixes: containers#515 Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
2 participants
Podman's --tmpfs command line option is currently ignored by libkrun. This means the host tmpfs, as arranged by crun, is auto-mounted as a virtiofs when first accessing the given directory. Having such a tmpfs adds some overhead when accessing it and reduces the VM isolation. Let's add a limited support for mount the given directory natively.
This is implemented by looking at `mounts' in /.krun_config.json and mounting the first matching tmpfs directory there, if any. "Matching" means it's a tmpfs entry and the directory is not yet mounted. The mount options are ignored.
We refuse to mount already mounted directories; this is because tmpfs mounts can be requested by Podman itself also for other directories, for example /dev, which libkrun already mounts itself. The mount check is implemented by looking at /proc/mounts; using stat/lstat would result in triggering the Podman's automount.
The mount directory must already exist, which is the case with Podman's --tmpfs.
This commit implements just the most basic easy part and is not fully compliant. Possible future improvements are:
Copy the files from the original directory to the tmpfs. This is requested by default from Podman (tmpcopyup option). It's not implemented right now because the copying is non-trivial and it's better to leave that for Rust implementation of init.
Create the mount directory if it doesn't exist.
Honour the other mount options.
Honour other mounts from the configuration.
Fixes: #515