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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2026-06-09 at 13:14 +0100, John Garry wrote:
> 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:
> 	...

You'd simply remove those since the pointer would be kfree'd on any
function return.

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

It can, but there's nuance.  If you do an early free (or a transfer of
ownership) you have to set the original pointer to NULL to avoid kfree
being called on it on return.

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

I was just looking at the multiple error legs in the patched code and
thinking it's an ideal candidate for a __free() attribute.  If there's
a way of rearranging the code to keep it longer that you think is more
readable, by all means do that instead.

Regards,

James