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

[Laurence Oberman <loberman@redhat.com>]

On Tue, 2026-09-29 at 17:33 +0000, sashiko-bot@kernel.org wrote:
> 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.
> 
> 
Hello Sashiko, 

This only affects MTLOAD in immediate mode with an O_NONBLOCK open;
with a blocking open check_tape() waits for the drive and the restore
works. In the non-blocking case the MODE SELECTs fail and the settings
stay at the drive defaults, which is the current behaviour without the
patch, so it is not a regression. I can add a "STp->ready == ST_READY"
check before the restore in a follow-up and leave v3 as is.

Thanks
Laurence