Hi, > The pagemap array ends 3,072 bytes before the data area starts (padding > > zone). Most out-of-bounds entries fall in this padding and cause silent > > corruption. The last 17 entries fall in the data area itself, causing > > bidirectional corruption: pagemap writes destroy allocated object data,
AFAICT, The pagemap array tracks all pages in the segment, including
the metadata
pages. The bug is that the pagemap entries for the metadata pages were
not allocated
when we go into the odd-sized allocation path.
It is indeed difficult to get a reproducer for this, but not allocating pagemap
entries for the metadata pages is indeed wrong.
So, we should first add an Assert that determines we have allocated
enough pagemap entries for all the pages. We can do this by computing the number
of pagemap entries that fit in the metadata region and verifying it is at
least as large as the total number of pages in the segment.
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -2196,6 +2196,8 @@ make_new_segment(dsa_area *area, size_t requested_pages)
/* See if that is enough... */
if (requested_pages > usable_pages)
{
+ size_t total_pages;
+
/*
* We'll make an odd-sized segment, working forward
from the requested
* number of pages.
@@ -2210,6 +2212,11 @@ make_new_segment(dsa_area *area, size_t requested_pages)
if (metadata_bytes % FPM_PAGE_SIZE != 0)
metadata_bytes += FPM_PAGE_SIZE -
(metadata_bytes % FPM_PAGE_SIZE);
total_size = metadata_bytes + usable_pages * FPM_PAGE_SIZE;
+ total_pages = total_size / FPM_PAGE_SIZE;
+
+ /* Verify we allocated enough pagemap entries for all pages */
+ Assert((metadata_bytes - MAXALIGN(sizeof(dsa_segment_header)) -
+ MAXALIGN(sizeof(FreePageManager))) /
sizeof(dsa_pointer) >= total_pages);
With the above assert in place, the usable_pages of 879 ars you have
in test_dsa.c crashes
due to the assertion failure.
As far as the fix, I do agree with what you have in 0001-, except I am
not so sure about the "+1 for rounding".
Can we just do ceiling division?
+ /*
+ * We must also account for pagemap entries needed to cover the
+ * metadata pages themselves. The pagemap must track
all pages in the
+ * segment, including the pages occupied by metadata.
+ */
+ metadata_bytes +=
+ ((metadata_bytes + (FPM_PAGE_SIZE -
sizeof(dsa_pointer)) - 1) /
+ (FPM_PAGE_SIZE - sizeof(dsa_pointer))) *
+ sizeof(dsa_pointer);
I don't think we should add a test_dsa, but I do think it was useful
to verify the issue.
This sounds like it should be backpatched, but we'll see what a
committer thinks.
See attached.
What do you think?
--
Sami Imseih
Amazon Web Services
v2-0001-Fix-DSA-pagemap-undersizing-in-make_new_segment.patch
Description: Binary data
