From 02abe00439ba873945bd6b60546a200b3da751f1 Mon Sep 17 00:00:00 2001 From: Dario Nieuwenhuis Date: Sat, 3 Dec 2022 00:56:16 +0100 Subject: [PATCH] net: don't use UnsafeCell. The "must not be called reentrantly" invariant is too "global" to maintain comfortably, and the cost of the RefCell is negligible, so this was a case of premature optimization. --- embassy-net/src/stack.rs | 32 ++++++++++------------ embassy-net/src/tcp.rs | 59 ++++++++++++++++++---------------------- embassy-net/src/udp.rs | 41 ++++++++++++---------------- 3 files changed, 57 insertions(+), 75 deletions(-) diff --git a/embassy-net/src/stack.rs b/embassy-net/src/stack.rs index 3a761075..63108740 100644 --- a/embassy-net/src/stack.rs +++ b/embassy-net/src/stack.rs @@ -1,4 +1,4 @@ -use core::cell::UnsafeCell; +use core::cell::RefCell; use core::future::{poll_fn, Future}; use core::task::{Context, Poll}; @@ -62,8 +62,8 @@ pub enum ConfigStrategy { } pub struct Stack { - pub(crate) socket: UnsafeCell, - inner: UnsafeCell>, + pub(crate) socket: RefCell, + inner: RefCell>, } struct Inner { @@ -81,8 +81,6 @@ pub(crate) struct SocketStack { next_local_port: u16, } -unsafe impl Send for Stack {} - impl Stack { pub fn new( device: D, @@ -143,40 +141,38 @@ impl Stack { } Self { - socket: UnsafeCell::new(socket), - inner: UnsafeCell::new(inner), + socket: RefCell::new(socket), + inner: RefCell::new(inner), } } - /// SAFETY: must not call reentrantly. - unsafe fn with(&self, f: impl FnOnce(&SocketStack, &Inner) -> R) -> R { - f(&*self.socket.get(), &*self.inner.get()) + fn with(&self, f: impl FnOnce(&SocketStack, &Inner) -> R) -> R { + f(&*self.socket.borrow(), &*self.inner.borrow()) } - /// SAFETY: must not call reentrantly. - unsafe fn with_mut(&self, f: impl FnOnce(&mut SocketStack, &mut Inner) -> R) -> R { - f(&mut *self.socket.get(), &mut *self.inner.get()) + fn with_mut(&self, f: impl FnOnce(&mut SocketStack, &mut Inner) -> R) -> R { + f(&mut *self.socket.borrow_mut(), &mut *self.inner.borrow_mut()) } pub fn ethernet_address(&self) -> [u8; 6] { - unsafe { self.with(|_s, i| i.device.device.ethernet_address()) } + self.with(|_s, i| i.device.device.ethernet_address()) } pub fn is_link_up(&self) -> bool { - unsafe { self.with(|_s, i| i.link_up) } + self.with(|_s, i| i.link_up) } pub fn is_config_up(&self) -> bool { - unsafe { self.with(|_s, i| i.config.is_some()) } + self.with(|_s, i| i.config.is_some()) } pub fn config(&self) -> Option { - unsafe { self.with(|_s, i| i.config.clone()) } + self.with(|_s, i| i.config.clone()) } pub async fn run(&self) -> ! { poll_fn(|cx| { - unsafe { self.with_mut(|s, i| i.poll(cx, s)) } + self.with_mut(|s, i| i.poll(cx, s)); Poll::<()>::Pending }) .await; diff --git a/embassy-net/src/tcp.rs b/embassy-net/src/tcp.rs index 85d9e5ee..60386535 100644 --- a/embassy-net/src/tcp.rs +++ b/embassy-net/src/tcp.rs @@ -1,4 +1,4 @@ -use core::cell::UnsafeCell; +use core::cell::RefCell; use core::future::poll_fn; use core::mem; use core::task::Poll; @@ -68,8 +68,7 @@ impl<'a> TcpWriter<'a> { impl<'a> TcpSocket<'a> { pub fn new(stack: &'a Stack, rx_buffer: &'a mut [u8], tx_buffer: &'a mut [u8]) -> Self { - // safety: not accessed reentrantly. - let s = unsafe { &mut *stack.socket.get() }; + let s = &mut *stack.socket.borrow_mut(); let rx_buffer: &'static mut [u8] = unsafe { mem::transmute(rx_buffer) }; let tx_buffer: &'static mut [u8] = unsafe { mem::transmute(tx_buffer) }; let handle = s.sockets.add(tcp::Socket::new( @@ -93,17 +92,15 @@ impl<'a> TcpSocket<'a> { where T: Into, { - // safety: not accessed reentrantly. - let local_port = unsafe { &mut *self.io.stack.get() }.get_local_port(); + let local_port = self.io.stack.borrow_mut().get_local_port(); - // safety: not accessed reentrantly. - match unsafe { self.io.with_mut(|s, i| s.connect(i, remote_endpoint, local_port)) } { + match { self.io.with_mut(|s, i| s.connect(i, remote_endpoint, local_port)) } { Ok(()) => {} Err(tcp::ConnectError::InvalidState) => return Err(ConnectError::InvalidState), Err(tcp::ConnectError::Unaddressable) => return Err(ConnectError::NoRoute), } - poll_fn(|cx| unsafe { + poll_fn(|cx| { self.io.with_mut(|s, _| match s.state() { tcp::State::Closed | tcp::State::TimeWait => Poll::Ready(Err(ConnectError::ConnectionReset)), tcp::State::Listen => unreachable!(), @@ -121,14 +118,13 @@ impl<'a> TcpSocket<'a> { where T: Into, { - // safety: not accessed reentrantly. - match unsafe { self.io.with_mut(|s, _| s.listen(local_endpoint)) } { + match self.io.with_mut(|s, _| s.listen(local_endpoint)) { Ok(()) => {} Err(tcp::ListenError::InvalidState) => return Err(AcceptError::InvalidState), Err(tcp::ListenError::Unaddressable) => return Err(AcceptError::InvalidPort), } - poll_fn(|cx| unsafe { + poll_fn(|cx| { self.io.with_mut(|s, _| match s.state() { tcp::State::Listen | tcp::State::SynSent | tcp::State::SynReceived => { s.register_send_waker(cx.waker()); @@ -149,51 +145,49 @@ impl<'a> TcpSocket<'a> { } pub fn set_timeout(&mut self, duration: Option) { - unsafe { self.io.with_mut(|s, _| s.set_timeout(duration)) } + self.io.with_mut(|s, _| s.set_timeout(duration)) } pub fn set_keep_alive(&mut self, interval: Option) { - unsafe { self.io.with_mut(|s, _| s.set_keep_alive(interval)) } + self.io.with_mut(|s, _| s.set_keep_alive(interval)) } pub fn set_hop_limit(&mut self, hop_limit: Option) { - unsafe { self.io.with_mut(|s, _| s.set_hop_limit(hop_limit)) } + self.io.with_mut(|s, _| s.set_hop_limit(hop_limit)) } pub fn local_endpoint(&self) -> Option { - unsafe { self.io.with(|s, _| s.local_endpoint()) } + self.io.with(|s, _| s.local_endpoint()) } pub fn remote_endpoint(&self) -> Option { - unsafe { self.io.with(|s, _| s.remote_endpoint()) } + self.io.with(|s, _| s.remote_endpoint()) } pub fn state(&self) -> tcp::State { - unsafe { self.io.with(|s, _| s.state()) } + self.io.with(|s, _| s.state()) } pub fn close(&mut self) { - unsafe { self.io.with_mut(|s, _| s.close()) } + self.io.with_mut(|s, _| s.close()) } pub fn abort(&mut self) { - unsafe { self.io.with_mut(|s, _| s.abort()) } + self.io.with_mut(|s, _| s.abort()) } pub fn may_send(&self) -> bool { - unsafe { self.io.with(|s, _| s.may_send()) } + self.io.with(|s, _| s.may_send()) } pub fn may_recv(&self) -> bool { - unsafe { self.io.with(|s, _| s.may_recv()) } + self.io.with(|s, _| s.may_recv()) } } impl<'a> Drop for TcpSocket<'a> { fn drop(&mut self) { - // safety: not accessed reentrantly. - let s = unsafe { &mut *self.io.stack.get() }; - s.sockets.remove(self.io.handle); + self.io.stack.borrow_mut().sockets.remove(self.io.handle); } } @@ -201,21 +195,19 @@ impl<'a> Drop for TcpSocket<'a> { #[derive(Copy, Clone)] struct TcpIo<'a> { - stack: &'a UnsafeCell, + stack: &'a RefCell, handle: SocketHandle, } impl<'d> TcpIo<'d> { - /// SAFETY: must not call reentrantly. - unsafe fn with(&self, f: impl FnOnce(&tcp::Socket, &Interface) -> R) -> R { - let s = &*self.stack.get(); + fn with(&self, f: impl FnOnce(&tcp::Socket, &Interface) -> R) -> R { + let s = &*self.stack.borrow(); let socket = s.sockets.get::(self.handle); f(socket, &s.iface) } - /// SAFETY: must not call reentrantly. - unsafe fn with_mut(&mut self, f: impl FnOnce(&mut tcp::Socket, &mut Interface) -> R) -> R { - let s = &mut *self.stack.get(); + fn with_mut(&mut self, f: impl FnOnce(&mut tcp::Socket, &mut Interface) -> R) -> R { + let s = &mut *self.stack.borrow_mut(); let socket = s.sockets.get_mut::(self.handle); let res = f(socket, &mut s.iface); s.waker.wake(); @@ -223,7 +215,7 @@ impl<'d> TcpIo<'d> { } async fn read(&mut self, buf: &mut [u8]) -> Result { - poll_fn(move |cx| unsafe { + poll_fn(move |cx| { // CAUTION: smoltcp semantics around EOF are different to what you'd expect // from posix-like IO, so we have to tweak things here. self.with_mut(|s, _| match s.recv_slice(buf) { @@ -244,7 +236,7 @@ impl<'d> TcpIo<'d> { } async fn write(&mut self, buf: &[u8]) -> Result { - poll_fn(move |cx| unsafe { + poll_fn(move |cx| { self.with_mut(|s, _| match s.send_slice(buf) { // Not ready to send (no space in the tx buffer) Ok(0) => { @@ -332,6 +324,7 @@ mod embedded_io_impls { #[cfg(all(feature = "unstable-traits", feature = "nightly"))] pub mod client { + use core::cell::UnsafeCell; use core::mem::MaybeUninit; use core::ptr::NonNull; diff --git a/embassy-net/src/udp.rs b/embassy-net/src/udp.rs index f2e33493..4ddad77d 100644 --- a/embassy-net/src/udp.rs +++ b/embassy-net/src/udp.rs @@ -1,4 +1,4 @@ -use core::cell::UnsafeCell; +use core::cell::RefCell; use core::future::poll_fn; use core::mem; use core::task::Poll; @@ -27,7 +27,7 @@ pub enum Error { } pub struct UdpSocket<'a> { - stack: &'a UnsafeCell, + stack: &'a RefCell, handle: SocketHandle, } @@ -39,8 +39,7 @@ impl<'a> UdpSocket<'a> { tx_meta: &'a mut [PacketMetadata], tx_buffer: &'a mut [u8], ) -> Self { - // safety: not accessed reentrantly. - let s = unsafe { &mut *stack.socket.get() }; + let s = &mut *stack.socket.borrow_mut(); let rx_meta: &'static mut [PacketMetadata] = unsafe { mem::transmute(rx_meta) }; let rx_buffer: &'static mut [u8] = unsafe { mem::transmute(rx_buffer) }; @@ -63,30 +62,26 @@ impl<'a> UdpSocket<'a> { { let mut endpoint = endpoint.into(); - // safety: not accessed reentrantly. if endpoint.port == 0 { // If user didn't specify port allocate a dynamic port. - endpoint.port = unsafe { &mut *self.stack.get() }.get_local_port(); + endpoint.port = self.stack.borrow_mut().get_local_port(); } - // safety: not accessed reentrantly. - match unsafe { self.with_mut(|s, _| s.bind(endpoint)) } { + match self.with_mut(|s, _| s.bind(endpoint)) { Ok(()) => Ok(()), Err(udp::BindError::InvalidState) => Err(BindError::InvalidState), Err(udp::BindError::Unaddressable) => Err(BindError::NoRoute), } } - /// SAFETY: must not call reentrantly. - unsafe fn with(&self, f: impl FnOnce(&udp::Socket, &Interface) -> R) -> R { - let s = &*self.stack.get(); + fn with(&self, f: impl FnOnce(&udp::Socket, &Interface) -> R) -> R { + let s = &*self.stack.borrow(); let socket = s.sockets.get::(self.handle); f(socket, &s.iface) } - /// SAFETY: must not call reentrantly. - unsafe fn with_mut(&self, f: impl FnOnce(&mut udp::Socket, &mut Interface) -> R) -> R { - let s = &mut *self.stack.get(); + fn with_mut(&self, f: impl FnOnce(&mut udp::Socket, &mut Interface) -> R) -> R { + let s = &mut *self.stack.borrow_mut(); let socket = s.sockets.get_mut::(self.handle); let res = f(socket, &mut s.iface); s.waker.wake(); @@ -94,7 +89,7 @@ impl<'a> UdpSocket<'a> { } pub async fn recv_from(&self, buf: &mut [u8]) -> Result<(usize, IpEndpoint), Error> { - poll_fn(move |cx| unsafe { + poll_fn(move |cx| { self.with_mut(|s, _| match s.recv_slice(buf) { Ok(x) => Poll::Ready(Ok(x)), // No data ready @@ -113,7 +108,7 @@ impl<'a> UdpSocket<'a> { T: Into, { let remote_endpoint = remote_endpoint.into(); - poll_fn(move |cx| unsafe { + poll_fn(move |cx| { self.with_mut(|s, _| match s.send_slice(buf, remote_endpoint) { // Entire datagram has been sent Ok(()) => Poll::Ready(Ok(())), @@ -128,30 +123,28 @@ impl<'a> UdpSocket<'a> { } pub fn endpoint(&self) -> IpListenEndpoint { - unsafe { self.with(|s, _| s.endpoint()) } + self.with(|s, _| s.endpoint()) } pub fn is_open(&self) -> bool { - unsafe { self.with(|s, _| s.is_open()) } + self.with(|s, _| s.is_open()) } pub fn close(&mut self) { - unsafe { self.with_mut(|s, _| s.close()) } + self.with_mut(|s, _| s.close()) } pub fn may_send(&self) -> bool { - unsafe { self.with(|s, _| s.can_send()) } + self.with(|s, _| s.can_send()) } pub fn may_recv(&self) -> bool { - unsafe { self.with(|s, _| s.can_recv()) } + self.with(|s, _| s.can_recv()) } } impl Drop for UdpSocket<'_> { fn drop(&mut self) { - // safety: not accessed reentrantly. - let s = unsafe { &mut *self.stack.get() }; - s.sockets.remove(self.handle); + self.stack.borrow_mut().sockets.remove(self.handle); } }