fix(nexus): fixing nexus unshare and nvmf error handling

NVMF subsystem API and Nexus unshare had some issues with error handling, which could potentially lead to use-after-free. Signed-off-by: Dmitry Savitskiy <[email protected]>
openebs · Jun 27, 2023 · 123a004 · 123a004
1 parent 122e96f
commit 123a004
Show file tree

Hide file tree

Showing 9 changed files with 198 additions and 222 deletions.
diff --git a/mayastor/src/bdev/nexus/nexus_bdev.rs b/mayastor/src/bdev/nexus/nexus_bdev.rs
@@ -479,9 +479,6 @@ pub struct Nexus<'n> {
     pub state: parking_lot::Mutex<NexusState>,
     /// The offset in blocks where the data partition starts.
     pub(crate) data_ent_offset: u64,
-    /// the handle to be used when sharing the nexus, this allows for the bdev
-    /// to be shared with vbdevs on top
-    pub(crate) share_handle: Option<String>,
     /// enum containing the protocol-specific target used to publish the nexus
     pub nexus_target: Option<NexusTarget>,
     /// Indicates if the Nexus has an I/O device.
@@ -588,7 +585,6 @@ impl<'n> Nexus<'n> {
             state: parking_lot::Mutex::new(NexusState::Init),
             bdev: None,
             data_ent_offset: 0,
-            share_handle: None,
             req_size: size,
             nexus_target: None,
             nvme_params,
@@ -879,7 +875,7 @@ impl<'n> Nexus<'n> {
     pub async fn destroy(mut self: Pin<&mut Self>) -> Result<(), Error> {
         info!("Destroying nexus {}", self.name);
 
-        self.as_mut().destroy_shares().await?;
+        self.as_mut().unshare_nexus().await?;
 
         // wait for all rebuild jobs to be cancelled before proceeding with the
         // destruction of the nexus
@@ -1473,6 +1469,13 @@ async fn nexus_create_internal(
     children: &[String],
     nexus_info_key: Option<String>,
 ) -> Result<(), Error> {
+    info!(
+        "Creating new nexus '{}' ({} child(ren): {:?})...",
+        name,
+        children.len(),
+        children
+    );
+
     if let Some(nexus) = nexus_lookup_name_uuid(name, nexus_uuid) {
         // FIXME: Instead of error, we return Ok without checking
         // that the children match, which seems wrong.

diff --git a/mayastor/src/bdev/nexus/nexus_io_subsystem.rs b/mayastor/src/bdev/nexus/nexus_io_subsystem.rs
@@ -90,7 +90,13 @@ impl<'n> NexusIoSubsystem<'n> {
                             NvmfSubsystem::nqn_lookup(&self.name)
                         {
                             trace!(nexus=%self.name, nqn=%subsystem.get_nqn(), "pausing subsystem");
-                            subsystem.pause().await.unwrap();
+                            if let Err(e) = subsystem.pause().await {
+                                panic!(
+                                    "Failed to pause subsystem '{}: {}",
+                                    subsystem.get_nqn(),
+                                    e
+                                );
+                            }
                             trace!(nexus=%self.name, nqn=%subsystem.get_nqn(), "subsystem paused");
                         }
                     }
@@ -183,7 +189,13 @@ impl<'n> NexusIoSubsystem<'n> {
                                 self.pause_state
                                     .store(NexusPauseState::Unpausing);
                                 trace!(nexus=%self.name, nqn=%subsystem.get_nqn(), "resuming subsystem");
-                                subsystem.resume().await.unwrap();
+                                if let Err(e) = subsystem.resume().await {
+                                    panic!(
+                                        "Failed to resume subsystem '{}: {}",
+                                        subsystem.get_nqn(),
+                                        e
+                                    );
+                                }
                                 trace!(nexus=%self.name, nqn=%subsystem.get_nqn(), "subsystem resumed");
                             }
                             self.pause_state.store(NexusPauseState::Unpaused);

diff --git a/mayastor/src/bdev/nexus/nexus_share.rs b/mayastor/src/bdev/nexus/nexus_share.rs
@@ -14,7 +14,6 @@ use super::{
 
 use crate::core::{Protocol, Share};
 
-#[async_trait(? Send)]
 ///
 /// The sharing of the nexus is different compared to regular bdevs
 /// the Impl of ['Share'] handles this accordingly
@@ -23,6 +22,7 @@ use crate::core::{Protocol, Share};
 /// endpoints (not targets) however, we want to avoid too many
 /// protocol specifics and for bdevs the need for different endpoints
 /// is not implemented yet as the need for it has not arrived yet.
+#[async_trait(? Send)]
 impl<'n> Share for Nexus<'n> {
     type Error = Error;
     type Output = String;
@@ -31,8 +31,10 @@ impl<'n> Share for Nexus<'n> {
         mut self: Pin<&mut Self>,
         cntlid_range: Option<(u16, u16)>,
     ) -> Result<Self::Output, Self::Error> {
-        match self.shared() {
+        let uri = match self.shared() {
             Some(Protocol::Off) | None => {
+                info!("{:?}: sharing NVMF target...", self);
+
                 let name = self.name.clone();
                 self.as_mut()
                     .pin_bdev_mut()
@@ -41,20 +43,37 @@ impl<'n> Share for Nexus<'n> {
                     .context(ShareNvmfNexus {
                         name,
                     })?;
+
+                let uri = self.share_uri().unwrap();
+                info!("{:?}: shared NVMF target as '{}'", self, uri);
+                uri
             }
-            Some(Protocol::Nvmf) => {}
-        }
-        Ok(self.share_uri().unwrap())
+            Some(Protocol::Nvmf) => {
+                let uri = self.share_uri().unwrap();
+                info!("{:?}: already shared as '{}'", self, uri);
+                uri
+            }
+        };
+
+        Ok(uri)
     }
 
     /// TODO
-    async fn unshare(
-        self: Pin<&mut Self>,
-    ) -> Result<Self::Output, Self::Error> {
+    async fn unshare(mut self: Pin<&mut Self>) -> Result<(), Self::Error> {
+        info!("{:?}: unsharing nexus bdev...", self);
+
         let name = self.name.clone();
-        self.pin_bdev_mut().unshare().await.context(UnshareNexus {
-            name,
-        })
+        self.as_mut()
+            .pin_bdev_mut()
+            .unshare()
+            .await
+            .context(UnshareNexus {
+                name,
+            })?;
+
+        info!("{:?}: unshared nexus bdev", self);
+
+        Ok(())
     }
 
     /// TODO
@@ -144,33 +163,21 @@ impl<'n> Nexus<'n> {
 
     /// TODO
     pub async fn unshare_nexus(mut self: Pin<&mut Self>) -> Result<(), Error> {
-        unsafe {
-            match self.as_mut().get_unchecked_mut().nexus_target.take() {
-                Some(NexusTarget::NbdDisk(disk)) => {
-                    disk.destroy();
-                }
-                Some(NexusTarget::NexusNvmfTarget) => {
-                    self.as_mut().unshare().await?;
-                }
-                None => {
-                    warn!("{} was not shared", self.name);
-                }
+        match unsafe { self.as_mut().get_unchecked_mut().nexus_target.take() } {
+            Some(NexusTarget::NbdDisk(disk)) => {
+                info!("{:?}: destroying NBD device target...", self);
+                disk.destroy();
+            }
+            Some(NexusTarget::NexusNvmfTarget) => {
+                info!("{:?}: unsharing NVMF target...", self);
+            }
+            None => {
+                // Try unshare nexus bdev anyway, just in case it was shared
+                // via bdev API. It is no-op if bdev was not shared.
             }
         }
 
-        Ok(())
-    }
-
-    /// Shutdowns all shares.
-    pub(crate) async fn destroy_shares(
-        mut self: Pin<&mut Self>,
-    ) -> Result<(), Error> {
-        let _ = self.as_mut().unshare_nexus().await;
-        assert_eq!(self.share_handle, None);
-
-        // no-op when not shared and will be removed once the old share bits are
-        // gone. Ignore device name provided in case of successful unsharing.
-        self.as_mut().unshare().await.map(|_| ())
+        self.as_mut().unshare().await
     }
 
     /// TODO

diff --git a/mayastor/src/core/bdev.rs b/mayastor/src/core/bdev.rs
@@ -199,21 +199,18 @@ where
     }
 
     /// unshare the bdev regardless of current active share
-    async fn unshare(
-        self: Pin<&mut Self>,
-    ) -> Result<Self::Output, Self::Error> {
+    async fn unshare(self: Pin<&mut Self>) -> Result<(), Self::Error> {
         match self.shared() {
             Some(Protocol::Nvmf) => {
-                if let Some(subsystem) = NvmfSubsystem::nqn_lookup(self.name())
-                {
-                    subsystem.stop().await.context(UnshareNvmf {})?;
-                    subsystem.destroy();
+                if let Some(ss) = NvmfSubsystem::nqn_lookup(self.name()) {
+                    ss.stop().await.context(UnshareNvmf {})?;
+                    ss.destroy();
                 }
             }
             Some(Protocol::Off) | None => {}
         }
 
-        Ok(self.name().to_string())
+        Ok(())
     }
 
     /// returns if the bdev is currently shared

diff --git a/mayastor/src/core/share.rs b/mayastor/src/core/share.rs
@@ -49,8 +49,7 @@ pub trait Share: std::fmt::Debug {
     ) -> Result<Self::Output, Self::Error>;
 
     /// TODO
-    async fn unshare(self: Pin<&mut Self>)
-        -> Result<Self::Output, Self::Error>;
+    async fn unshare(self: Pin<&mut Self>) -> Result<(), Self::Error>;
 
     /// TODO
     fn shared(&self) -> Option<Protocol>;

diff --git a/mayastor/src/lvs/lvol.rs b/mayastor/src/lvs/lvol.rs
@@ -134,20 +134,18 @@ impl Share for Lvol {
     }
 
     /// unshare the nvmf target
-    async fn unshare(
-        mut self: Pin<&mut Self>,
-    ) -> Result<Self::Output, Self::Error> {
-        let share =
-            Pin::new(&mut self.as_bdev()).unshare().await.map_err(|e| {
-                Error::LvolUnShare {
-                    source: e,
-                    name: self.name(),
-                }
-            })?;
+    async fn unshare(mut self: Pin<&mut Self>) -> Result<(), Self::Error> {
+        Pin::new(&mut self.as_bdev()).unshare().await.map_err(|e| {
+            Error::LvolUnShare {
+                source: e,
+                name: self.name(),
+            }
+        })?;
 
         self.as_mut().set(PropValue::Shared(false)).await?;
+
         info!("unshared {}", self);
-        Ok(share)
+        Ok(())
     }
 
     /// return the protocol this bdev is shared under

diff --git a/mayastor/src/subsys/nvmf/mod.rs b/mayastor/src/subsys/nvmf/mod.rs
@@ -63,6 +63,12 @@ pub enum Error {
     PgError { msg: String },
     #[snafu(display("Failed to create transport {}", msg))]
     Transport { source: Errno, msg: String },
+    #[snafu(display(
+        "Failed to {} subsystem '{}': subsystem is busy",
+        op,
+        nqn
+    ))]
+    SubsystemBusy { nqn: String, op: String },
     #[snafu(display("Failed nvmf subsystem operation for {} {} error: {}", source.desc(), nqn, msg))]
     Subsystem {
         source: Errno,