[PATCH] fix #7823: tape: don't set up a data-in transfer for non-data commands

[PATCH] fix #7823: tape: don't set up a data-in transfer for non-data commands

[Dominik Csapak]

do_command() required a transfer buffer of at least 16 bytes, so all
commands without a data transfer phase (REWIND, SPACE, LOCATE, MOVE
MEDIUM, ...) allocated a dummy buffer just to satisfy that check. Since
create_scsi_pt_obj() hands any non-empty buffer to
set_scsi_pt_data_in(), those commands announced a data-in transfer that
they never fulfill.

Targets do not have to tolerate that. SCST for example compares the
direction announced by the transport with the one it decodes from the
CDB and fails the command with ILLEGAL REQUEST if they differ.

Drop the minimum buffer size check and pass a buffer size of zero for
commands without a data transfer phase, so that no data-in transfer is
set up at all. This is also what sg3_utils and mtx do.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---

Tested some action (move/unload/load/backup/etc.) with a VTL and a
physical tape library and both worked fine. Nonetheless, I think the
change and its regression potential warrants a bit of extra testing and
maybe some extra time on the testing repo though.

 pbs-tape/src/sg_pt_changer.rs |  8 ++++----
 pbs-tape/src/sg_tape.rs       | 24 ++++++++++++------------
 pbs-tape/src/sgutils2.rs      | 15 +++++++++++----
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/pbs-tape/src/sg_pt_changer.rs b/pbs-tape/src/sg_pt_changer.rs
index 6a1285156..aff57b1b3 100644
--- a/pbs-tape/src/sg_pt_changer.rs
+++ b/pbs-tape/src/sg_pt_changer.rs
@@ -23,7 +23,7 @@ const SCSI_VOLUME_TAG_LEN: usize = 36;
 
 /// Initialize element status (Inventory)
 pub fn initialize_element_status<F: AsRawFd>(file: &mut F) -> Result<(), Error> {
-    let mut sg_raw = SgRaw::new(file, 64)?;
+    let mut sg_raw = SgRaw::new(file, 0)?;
 
     // like mtx(1), set a very long timeout (30 minutes)
     sg_raw.set_timeout(30 * 60);
@@ -181,7 +181,7 @@ pub fn load_slot(file: &mut File, from_slot: u64, drivenum: u64) -> Result<(), E
         drive_element_address,
     );
 
-    let mut sg_raw = SgRaw::new(file, 64)?;
+    let mut sg_raw = SgRaw::new(file, 0)?;
     sg_raw.set_timeout(SCSI_CHANGER_MOVE_MEDIUM_TIMEOUT);
 
     sg_raw
@@ -205,7 +205,7 @@ pub fn unload(file: &mut File, to_slot: u64, drivenum: u64) -> Result<(), Error>
         target_element_address,
     );
 
-    let mut sg_raw = SgRaw::new(file, 64)?;
+    let mut sg_raw = SgRaw::new(file, 0)?;
     sg_raw.set_timeout(SCSI_CHANGER_MOVE_MEDIUM_TIMEOUT);
 
     sg_raw
@@ -233,7 +233,7 @@ pub fn transfer_medium<F: AsRawFd>(
         target_element_address,
     );
 
-    let mut sg_raw = SgRaw::new(file, 64)?;
+    let mut sg_raw = SgRaw::new(file, 0)?;
     sg_raw.set_timeout(SCSI_CHANGER_MOVE_MEDIUM_TIMEOUT);
 
     sg_raw.do_command(&cmd).map_err(|err| {
diff --git a/pbs-tape/src/sg_tape.rs b/pbs-tape/src/sg_tape.rs
index 3e6aac685..3fc6873a8 100644
--- a/pbs-tape/src/sg_tape.rs
+++ b/pbs-tape/src/sg_tape.rs
@@ -254,7 +254,7 @@ impl SgTape {
     /// Tape).
     #[allow(clippy::vec_init_then_push)]
     pub fn erase_media(&mut self, fast: bool) -> Result<(), Error> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
         let mut cmd = Vec::new();
         cmd.push(0x19);
@@ -302,7 +302,7 @@ impl SgTape {
         } else {
             self.rewind()?;
 
-            let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+            let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
             sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
             let mut cmd = Vec::new();
 
@@ -332,7 +332,7 @@ impl SgTape {
 
     /// Lock/Unlock drive door
     pub fn set_medium_removal(&mut self, allow: bool) -> Result<(), ScsiError> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
         let mut cmd = Vec::new();
         cmd.extend([0x1E, 0, 0, 0]);
@@ -349,7 +349,7 @@ impl SgTape {
     }
 
     pub fn rewind(&mut self) -> Result<(), Error> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
         let mut cmd = Vec::new();
         cmd.extend([0x01, 0, 0, 0, 0, 0]); // REWIND
@@ -372,7 +372,7 @@ impl SgTape {
         // Special case for position 1, because LOCATE 0 does not work
         if position == 1 {
             self.rewind()?;
-            let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+            let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
             sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
             sg_raw
                 .do_command(SPACE_ONE_FILEMARK)
@@ -380,7 +380,7 @@ impl SgTape {
             return Ok(());
         }
 
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
 
         // Note: LOCATE(16) works for LTO4 or newer
@@ -518,7 +518,7 @@ impl SgTape {
     }
 
     pub fn move_to_eom(&mut self, write_missing_eof: bool) -> Result<(), Error> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
         let mut cmd = Vec::new();
         cmd.extend([0x11, 0x03, 0, 0, 0, 0]); // SPACE(6) move to EOD
@@ -535,7 +535,7 @@ impl SgTape {
     }
 
     fn space(&mut self, count: isize, blocks: bool) -> Result<(), ScsiError> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
         let mut cmd = Vec::new();
 
@@ -580,7 +580,7 @@ impl SgTape {
     }
 
     pub fn eject(&mut self) -> Result<(), Error> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
         let mut cmd = Vec::new();
         cmd.extend([0x1B, 0, 0, 0, 0, 0]); // LODA/UNLOAD HOLD=0, LOAD=0
@@ -593,7 +593,7 @@ impl SgTape {
     }
 
     pub fn load(&mut self) -> Result<(), Error> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(Self::SCSI_TAPE_DEFAULT_TIMEOUT);
         let mut cmd = Vec::new();
         cmd.extend([0x1B, 0, 0, 0, 0b0000_0001, 0]); // LODA/UNLOAD HOLD=0, LOAD=1
@@ -610,7 +610,7 @@ impl SgTape {
             proxmox_lang::io_bail!("write_filemarks failed: got strange count '{count}'");
         }
 
-        let mut sg_raw = SgRaw::new(&mut self.file, 16).map_err(|err| {
+        let mut sg_raw = SgRaw::new(&mut self.file, 0).map_err(|err| {
             proxmox_lang::io_format_err!("write_filemarks failed (alloc) - {err}")
         })?;
 
@@ -646,7 +646,7 @@ impl SgTape {
     }
 
     pub fn test_unit_ready(&mut self) -> Result<(), Error> {
-        let mut sg_raw = SgRaw::new(&mut self.file, 16)?;
+        let mut sg_raw = SgRaw::new(&mut self.file, 0)?;
         sg_raw.set_timeout(30); // use short timeout
         let mut cmd = Vec::new();
         cmd.extend([0x00, 0, 0, 0, 0, 0]); // TEST UNIT READY
diff --git a/pbs-tape/src/sgutils2.rs b/pbs-tape/src/sgutils2.rs
index 340616c73..908d50b2f 100644
--- a/pbs-tape/src/sgutils2.rs
+++ b/pbs-tape/src/sgutils2.rs
@@ -604,21 +604,28 @@ impl<'a, F: AsRawFd> SgRaw<'a, F> {
     }
 
     /// Run the specified RAW SCSI command
+    ///
+    /// The transfer buffer passed to [`SgRaw::new`] is used as data-in buffer.
+    /// Commands without a data transfer phase must be run with a buffer size of
+    /// zero, so that no transfer is set up at all and the returned slice stays
+    /// empty. Otherwise such a command asks the device for data it never sends,
+    /// which some devices reject outright.
     pub fn do_command(&mut self, cmd: &[u8]) -> Result<&[u8], ScsiError> {
         if !unsafe { sg_is_scsi_cdb(cmd.as_ptr(), cmd.len() as c_int) } {
             return Err(format_err!("no valid SCSI command").into());
         }
 
-        if self.buffer.len() < 16 {
-            return Err(format_err!("input buffer too small").into());
-        }
-
         let mut ptvp = self.create_scsi_pt_obj()?;
 
         unsafe { set_scsi_pt_cdb(ptvp.as_mut_ptr(), cmd.as_ptr(), cmd.len() as c_int) };
 
         self.do_scsi_pt_checked(&mut ptvp)?;
 
+        if self.buffer.is_empty() {
+            // no data transfer was set up, so there is nothing to read
+            return Ok(&[]);
+        }
+
         let resid = unsafe { get_scsi_pt_resid(ptvp.as_ptr()) } as usize;
         if resid > self.buffer.len() {
             return Err(
-- 
2.47.3