Skip to content

hooks: allow ignoring chdir permission errors for container hooks - #2035

Merged
kolyshkin merged 1 commit into
containers:mainfrom
giuseppe:ignore-chdir-errors-hooks
Mar 4, 2026
Merged

hooks: allow ignoring chdir permission errors for container hooks#2035
kolyshkin merged 1 commit into
containers:mainfrom
giuseppe:ignore-chdir-errors-hooks

Conversation

@giuseppe

@giuseppe giuseppe commented Mar 3, 2026

Copy link
Copy Markdown
Member

When running containers with user namespaces (--userns nomap/auto), createContainer and startContainer hooks may fail with EACCES/EPERM when attempting to chdir to directories they don't have permission to access due to user namespace restrictions. This change adds a can_ignore_chdir_errors flag that allows these specific permission errors to be ignored for hooks that run in the container namespace context, while maintaining strict error handling for all other cases.

Fixes issue with NVIDIA CDI hooks and other container hooks failing in user namespace scenarios.

@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello, 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!

This pull request enhances the robustness of container hook execution, especially within user namespaces. It addresses issues where createContainer and startContainer hooks might fail due to EACCES or EPERM errors when attempting to change directories to paths they lack permission to access. By introducing a mechanism to conditionally ignore these specific permission errors for certain hooks, the change ensures smoother operation and compatibility with tools like NVIDIA CDI, without compromising strict error handling for other critical operations.

Highlights

  • Conditional chdir error ignoring: Introduced a can_ignore_chdir_errors flag to allow chdir permission errors (EACCES, EPERM) to be ignored under specific circumstances, particularly for container hooks running in user namespaces.
  • Hook-specific error handling: The createContainer and startContainer hooks are now configured to ignore chdir permission errors, while other hooks like poststop, prestart, create_runtime, and poststart maintain strict error handling.
  • Function signature updates: Modified the signatures of do_hooks, run_process_child, and run_process_with_stdin_timeout_envp functions to propagate the new can_ignore_chdir_errors flag.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • src/libcrun/container.c
    • Updated the do_hooks function signature to accept a new can_ignore_chdir_errors boolean parameter.
    • Modified calls to run_process_with_stdin_timeout_envp within do_hooks to pass the new error-ignoring flag.
    • Set can_ignore_chdir_errors to true for create_container and start_container hooks.
    • Set can_ignore_chdir_errors to false for poststop, prestart, create_runtime, and poststart hooks.
  • src/libcrun/utils.c
    • Modified the run_process_child function signature to include a can_ignore_chdir_errors parameter.
    • Implemented conditional logic within run_process_child to ignore chdir errors (EACCES or EPERM) if can_ignore_chdir_errors is true.
    • Updated the run_process_with_stdin_timeout_envp function signature to accept the can_ignore_chdir_errors flag.
    • Passed the can_ignore_chdir_errors flag from run_process_with_stdin_timeout_envp to run_process_child.
  • src/libcrun/utils.h
    • Updated the function prototype for run_process_with_stdin_timeout_envp to include the new can_ignore_chdir_errors boolean parameter.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

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.

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 gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a can_ignore_chdir_errors flag to handle chdir permission errors for container hooks, which can occur when using user namespaces. The implementation correctly propagates this flag and enables it only for createContainer and startContainer hooks, which run within the container's namespace context. The logic to ignore only EACCES and EPERM errors is also correct. I have one suggestion to refactor a conditional block for better readability.

Comment thread src/libcrun/utils.c
Comment on lines 1667 to +1675
if (cwd && chdir (cwd) < 0)
_safe_exit (EXIT_FAILURE);
{
if (can_ignore_chdir_errors && (errno == EACCES || errno == EPERM))
{
/* Permission errors for chdir are ignored for container hooks that run in user namespace */
}
else
_safe_exit (EXIT_FAILURE);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This logic is correct, but the empty if block can be avoided by inverting the condition. This can make the code more concise and arguably clearer about the intent to only act on the failure case.

  if (cwd && chdir (cwd) < 0)
    {
      // For some hooks running in user namespaces, chdir can fail with permission errors.
      // If can_ignore_chdir_errors is set, we ignore EACCES and EPERM.
      if (!can_ignore_chdir_errors || (errno != EACCES && errno != EPERM))
        _safe_exit (EXIT_FAILURE);
    }

@packit-as-a-service

Copy link
Copy Markdown

TMT tests failed. @containers/packit-build please check.

1 similar comment
@packit-as-a-service

Copy link
Copy Markdown

TMT tests failed. @containers/packit-build please check.

@giuseppe
giuseppe force-pushed the ignore-chdir-errors-hooks branch from 68259d3 to 82c25d9 Compare March 3, 2026 15:54

@kolyshkin kolyshkin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it looks like we want to ignore chdir failures in those hooks that are run inside container. I might be wrong here but maybe it makes sense to s/can_ignore_chdir_errors/in_container/g (or some such) for clarity and overall generalization (and fix comments accordingly).

Also, do we need to add a check if we're running inside userns?

Comment thread src/libcrun/utils.c Outdated
_safe_exit (EXIT_FAILURE);
{
/* For some hooks running in user namespaces, chdir can fail with permission errors.
If can_ignore_chdir_errors is set, we ignore EACCES and EPERM. */

@kolyshkin kolyshkin Mar 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems redundant, I'd remove it. It is very clear from the code that we ignore EACCES and EPERM if can_ignore_chdir_errors is set.

@giuseppe
giuseppe force-pushed the ignore-chdir-errors-hooks branch from 82c25d9 to b8e0a48 Compare March 4, 2026 10:19
@giuseppe

giuseppe commented Mar 4, 2026

Copy link
Copy Markdown
Member Author

So it looks like we want to ignore chdir failures in those hooks that are run inside container. I might be wrong here but maybe it makes sense to s/can_ignore_chdir_errors/in_container/g (or some such) for clarity and overall generalization (and fix comments accordingly).

I've changed the check to be used only when running in a user namespace so it is limited to that case

@giuseppe
giuseppe force-pushed the ignore-chdir-errors-hooks branch from b8e0a48 to 4ac5bc1 Compare March 4, 2026 10:30

@kolyshkin kolyshkin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin

Copy link
Copy Markdown
Collaborator

@giuseppe alas this needs a rebase after #2038 merge

When running containers with user namespaces (--userns nomap/auto),
createContainer and startContainer hooks may fail with EACCES/EPERM
when attempting to chdir to directories they don't have permission to
access due to user namespace restrictions. This change adds a
can_ignore_chdir_errors flag that allows these specific permission
errors to be ignored for hooks that run in the container namespace
context, while maintaining strict error handling for all other cases.

Fixes issue with NVIDIA CDI hooks and other container hooks failing
in user namespace scenarios.

Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
giuseppe force-pushed the ignore-chdir-errors-hooks branch from 4ac5bc1 to 3414373 Compare March 4, 2026 21:34
@giuseppe

giuseppe commented Mar 4, 2026

Copy link
Copy Markdown
Member Author

rebased!

@kolyshkin
kolyshkin merged commit a259f70 into containers:main Mar 4, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants