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

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

> [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.
Fixed in a new commit: "dm-ploop: reject top delta with empty BAT".
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel