mount sysfs in namespace

[gabibeyer]

mount sysfs so the process running in the container
can see the correct /sys/class/net entries

Signed-off-by: Gabi Beyer [email protected]

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gabibeyer
To complete the pull request process, please assign jwhonce
You can assign the PR to them by writing /assign @jwhonce in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

Hi @gabibeyer. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rhatdan]

/ok-to-test
Thanks @gabibeyer
@mheon @giuseppe PTAL

[mheon]

Hmm. If we just need this in the container, shouldn't we add the mount in the OCI spec?

…

On Fri, Aug 16, 2019, 06:57 Daniel J Walsh ***@***.***> wrote: /ok-to-test Thanks @gabibeyer <https://github.com/gabibeyer> @mheon <https://github.com/mheon> @giuseppe <https://github.com/giuseppe> PTAL — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3831>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB3AOCB5GUN5PEUYH5QV5Q3QE2BZFANCNFSM4IMCX4XQ> .

[amshinde]

@mheon This is something that may be required for the OCI runtime itself, not just the container.
We see that this is currently handled with rootlesskit for rootless docker, would be nice if this is behaviour is implemented in podman as well.

[giuseppe]

The goroutine is running in a new network namespace but it is sharing the mount namespace with the host. How is the new mount visible from the container?

[gabibeyer]

I may be very off in my understanding of namespaces, but by default doesn't podman create a user namespace that all containers share (unless explicitly stated). The network namespace is being created inside that user namespace, and since mount namespaces require an owner user namespace all that information would be accessible by the container if it enters the network namespace?

[gabibeyer]

blah disregard that. I think what's happening is that it is being mounted correctly, but it needs to do a mount sooner, right after the network namespace creation in order to the /sys/class/net to be generated correctly for the oci runtime to use.

[amshinde]

@gabibeyer Perhaps, the commit message needs to be rephrased a bit, to specify that the OCI runtime needs to access the /sys.
@giuseppe We need this behaviour for the runtime. In case of Kata, we need to inspect the network namespace to get the type of network interfaces created in order to connect them to the virtual machine.
Most of this information can be obtained from netlink API.
But we may need to get this from sysfs in some cases. In case of tap interfaces, we do not get the information whether a tuntap device is actually "tap" or "tun" on older kernels from netlink. In that case we need to fallback to the sysfs, and expect the sysfs to be remounted to reflect the correct /sys/class/net information corresponding to the new network namespace created by podman.

I hope that makes things a bit clearer. A related PR submitted to netlink:
vishvananda/netlink@db99c04

[rh-atomic-bot]

☔ The latest upstream changes (presumably #2940) made this pull request unmergeable. Please resolve the merge conflicts.

[giuseppe]

@gabibeyer is it still needed?

[gabibeyer]

@giuseppe I believe so. @amshinde @marcov would you happen to know more?

[giuseppe]

do we have an example of issue we are trying to solve with this PR? Still it is not clear to me how mounting /sys again can make any difference. Don't we also leak a mount each time the code runs?

[marcov]

@amshinde explained here why access to sysfs is needed, #3831 (comment)

@gabibeyer: is this something specific for rootless?

[amshinde]

@giuseppe As @marcov mentioned, I have added an explanation in an earlier comment.
@marcov This isnt specific to rootless as such, its generally a good idea to have the sysfs remounted for a new network namespace, but the tap interfaces created in case of rootless makes this a requirement.

[marcov]

Got it @amshinde, then maybe you could consider only bind mounting the path necessary to look up the tap interfaces, and not the whole /sys

[giuseppe]

@giuseppe As @marcov mentioned, I have added an explanation in an earlier comment.

@amshinde thanks, I've read that comment but it is still not clear for me, isn't the mount namespace at this point the same as on the host? Isn't /sys already mounted there?

Is it needed for the rootless case?

[amshinde]

Let me see if I can explain this better.

I've read that comment but it is still not clear for me, isn't the mount namespace at this point the same as on the host? Isn't /sys already mounted there?

@giuseppe Yes, the mount namespace is the same on the host, /sys is already mounted there, but /sys is not tied to mount namespaces. The /sys on entering the new network namespace still points to the host network namespace.

A little background on the issue: https://lwn.net/Articles/295587/
Any sysfs directory can be associated with (at most) one type of tag, where that type identifies which type of namespace controls how that directory is viewed. Thus, for example, /sys/class/net would have a tag type identifying the network namespace subsystem as the one which is in control there.
When podman creates the network namespace, once Kata runtime enters the network namespace to inspect it, the sysfs entries in /sys/class/net still refer to the network interfaces in the host network namespace.

An issue that describes this in some more detail:
https://unix.stackexchange.com/questions/457025/switching-into-a-network-namespace-does-not-change-sys-class-net

I see that a similar project rootless-kit addressing this here:
https://github.com/rootless-containers/rootlesskit/blob/2cb5ec5819fddecb93aeea843ecdf29d6ecaf9af/pkg/child/child.go#L131

Let me know if you have any further questions, maybe we can discuss this on the crio call this week if you are on it.

[amshinde]

@giuseppe @mheon Any updates on this PR?

[giuseppe]

@giuseppe @mheon Any updates on this PR?

I've verified it locally and it creates a new RDONLY /sys mount on the host . I don't think that should happen.

If /sys is not already mounted, we should probably raise an error and ask the users to fix it.

[giuseppe]

@gabibeyer @amshinde is this patch still needed? If you'd like I can help here, can we discuss the issue you are having?

[giuseppe]

as there is no progress, I am closing the PR. Please re-open if it is still needed