Re: [PATCH v2] scsi: target: copy iSCSI ISID before unmapping the PR OUT buffer

[John Garry <john.g.garry@oracle.com>]

On 09/06/2026 12:36, James Bottomley wrote:
> On Tue, 2026-06-09 at 09:50 +0100, John Garry wrote:
>>> @@ -1808,9 +1809,11 @@ core_scsi3_decode_spec_i_port(
>>>     		core_scsi3_tpg_undepend_item(dest_tpg);
>>>     	}
>>>
>>> +	kfree(iport_ptr);
>>>     	return 0;
>>>     out_unmap:
>>>     	transport_kunmap_data_sg(cmd);
>>> +	kfree(iport_ptr);
>>
>> sorry for suggesting this change, but this is worse than what you had
>> originally, as we have so many paths to call kfree() [which means
>> more bugs possible]
>>
>> it's hard to make good suggestions for this target code as the
>> functions are so large and complex.
> 
> Given that it's allocated in a function called by the routine but never
> actually retained by anything what about defining it as
> 
> chat *iport_ptr __free(kfree) = NULL;
> 
> ?  That way we don't need to care about freeing it in the error legs.

It's not just freed in the error legs, as 
core_scsi3_decode_spec_i_port() calls target_parse_pr_out() in a loop

while (tpdl > 0) {

...

	spin_lock(&dev->se_port_lock);
	list_for_each_entry(tmp_lun, &dev->dev_sep_list, lun_dev_link) {
		...

		kfree(iport_ptr);
		iport_ptr = NULL;
		tid_found = target_parse_pr_out_transport_id(tmp_tpg,
				ptr, &tid_len, &iport_ptr, i_str);
		if (!tid_found)
			continue;

....

	kfree(iport_ptr);
out_unmap:
	kfree(iport_ptr);
out:
	...

If __free(kfree) attr can still handle this sort of flow, then ok.

I still think that it would be simpler to keep the mapping in place for 
longer (to avoid all of this), if possible.

Thanks,
John