From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 3B37B1FF0A7 for ; Mon, 17 Aug 2026 14:31:32 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 1217623DA1; Mon, 17 Aug 2026 14:31:32 +0200 (CEST) From: Dominik Csapak To: pbs-devel@lists.proxmox.com Subject: [PATCH] fix #7823: tape: don't set up a data-in transfer for non-data commands Date: Mon, 17 Aug 2026 14:29:52 +0200 Message-ID: <20260817123128.2813924-1-d.csapak@proxmox.com> X-Mailer: git-send-email 2.47.3 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.964 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: C3WSODPGPWAUMX2B4CDDE6L7F3SVLAH6 X-Message-ID-Hash: C3WSODPGPWAUMX2B4CDDE6L7F3SVLAH6 X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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(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( 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