Re: [PATCH v3 2/4] scsi: st: Record the tape position after a successful MTLOAD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: st: data corruption via rogue filemarks after MTLOAD
--

commit fa623b9485fe23a48925ee926b259a954fdd216e
Author: Laurence Oberman <loberman@redhat.com>

scsi: st: Record the tape position after a successful MTLOAD

This commit records the tape partition, position, and EOF state after a
successful MTLOAD, similar to how check_tape() does for a new session.
This fixes an issue where the driver would incorrectly report file/block
as -1 because drives didn't report a new-medium unit attention when
already loaded.

> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index 0a4263bef9cbb..e5b2e2d4872d3 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
> @@ -2686,6 +2686,22 @@ static int do_load_unload(struct scsi_tape *STp, struct file *filp, int load_cod
>  		else {
>  			STp->rew_at_close = STp->autorew_dev;
>  			retval = check_tape(STp, filp);
> +			/*
> +			 * LOAD leaves the medium at the beginning of partition
> +			 * 0.  check_tape() records that only for a new session;
> +			 * without a new-medium unit attention (the medium was
> +			 * already loaded) the partition, position and EOF state
> +			 * would be left as before the load.  Set them as a new
> +			 * session and MTREW do.
> +			 */
> +			if (retval == CHKRES_READY) {
> +				STp->partition = STp->new_partition = 0;
> +				STps = &(STp->ps[0]);
> +				STps->drv_file = STps->drv_block = 0;
> +				STps->eof = ST_NOEOF;
> +				STps->at_sm = 0;
> +				STps->last_block_valid = 0;

[Severity: High]
Does this code risk data corruption by omitting a reset of the rw state?

If a user writes data to partition 0 (setting ps[0].rw = ST_WRITING), issues
MTSETPART 1, and then switches to partition 1, ps[0].rw remains ST_WRITING.

If the user then issues MTLOAD, the ioctl clears the rw state for the current
partition (ps[1]), but do_load_unload() then forces STp->partition = 0 without
clearing ps[0].rw.

Upon closing the device, st_flush() would see ps[0].rw == ST_WRITING and
unconditionally execute WRITE_FILEMARKS at the beginning of the tape (BOT),
overwriting and destroying previously recorded data.

Should STps->rw = ST_IDLE; be added to this block to prevent rogue filemark
writes at BOT?

> +			}
>  			if (retval > 0)
>  				retval = 0;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929171813.844733-1-loberman@redhat.com?part=2