Re: [PATCH v2] iomap: avoid memset iomap when iter is done

在 2026/4/20 15:32, [email protected] 写道:
> Sashiko AI review found 1 potential issue(s):
> - [High] Early return paths in `iomap_iter()` bypass `folio_batch` cleanup, leaking page references.
> --
>
> commit 20686db0394fe5c92c4d207b4ec3987147d0e1ab
> Author: Fengnan Chang <[email protected]>
>
> iomap: avoid memset iomap when iter is done
>
> This commit splits the cleanup logic in iomap_iter() to avoid wasted memory
> writes by skipping the memset of iomap and srcmap when iteration finishes,
> while extracting the folio_batch release into iomap_iter_clean_fbatch().
>
>> However, we MUST still call iomap_iter_reset_iomap() to release the
>> folio_batch if IOMAP_F_FOLIO_BATCH is set, otherwise we leak page
>> references. Therefore, split the cleanup logic: always release the
>> folio_batch, but skip the memset() when ret <= 0.
> Does this actually always release the folio_batch?

Just skip memset, always release the folio_batch.

>
>> diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c
>> index c04796f6e57fa..e4a29829591a7 100644
>> --- a/fs/iomap/iter.c
>> +++ b/fs/iomap/iter.c
> [ ... ]
>> @@ -102,10 +98,14 @@ int iomap_iter(struct iomap_iter *iter, const struct iomap_ops *ops)
>>   		ret = 0;
>>   	else
>>   		ret = 1;
>> -	iomap_iter_reset_iomap(iter);
>> +	iomap_iter_clean_fbatch(iter);
>> +	iter->status = 0;
>>   	if (ret <= 0)
>>   		return ret;
>>   
>> +	memset(&iter->iomap, 0, sizeof(iter->iomap));
>> +	memset(&iter->srcmap, 0, sizeof(iter->srcmap));
>> +
>>   begin:
>>   	ret = ops->iomap_begin(iter->inode, iter->pos, iter->len, iter->flags,
>>   			       &iter->iomap, &iter->srcmap);
> If ops->iomap_begin() populates iter->fbatch and sets IOMAP_F_FOLIO_BATCH
> (for example, via iomap_fill_dirty_folios() in
> xfs_buffered_write_iomap_begin()) but subsequently fails, the next lines
> in the function are:
>
> 	if (ret < 0)
> 		return ret;
>
> This returns directly and bypasses the iomap_iter_clean_fbatch() call.
>
> There is also a similar early return at the start of iomap_iter() before
> the new cleanup logic:
>
> 	if (ops->iomap_end) {
> 		ret = ops->iomap_end(iter->inode, iter->iter_start_pos,
> 				iomap_length_trim(iter, iter->iter_start_pos,
> 						  olen),
> 				advanced, iter->flags, &iter->iomap);
> 		if (ret < 0 && !advanced)
> 			return ret;
>
> If ops->iomap_end() fails without advancing, this early return also skips
> the cleanup.
>
> Since callers like iomap_zero_range() allocate the batch on the stack and
> rely on iomap_iter() for cleanup, could these error paths leak folio
> references when transient I/O errors occur?
>