Re: [PATCH v2] scsi: ufs: fix use-after free in init error and remove paths

[Bart Van Assche <bvanassche@acm.org>]

On 1/14/25 11:56 AM, André Draszik wrote:
> Hi Bart,
> 
> On Tue, 2025-01-14 at 09:55 -0800, Bart Van Assche wrote:
>> On 1/14/25 8:16 AM, André Draszik wrote:
>>> +/**
>>> + * ufshcd_scsi_host_put_callback - deallocate underlying Scsi_Host and
>>> + *				   thereby the Host Bus Adapter (HBA)
>>> + * @host: pointer to SCSI host
>>> + */
>>> +static void ufshcd_scsi_host_put_callback(void *host)
>>> +{
>>> +	scsi_host_put(host);
>>> +}
>>
>> Please rename ufshcd_scsi_host_put_callback() such that the function
>> name makes clear when this function is called instead of what the
>> function does.
> 
> Would you have a suggestion for such a name? Something like
> ufshcd_driver_release_action()?
> 
> Unless I'm misunderstanding you, I believe most drivers use
> a function name that says what the function does, e.g.
> dell_wmi_ddv_debugfs_remove (just as a completely random
> example out of many).
> 
> If going by when it is called and if applying this principle
> throughout ufshcd, then there can only ever be one such
> function in ufshcd, as all devm_add_action() callback actions
> happen at driver release, which surely isn't what you mean.
> 
> You probably meant something different?

I meant what I wrote in my previous email: to chose another name
for ufshcd_scsi_host_put_callback() only. Having a function name that
duplicates the function body leaves readers of the code guessing
from where the function is called. BTW, naming callbacks after their
call site is a normal practice as far as I know. From ufs-qcom.c:

static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
	.name                   = "qcom",
	.init                   = ufs_qcom_init,
	.exit                   = ufs_qcom_exit,
	.get_ufs_hci_version	= ufs_qcom_get_ufs_hci_version,
	.clk_scale_notify	= ufs_qcom_clk_scale_notify,
	.setup_clocks           = ufs_qcom_setup_clocks,
	.hce_enable_notify      = ufs_qcom_hce_enable_notify,
	.link_startup_notify    = ufs_qcom_link_startup_notify,
	.pwr_change_notify	= ufs_qcom_pwr_change_notify,
	.apply_dev_quirks	= ufs_qcom_apply_dev_quirks,
	.fixup_dev_quirks       = ufs_qcom_fixup_dev_quirks,
	.suspend		= ufs_qcom_suspend,
	.resume			= ufs_qcom_resume,
	.dbg_register_dump	= ufs_qcom_dump_dbg_regs,
	.device_reset		= ufs_qcom_device_reset,
	.config_scaling_param = ufs_qcom_config_scaling_param,
	.reinit_notify		= ufs_qcom_reinit_notify,
	.mcq_config_resource	= ufs_qcom_mcq_config_resource,
	.get_hba_mac		= ufs_qcom_get_hba_mac,
	.op_runtime_config	= ufs_qcom_op_runtime_config,
	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
	.config_esi		= ufs_qcom_config_esi,
};

Bart.