Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--mount type=glob does not preserve symlinks #20098

Closed
vrothberg opened this issue Sep 22, 2023 · 15 comments · Fixed by #20356
Closed

--mount type=glob does not preserve symlinks #20098

vrothberg opened this issue Sep 22, 2023 · 15 comments · Fixed by #20356
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. stale-issue

Comments

@vrothberg
Copy link
Member

Broken out of #20000:

Here's an example.

I'm using this line in my module file to do the glob mount:

"type=glob,source=/usr/lib64/libcuda*,destination=/usr/lib64,ro",

Here are our target libraries on the host system:

stephey@perlmutter:login24:/usr/lib64> ls -la | grep libcuda
lrwxrwxrwx  1 root root        29 Mar 28 20:44 libcudadebugger.so.1 -> libcudadebugger.so.525.105.17
-rwxr-xr-x  1 root root  10490248 Mar 28 20:44 libcudadebugger.so.525.105.17
lrwxrwxrwx  1 root root        12 Mar 28 20:44 libcuda.so -> libcuda.so.1
lrwxrwxrwx  1 root root        21 Mar 28 20:44 libcuda.so.1 -> libcuda.so.525.105.17
-rwxr-xr-x  1 root root  29867944 Mar 28 20:44 libcuda.so.525.105.17
stephey@perlmutter:login24:/usr/lib64> 

Here are the mounted libraries inside the container running with my gpu module:

root@8dd9e54b936a:/usr/lib64# ls -la | grep libcuda
-rwxr-xr-x  1 nobody nogroup  29867944 Mar 29 03:44 libcuda.so
-rwxr-xr-x  1 nobody nogroup  29867944 Mar 29 03:44 libcuda.so.1
-rwxr-xr-x  1 nobody nogroup  29867944 Mar 29 03:44 libcuda.so.525.105.17
-rwxr-xr-x  1 nobody nogroup  10490248 Mar 29 03:44 libcudadebugger.so.1
-rwxr-xr-x  1 nobody nogroup  10490248 Mar 29 03:44 libcudadebugger.so.525.105.17
root@8dd9e54b936a:/usr/lib64# 

We'd like the existing symlinks to be preserved.

Originally posted by @lastephey in #20000 (comment)

@vrothberg
Copy link
Member Author

Assigning to @giuseppe who has some ideas on how to preserve symlinks.

@giuseppe
Copy link
Member

my comment was unclear, but I don't think we need to change the semantics of how mounts work.

If we want to preserve symlinks we need a "copy" option.

In any case, I am not able to look into it in the near future, so unassigning myself.

@giuseppe giuseppe removed their assignment Sep 27, 2023
@vrothberg
Copy link
Member Author

@giuseppe, can you elaborate on the "copy" idea? Maybe somebody else can tackle the issue but I want to make sure it's clear what to do.

So in addition to --mount you envision a --copy option that wouldn't mount host data into the container mount NS but create actual copies?

@giuseppe
Copy link
Member

the mount semantic in Linux is to always follow symlinks for the source, and it makes sense somehow since the symlink could point to a path not present in the new namespace as well as anyway we still need to create the symlink inode before mounting on top of it, so it makes sense to just create it with the right content.

Maybe it could just be an option for the mount command, e.g. "type=glob,copy-symlinks,source=/usr/lib64/libcuda*,destination=/usr/lib64,ro"

and when Podman encounters it, it creates the symlink inside the rootfs inside of adding a mount for the OCI runtime.

This must be handled directly from Podman, there is no need to extend the OCI runtime for doing the copy IMO.

@vrothberg
Copy link
Member Author

and when Podman encounters it, it creates the symlink inside the rootfs inside of adding a mount for the OCI runtime.

That sounds fairly straight-forward. I'd even vote to always re-create symlinks for type=glob to prevent users from having to deal with potentially (very) subtle errors coming from broken symlinks.

@rhatdan
Copy link
Member

rhatdan commented Sep 29, 2023

I agree.

@vrothberg
Copy link
Member Author

I had another chat with @giuseppe on this issue. We settled on adding a new mount option that will work for all mount types, not only the glob one.

@vrothberg
Copy link
Member Author

@rhatdan @giuseppe - the below tests reveals some behavior I don't yet grasp. I somehow expected --mount type=bind,src=$dir/link,dst=/tmp/link to behave the same as when glob'ing the same link. But it doesn't. With --type=bind, the linked file is copied. With glob not. Shouldn't it behave the same?

diff --git a/test/system/060-mount.bats b/test/system/060-mount.bats
index 4452a5aa59cf..b4afacb5b6aa 100644
--- a/test/system/060-mount.bats
+++ b/test/system/060-mount.bats
@@ -308,4 +308,32 @@ EOF
     fi
 }
 
+@test "podman mount preserve symlinks" {
+    dir=$PODMAN_TMPDIR/sub-dir
+    data_file=$PODMAN_TMPDIR/sub-dir/data
+    link=$PODMAN_TMPDIR/sub-dir/link
+
+    mkdir $dir
+    random_host="on_the_host_$(random_string 15)"
+    random_img="on_the_image_$(random_string 15)"
+    echo "$random_host" > $data_file
+    ln -s $data_file $link
+
+    dockerfile=$PODMAN_TMPDIR/Dockerfile
+    cat >$dockerfile <<EOF
+FROM $IMAGE
+RUN echo $random_img > /tmp/data
+EOF
+
+    img="localhost/preserve:symlinks"
+    run_podman build -t $img -f $dockerfile
+
+    run_podman run --mount type=bind,src=$dir/link,dst=/tmp/link --rm --privileged $img cat /tmp/link
+    assert "$output" = "$random_host" "by default, symlink is resolved on the host"
+    run_podman run --mount type=glob,src=$dir/lin*,dst=/tmp/link --rm --privileged $img cat /tmp/link
+    assert "$output" = "$random_host" "by default, symlink is resolved on the host"
+
+    run_podman rmi -f $img
+}
+

@edsantiago
Copy link
Collaborator

The failure I see is cat: read error: Is a directory

Replacing cat /tmp/link with ls /tmp shows that container:/tmp/link is a directory containing a file named link.

Does that help?

@vrothberg
Copy link
Member Author

Thanks, @edsantiago. I will continue digging. Something seems off/broken in the glob mounts.

@edsantiago
Copy link
Collaborator

The behavior I'm seeing looks 100% correct. The simplest way I can describe it is, assuming this:

...type=glob,src=$dir/lin*,dst=/tmp/link

You cannot just mount file $dir/link as file /tmp/link. That would be evil and unreliable and inconsistent, because imagine these situations:

  • There exists one file, $dir/link
  • There exist two files, $dir/link1 and $dir/link2
  • ..... one file $dir/link, one directory $dir/lint
  • no matches for $dir/lin*

Does that make sense?

@vrothberg
Copy link
Member Author

makes perfect sense, thanks Ed!

Juggling too many things atm.

@rhatdan
Copy link
Member

rhatdan commented Oct 10, 2023

GLOB and direct path should be exactly the same.

@vrothberg
Copy link
Member Author

@rhatdan agreed. The confusion was on my end.

Ed's explanation makes quite some sense. If I have /dir/file1 and /dir/file2 etc. and then glob mount with src=/dir/file*,dest=/tmp/file ... dest=/tmp/file cannot be a file but must be directory. So it's just a fart in my test.

vrothberg added a commit to vrothberg/libpod that referenced this issue Oct 13, 2023
WIP

Fixes: containers#20098
Fixes: issues.redhat.com/browse/RUN-1935
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
vrothberg added a commit to vrothberg/libpod that referenced this issue Oct 18, 2023
WIP

Fixes: containers#20098
Fixes: issues.redhat.com/browse/RUN-1935
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
Copy link

A friendly reminder that this issue had no activity for 30 days.

vrothberg added a commit to vrothberg/libpod that referenced this issue Nov 20, 2023
Add a new copy-symlink mount option supported by crun 1.11+ to
re-create/copy a symlink if it's the source of a mount.  By default the
kernel will resolve the symlink on the host and mount the target.
As reported in containers#20098, there are use cases where the symlink structure
must be preserved by all means.

Fixes: containers#20098
Fixes: issues.redhat.com/browse/RUN-1935
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
vrothberg added a commit to vrothberg/libpod that referenced this issue Nov 20, 2023
Add a new copy-symlink mount option supported by crun 1.11+ to
re-create/copy a symlink if it's the source of a mount.  By default the
kernel will resolve the symlink on the host and mount the target.
As reported in containers#20098, there are use cases where the symlink structure
must be preserved by all means.

Fixes: containers#20098
Fixes: issues.redhat.com/browse/RUN-1935
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
vrothberg added a commit to vrothberg/libpod that referenced this issue Nov 20, 2023
Add a new copy-symlink mount option supported by crun 1.11+ to
re-create/copy a symlink if it's the source of a mount.  By default the
kernel will resolve the symlink on the host and mount the target.
As reported in containers#20098, there are use cases where the symlink structure
must be preserved by all means.

Fixes: containers#20098
Fixes: issues.redhat.com/browse/RUN-1935
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
vrothberg added a commit to vrothberg/libpod that referenced this issue Nov 21, 2023
Add a new `no-dereference` mount option supported by crun 1.11+ to
re-create/copy a symlink if it's the source of a mount.  By default the
kernel will resolve the symlink on the host and mount the target.
As reported in containers#20098, there are use cases where the symlink structure
must be preserved by all means.

Fixes: containers#20098
Fixes: issues.redhat.com/browse/RUN-1935
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Feb 20, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. stale-issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants