Re: [PATCH] scsi: scsi_dh_alua: Use common code in alua_rtpg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A race condition in `alua_rtpg` can cause ALUA port group state updates to be permanently dropped.
--

commit a4fc4f8579d1bb0ef6314519e4902a1bf335fb2b
Author: Markus Elfring <elfring@users.sourceforge.net>
Subject: scsi: scsi_dh_alua: Use common code in alua_rtpg()

This commit introduces a goto label to consolidate the memory freeing
and return logic in alua_rtpg(). It replaces multiple redundant kfree()
calls throughout the error handling paths of the function.

> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 80ab0ff921d43..0161507010643 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c

[ ... ]

> @@ -632,15 +632,14 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  			sdev_printk(KERN_ERR, sdev, "%s: rtpg retry\n",
>  				    ALUA_DH_NAME);
>  			scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
> -			kfree(buff);
> -			return err;
> +			goto free_buff;
>  		}
>  		sdev_printk(KERN_ERR, sdev, "%s: rtpg failed\n",
>  			    ALUA_DH_NAME);
>  		scsi_print_sense_hdr(sdev, ALUA_DH_NAME, &sense_hdr);
> -		kfree(buff);
>  		pg->expiry = 0;
> -		return SCSI_DH_IO;
> +		err = SCSI_DH_IO;
> +		goto free_buff;
>  	}
>  
>  	len = get_unaligned_be32(&buff[0]) + 4;

[Severity: High]
This is a pre-existing issue, but was not introduced by the patch under
review. While we are consolidating the return paths in alua_rtpg(), does
this function still have a race condition when applying the new access
state?

Looking at the loop that processes the descriptors further down in
alua_rtpg():

    if (spin_trylock_irqsave(&tmp_pg->lock, flags)) {
        if ((tmp_pg == pg) ||
            !(tmp_pg->flags & ALUA_PG_RUNNING)) {
            struct alua_dh_data *h;

            tmp_pg->state = desc[0] & 0x0f;

If this trylock fails because the lock is held by a function that doesn't
set the ALUA_PG_RUN_RTPG flag (like alua_bus_detach() or
alua_set_params()), the new ALUA access state is silently discarded without
queuing a retry.

Could this cause the port group's access state to become permanently stale
(e.g., stuck in SCSI_ACCESS_STATE_STANDBY despite the hardware
transitioning to an active state), ultimately failing all I/O to
non-optimal paths and causing data unavailability?

> @@ -770,6 +769,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  		break;
>  	}
>  	spin_unlock_irqrestore(&pg->lock, flags);
> +free_buff:
>  	kfree(buff);
>  	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/366c376e-ef8a-490d-a580-570f2f51ec81@web.de?part=1