Skip to content

dhcpv4: support socket creation in netns - #79

Closed
blackdragoon26 wants to merge 1 commit into
nispor:mainfrom
blackdragoon26:netns-dhcp-sockets
Closed

dhcpv4: support socket creation in netns#79
blackdragoon26 wants to merge 1 commit into
nispor:mainfrom
blackdragoon26:netns-dhcp-sockets

Conversation

@blackdragoon26

Copy link
Copy Markdown

This is a dependency PR for netavark work here:
containers/netavark#1307

This adds support for opening DHCPv4 sockets inside a target network namespace
without moving the higher-level async DHCP flow into that namespace.

Summary:

  • add an optional socket netns path to DhcpV4Config
  • switch into the target netns only while creating/binding DHCP sockets
  • restore the original netns before returning to async code
  • use the same scoped handling for raw sockets and UDP renewal/release sockets

Why:
Netavark needs DHCP traffic for unmanaged bridge VLAN setups to originate from
the target network namespace, but doing that in Netavark by spawning a dedicated
Tokio runtime per container is the wrong layer and does not scale well.

This keeps the namespace-sensitive part limited to socket open/bind time and
lets callers keep their normal async/task model.

@blackdragoon26

Copy link
Copy Markdown
Author

@cathay4t Can you review this PR.
Thanks

@cathay4t cathay4t left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please also provide integration test case for this feature, ideally example code also.

Comment thread src/dhcpv4/socket.rs Outdated
Comment thread src/dhcpv4/config.rs Outdated
Comment thread src/dhcpv4/socket.rs
@cathay4t

Copy link
Copy Markdown
Contributor

Copilot also suggest this changes which I find reasonable:

diff --git a/src/dhcpv4/socket.rs b/src/dhcpv4/socket.rs
index 4f2ba9e..4fad44c 100644
--- a/src/dhcpv4/socket.rs
+++ b/src/dhcpv4/socket.rs
@@ -4,6 +4,7 @@ use std::{
     ffi::CString,
     fs::File,
     future::Future,
+    marker::PhantomData,
     net::{Ipv4Addr, UdpSocket as StdUdpSocket},
     os::{
         fd::{AsFd, AsRawFd, OwnedFd},
@@ -327,13 +328,20 @@ fn bind_socket_to_iface(fd: RawFd, iface_name: &str) -> Result<(), DhcpError> {

 struct NetnsGuard {
     host_netns: Option<File>,
+    // Prevent Send so this guard cannot be held across await points.
+    // setns() only affects the calling thread; moving across threads would
+    // silently switch the wrong thread's namespace.
+    _not_send: PhantomData<*const ()>,
 }

 impl NetnsGuard {
     // This switches only the current thread and must not be held across await.
     fn enter(netns_path: Option<&str>) -> Result<Self, DhcpError> {
         let Some(netns_path) = netns_path else {
-            return Ok(Self { host_netns: None });
+            return Ok(Self {
+                host_netns: None,
+                _not_send: PhantomData,
+            });
         };

         let host_netns = File::open("/proc/self/ns/net").map_err(|e| {
@@ -362,6 +370,7 @@ impl NetnsGuard {

         Ok(Self {
             host_netns: Some(host_netns),
+            _not_send: PhantomData,
         })
     }
 }

@blackdragoon26

Copy link
Copy Markdown
Author

Copilot also suggest this changes which I find reasonable:

diff --git a/src/dhcpv4/socket.rs b/src/dhcpv4/socket.rs
index 4f2ba9e..4fad44c 100644
--- a/src/dhcpv4/socket.rs
+++ b/src/dhcpv4/socket.rs
@@ -4,6 +4,7 @@ use std::{
     ffi::CString,
     fs::File,
     future::Future,
+    marker::PhantomData,
     net::{Ipv4Addr, UdpSocket as StdUdpSocket},
     os::{
         fd::{AsFd, AsRawFd, OwnedFd},
@@ -327,13 +328,20 @@ fn bind_socket_to_iface(fd: RawFd, iface_name: &str) -> Result<(), DhcpError> {

 struct NetnsGuard {
     host_netns: Option<File>,
+    // Prevent Send so this guard cannot be held across await points.
+    // setns() only affects the calling thread; moving across threads would
+    // silently switch the wrong thread's namespace.
+    _not_send: PhantomData<*const ()>,
 }

 impl NetnsGuard {
     // This switches only the current thread and must not be held across await.
     fn enter(netns_path: Option<&str>) -> Result<Self, DhcpError> {
         let Some(netns_path) = netns_path else {
-            return Ok(Self { host_netns: None });
+            return Ok(Self {
+                host_netns: None,
+                _not_send: PhantomData,
+            });
         };

         let host_netns = File::open("/proc/self/ns/net").map_err(|e| {
@@ -362,6 +370,7 @@ impl NetnsGuard {

         Ok(Self {
             host_netns: Some(host_netns),
+            _not_send: PhantomData,
         })
     }
 }

Done. I added the PhantomData<*const ()> marker so NetnsGuard is !Send.

The implementation is slightly adjusted from the suggested diff because the guard is now only constructed when socket_netns_path is Some, so NetnsGuard::enter takes the namespace path directly and stores the host netns File directly. The safety intent is the same: the guard cannot be sent across threads while setns() is active.

@blackdragoon26

Copy link
Copy Markdown
Author

@cathay4t I addressed the review comments and force-pushed the updated branch.

Changes made:

  • only create NetnsGuard when socket_netns_path is set
  • changed set_socket_netns_path() to accept Option
  • rebased on latest main after the libc::close() change
  • added PhantomData<*const ()> so NetnsGuard is !Send
  • added integration coverage for DHCPv4 proxy socket creation inside a target netns
  • added an example for the socket_netns_path flow

Validation completed:

  • real x86_64 OrbStack Linux VM:

    • cargo test test_dhcpv4_proxy_socket_netns -- --test-threads=1 --show-output
    • cargo test -- --test-threads=1 --show-output
    • cargo fmt --all -- --check
    • cargo clippy --all-targets --all-features -- -D warnings
  • privileged Linux arm64 Docker:

    • targeted netns test passed
    • full test suite passed
    • clippy passed

The x86_64 VM full test result was 13 tests passed plus 2 doctests passed.
Could you please take another look when you get time?
Thanks once again!

@blackdragoon26
blackdragoon26 requested a review from cathay4t April 29, 2026 03:36
@cathay4t

Copy link
Copy Markdown
Contributor

Please squash commits into single one. Thanks!

Please give me more time on reviewing.

@cathay4t

Copy link
Copy Markdown
Contributor

I have doubt on using File::open as NetnsGuard, need to consult with kernel network team.

Allow DHCPv4 callers to request socket creation inside a target network namespace without moving the async DHCP flow into that namespace.

The netns switch is scoped to socket open/bind, only runs when socket_netns_path is set, and uses a non-Send guard so the setns state cannot cross await points. Add integration coverage and an example for the netns socket path flow.
@blackdragoon26

Copy link
Copy Markdown
Author

Cool, squashed into a single commit.

Please take your time on the review.

and on the File::open concern, the intent is not that File itself is the guard logic, but that the RAII NetnsGuard owns an fd for /proc/self/ns/net so Drop can setns() back to the original namespace and then close the fd normally.

My understanding is that holding an fd to the namespace is the standard way to pin and restore it, but I agree this is subtle kernel behavior, so I am happy to adjust as the kernel network team recommends a different pattern, and based on their response learn new things.

Comment on lines +21 to +23
let mut config = DhcpV4Config::new_proxy(&args[1], &args[2])?;
config.set_iface_index(iface_index);
config.set_socket_netns_path(Some(args[4].clone()));

@cathay4t cathay4t May 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this example is wrong.

The iface_index is in host network namespace which does not exist in container's namespace. This socket binding will fail after we switched to container namespace.

The whole point of DHCP proxy in container usage is running DHCP process in host network namespace.

@cathay4t cathay4t left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please provide more detail on this unmanaged bridge VLAN setups use case.

And this tokio async runner in host namespace, but UDP socket in container namespace is weird (I don't even know how to describe why we need this use case to kernel network team).

@blackdragoon26

Copy link
Copy Markdown
Author

My bad @cathay4t ,
thanks for the review and for pushing back on the layering here.

After spending more time reproducing the original Netavark issue, I agree this mozim change is not the right direction.

The issue turned out to be specific to Netavark unmanaged bridge VLAN handling: for a network configured with mode=unmanaged and vlan=<id>, Netavark was asking the DHCP proxy to bind to the bridge itself. In a VLAN-filtered bridge setup, DHCP sourced from the bridge can go out through the bridge/default VLAN instead of the configured container VLAN.

I’m reworking the Netavark PR to fix this in Netavark by using a bridge VLAN interface for DHCP traffic, without changing mozim’s socket/netns behavior or adding this API.

Closing this PR to avoid adding an unnecessary mozim feature.

Thanks again for the guidance and pointing the issue out!

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