Re: [PATCH v2 3/3] zfcp: trace return values of sysfs unit add store

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

Pre-existing issues:
- [High] Unbalanced atomic_dec of port->units on the device_register error path in zfcp_unit_add, leading to integer underflow and port state corruption.
--

--- Patch [3]: [PATCH v2 3/3] zfcp: trace return values of sysfs unit add store ---
commit 5a4c6fdfefd98873215621f0e0b76392831e1659
Author: Chinmaya Kajagar <[email protected]>

zfcp: trace return values of sysfs unit add store

This commit adds a new debug trace to log failures when adding LUNs via the
sysfs unit_add attribute. It records the wwpn, fcp_lun, return value, and
associated hba whenever zfcp_unit_add() or the initial parsing fails.

> diff --git a/drivers/s390/scsi/zfcp_sysfs.c b/drivers/s390/scsi/zfcp_sysfs.c
> --- a/drivers/s390/scsi/zfcp_sysfs.c
> +++ b/drivers/s390/scsi/zfcp_sysfs.c
[ ... ]
> @@ -442,17 +442,24 @@ static ssize_t zfcp_sysfs_unit_add_store(struct device *dev,
[ ... ]
>  	flush_work(&port->rport_work);
>  
> -	retval = zfcp_unit_add(port, fcp_lun);
> -	if (retval)
> +	retval = zfcp_unit_add(port, fcp_lun);

[Severity: High]
This is a pre-existing issue, but since this patch handles and traces
errors returned from zfcp_unit_add(), does this error path contain an
unbalanced decrement?

Looking at zfcp_unit_add() in drivers/s390/scsi/zfcp_unit.c, if
device_register() fails:

drivers/s390/scsi/zfcp_unit.c:zfcp_unit_add() {
    ...
	if (device_register(&unit->dev)) {
		put_device(&unit->dev);
		retval = -ENOMEM;
		goto out;
	}

	atomic_inc(&port->units);
    ...
}

The put_device() call triggers the device release callback,
zfcp_unit_release(), which unconditionally decrements port->units:

drivers/s390/scsi/zfcp_unit.c:zfcp_unit_release() {
    ...
	atomic_dec(&unit->port->units);
    ...
}

Since device_register() fails before atomic_inc(&port->units) is called,
wouldn't the atomic_dec() in the release callback underflow the
port->units counter, corrupting the port state machine?

> +	if (retval) {
> +		zfcp_dbf_hba_uas("syuast2", 3, adapter, port->wwpn,
> +				 fcp_lun, retval);
>  		return retval;
> +	}
>  
>  	return count;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3