Re: [PATCH v3 1/4] scsi: st: Restore changed drive settings after reset also for MTLOAD and MTRETEN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: st: MTLOAD in immediate mode attempts premature settings restore
--

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

scsi: st: Restore changed drive settings after reset also for MTLOAD and MTRETEN

A device reset returns the drive's density and block size to their defaults.
This patch attempts to re-apply values changed by the user when recovering
with MTLOAD or MTRETEN.

> diff --git a/drivers/scsi/st.c b/drivers/scsi/st.c
> index f1c3c49466379..0a4263bef9cbb 100644
> --- a/drivers/scsi/st.c
> +++ b/drivers/scsi/st.c
[ ... ]
> @@ -3819,6 +3848,9 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
>  
>  		if (mtc.mt_op == MTLOAD) {
>  			retval = do_load_unload(STp, file, max(1, mtc.mt_count));
> +			if (!retval)
> +				st_restore_changed_settings(STp, restore_density,
> +							    restore_blksize);

[Severity: High]
Does this code attempt a premature settings restore if MTLOAD is called
in immediate mode?

If a user application issues MTLOAD with O_NONBLOCK and STp->immediate
set to recover after a drive reset, do_load_unload() will issue the load
command and return immediately. This unconditional call to
st_restore_changed_settings() would then cause MODE SELECT commands to
fail because the drive is still asynchronously loading.

Later, when the drive is accessed, check_tape() detects the new session
and clears the changed flags, causing the drive to silently revert to
its default density and block size. Subsequent writes might then produce
incorrectly formatted tapes without errors.

For MTRETEN, this patch correctly checks for !STp->immediate before restoring
the settings:

>  			goto out;
>  		}
[ ... ]
> @@ -3837,6 +3869,19 @@ static long st_ioctl(struct file *file, unsigned int cmd_in, unsigned long arg)
>  			retval = st_compression(STp, (mtc.mt_count & 1));
>  		else
>  			retval = st_int_ioctl(STp, mtc.mt_op, mtc.mt_count);
> +		if (!retval && mtc.mt_op == MTRETEN && !STp->immediate &&
> +		    (restore_density >= 0 || restore_blksize >= 0)) {

Should MTLOAD also verify !STp->immediate before calling
st_restore_changed_settings() in st_ioctl()? Note that follow-up patches
in this series do not correct this omission.

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