Re: [Devel] [PATCH hci-8.1 v3 2/5] dm-ploop: set delta is_raw before applying its mappings

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dm-ploop: if (nr_be) guard in ploop_add_delta() skips the top delta header copy
--

commit fadb633bf83be4165d2be5c5a7e26f5f3b0c5554
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

dm-ploop: set delta is_raw before applying its mappings

This commit sets deltas[level].is_raw before ploop_apply_delta_mappings()
runs, so that raw deltas are no longer handled as non-raw ones. It also
skips applying mappings when the delta is empty (nr_be == 0).

Link: https://virtuozzo.atlassian.net/browse/VSTOR-148171

> diff --git a/drivers/md/dm-ploop-bat.c b/drivers/md/dm-ploop-bat.c
> --- a/drivers/md/dm-ploop-bat.c
> +++ b/drivers/md/dm-ploop-bat.c

[ ... ]

> @@ -501,7 +501,10 @@ int ploop_add_delta(struct ploop *ploop, u32 level, struct file *file, bool is_r
>  	if (level != ploop_top_level(ploop) && nr_be > deltas[level + 1].nr_be)
>  		goto out;
>  
> -	ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);
> +	deltas[level].is_raw = is_raw;
> +	/* Empty delta has no mappings, and BAT iterators can't handle it */
> +	if (nr_be)
> +		ploop_apply_delta_mappings(ploop, deltas, level, &md_root, nr_be);

[Severity: Low]
Can this unconditional if (nr_be) guard skip the top-level header copy?
The commit message only justifies the guard for an empty raw delta.

In ploop_apply_delta_mappings(), the on-disk header is copied into md0
only here:

    if (is_top_level && md->id == 0 && !is_raw) {
            /* bat_entries before PLOOP_MAP_OFFSET is hdr */
            memcpy(bat_entries, d_bat_entries,
                   sizeof(struct ploop_pvd_header));
    }

A non-raw top delta with m_Size == 0 appears to reach this path.
ploop_add_deltas_stack() calls ploop_check_top_delta() and then
ploop_add_delta().  ploop_check_top_delta() does not reject m_Size == 0,
and ploop_read_delta_metadata() can then return nr_be == 0.

Before the patch, ploop->bat_entries held md page 0 in this case, so the
loop body ran once and the memcpy was executed.  With the guard, the whole
function is skipped.

ploop_check_top_delta() only allocates md pages through
ploop_prealloc_md_pages().  Those pages are filled with BAT_ENTRY_NONE and
do not get the header.

Would md0 then keep the BAT_ENTRY_NONE fill instead of the header?
ploop_resize() reads hdr->m_SizeInSectors_v2 from md0, and writeback or
grow writes it back.

This looks limited to crafted or corrupt images, since a device with
nr_bat_entries == 0 cannot serve normal I/O.

Would it be better to apply the guard only for the raw case, or to still
copy the header for a non-raw top delta?  The later ploop_read_delta() and
ploop_reload_metadata() changes in the series keep the same if (nr_be)
guard, so it does not look like this is fixed later.

>  
>  	deltas[level].file = file;
>  	deltas[level].mtfile = kcalloc(ploop->nkt_runners, sizeof(*file),

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261008140333.614909-1-alexander.ivanov@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel