Re: Shared buffer access rule violations?
Hi, On 2019-01-13 19:52:32 -0500, Tom Lane wrote: > Thomas Munro writes: > > On Thu, Aug 9, 2018 at 12:59 PM Asim R P wrote: > >> On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan wrote: > >>> I wonder if it would be a better idea to enable Valgrind's memcheck to > >>> mark buffers as read-only or read-write. > > >> Basic question: how do you mark buffers as read-only using memcheck > >> tool? Running installcheck with valgrind didn't uncover any errors: > > > Presumably with VALGRIND_xxx macros, but is there a way to make that > > work for shared memory? > > > I like the mprotect() patch. This could be enabled on a build farm > > animal. > > I think this is a cute idea and potentially useful as an alternative > to valgrind, but I don't like the patch much. It'd be better to > set things up so that the patch adds support for catching bad accesses > with either valgrind or mprotect, according to compile options. Also, > this sort of thing is gratitously ugly: > > +#ifdef MPROTECT_BUFFERS > + BufferMProtect(buf, PROT_NONE); > +#endif > > The right way IMO is to just have macro calls like > > ProtectBuffer(buf, NO_ACCESS); > > which expand to nothing at all if the feature isn't enabled by #ifdefs, > and otherwise to whatever we need it to do. (The access-type symbols > then need to be something that can be defined correctly for either > implementation.) As this has not been addressed since, and the CF has ended, I'm marking this patch as returned with feedback. Please resubmit once that's addressed. > > I guess it would either fail explicitly or behave incorrectly > > for VM page size > BLCKSZ depending on OS, but at least on Linux/amd64 > > you have to go out of your way to get pages > 4KB so that seems OK for > > a debugging feature. > > What worries me more is that I don't think we try hard to ensure that > buffers are aligned on system page boundaries. It's probably worthwhile to just always force that, to reduce unnecessary page misses. Greetings, Andres Freund
Re: Shared buffer access rule violations?
Thomas Munro writes: > On Thu, Aug 9, 2018 at 12:59 PM Asim R P wrote: >> On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan wrote: >>> I wonder if it would be a better idea to enable Valgrind's memcheck to >>> mark buffers as read-only or read-write. >> Basic question: how do you mark buffers as read-only using memcheck >> tool? Running installcheck with valgrind didn't uncover any errors: > Presumably with VALGRIND_xxx macros, but is there a way to make that > work for shared memory? > I like the mprotect() patch. This could be enabled on a build farm > animal. I think this is a cute idea and potentially useful as an alternative to valgrind, but I don't like the patch much. It'd be better to set things up so that the patch adds support for catching bad accesses with either valgrind or mprotect, according to compile options. Also, this sort of thing is gratitously ugly: +#ifdef MPROTECT_BUFFERS + BufferMProtect(buf, PROT_NONE); +#endif The right way IMO is to just have macro calls like ProtectBuffer(buf, NO_ACCESS); which expand to nothing at all if the feature isn't enabled by #ifdefs, and otherwise to whatever we need it to do. (The access-type symbols then need to be something that can be defined correctly for either implementation.) > I guess it would either fail explicitly or behave incorrectly > for VM page size > BLCKSZ depending on OS, but at least on Linux/amd64 > you have to go out of your way to get pages > 4KB so that seems OK for > a debugging feature. What worries me more is that I don't think we try hard to ensure that buffers are aligned on system page boundaries. regards, tom lane
Re: Shared buffer access rule violations?
On Thu, Aug 9, 2018 at 12:59 PM Asim R P wrote: > On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan wrote: > > I wonder if it would be a better idea to enable Valgrind's memcheck to > > mark buffers as read-only or read-write. We've considered doing > > something like that for years, but for whatever reason nobody followed > > through. > > Basic question: how do you mark buffers as read-only using memcheck > tool? Running installcheck with valgrind didn't uncover any errors: > > valgrind --trace-children=yes pg_ctl -D datadir start > make installcheck-parallel Presumably with VALGRIND_xxx macros, but is there a way to make that work for shared memory? I like the mprotect() patch. This could be enabled on a build farm animal. I guess it would either fail explicitly or behave incorrectly for VM page size > BLCKSZ depending on OS, but at least on Linux/amd64 you have to go out of your way to get pages > 4KB so that seems OK for a debugging feature. I would like to do something similar with DSA, to electrify superblocks and whole segments that are freed. That would have caught a recent bug in DSA itself and in client code. -- Thomas Munro http://www.enterprisedb.com
Re: Shared buffer access rule violations?
On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan wrote: > > I wonder if it would be a better idea to enable Valgrind's memcheck to > mark buffers as read-only or read-write. We've considered doing > something like that for years, but for whatever reason nobody followed > through. Basic question: how do you mark buffers as read-only using memcheck tool? Running installcheck with valgrind didn't uncover any errors: valgrind --trace-children=yes pg_ctl -D datadir start make installcheck-parallel Asim & David
Re: Shared buffer access rule violations?
On Tue, Aug 7, 2018 at 6:43 PM, Asim R P wrote: > Please find attached a patch to mark a shared buffer as read-write or > read-only using mprotect(). The idea is to catch violations of shared > buffer access rules. This patch was useful to detect the access > violations reported in this thread. The mprotect() calls are enabled > by -DMPROTECT_BUFFER compile time flag. I wonder if it would be a better idea to enable Valgrind's memcheck to mark buffers as read-only or read-write. We've considered doing something like that for years, but for whatever reason nobody followed through. Using Valgrind would have the advantage of making it possible to mark memory as undefined or as noaccess. I can imagine doing even fancier things by making that distinction within buffers. For example, marking the hole in the middle of a page/buffer as undefined, while still marking the entire buffer noaccess when there is no pin held. -- Peter Geoghegan
Re: Shared buffer access rule violations?
Please find attached a patch to mark a shared buffer as read-write or
read-only using mprotect(). The idea is to catch violations of shared
buffer access rules. This patch was useful to detect the access
violations reported in this thread. The mprotect() calls are enabled
by -DMPROTECT_BUFFER compile time flag.
Asim
From ed7dcf633600b3a527ed52ffacd1b779da8b0235 Mon Sep 17 00:00:00 2001
From: Asim R P
Date: Wed, 18 Jul 2018 18:32:40 -0700
Subject: [PATCH 1/2] Facility to detect shared buffer access violations
Using mprotect() to allow/disallow read/write access to one or more
shared buffers, this patch enables detection of shared buffer access
violations. A new compile time flag "MPROTECT_BUFFERS"
(CFLAGS=-DMPROTECT_BUFFERS) is introduced to enable the detection.
A couple of violations have already been caught and fixed using this
facility: 130beba36d6dd46b8c527646f9f2433347cbfb11
---
src/backend/storage/buffer/buf_init.c | 31 +++
src/backend/storage/buffer/bufmgr.c | 122 ++
src/backend/storage/ipc/shmem.c | 18
3 files changed, 153 insertions(+), 18 deletions(-)
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index 144a2cee6f..22e3d2821c 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -14,6 +14,11 @@
*/
#include "postgres.h"
+#ifdef MPROTECT_BUFFERS
+#include
+#include "miscadmin.h"
+#endif
+
#include "storage/bufmgr.h"
#include "storage/buf_internals.h"
@@ -24,6 +29,28 @@ LWLockMinimallyPadded *BufferIOLWLockArray = NULL;
WritebackContext BackendWritebackContext;
CkptSortItem *CkptBufferIds;
+#ifdef MPROTECT_BUFFERS
+/*
+ * Protect the entire shared buffers region such that neither read nor write
+ * is allowed. Protection will change for specific buffers when accessed
+ * through buffer manager's interface. The intent is to catch violation of
+ * buffer access rules.
+ */
+static void
+ProtectMemoryPoolBuffers()
+{
+ Size bufferBlocksTotalSize = mul_size((Size)NBuffers, (Size) BLCKSZ);
+ if (IsUnderPostmaster && IsNormalProcessingMode() &&
+ mprotect(BufferBlocks, bufferBlocksTotalSize, PROT_NONE))
+ {
+ ereport(ERROR,
+(errmsg("unable to set memory level to %d, error %d, "
+ "allocation size %ud, ptr %ld", PROT_NONE,
+ errno, (unsigned int) bufferBlocksTotalSize,
+ (long int) BufferBlocks)));
+ }
+}
+#endif
/*
* Data Structures:
@@ -149,6 +176,10 @@ InitBufferPool(void)
/* Initialize per-backend file flush context */
WritebackContextInit(,
_flush_after);
+
+#ifdef MPROTECT_BUFFERS
+ ProtectMemoryPoolBuffers();
+#endif
}
/*
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 01eabe5706..2ef3c75b6a 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -31,6 +31,9 @@
#include "postgres.h"
#include
+#ifdef MPROTECT_BUFFERS
+#include
+#endif
#include
#include "access/xlog.h"
@@ -177,6 +180,75 @@ static PrivateRefCountEntry *GetPrivateRefCountEntry(Buffer buffer, bool do_move
static inline int32 GetPrivateRefCount(Buffer buffer);
static void ForgetPrivateRefCountEntry(PrivateRefCountEntry *ref);
+#ifdef MPROTECT_BUFFERS
+#define ShouldMemoryProtect(buf) (IsUnderPostmaster && \
+ IsNormalProcessingMode() && \
+ !BufferIsLocal(buf->buf_id+1) && \
+ !BufferIsInvalid(buf->buf_id+1))
+
+static inline void BufferMProtect(volatile BufferDesc *bufHdr, int protectionLevel)
+{
+ if (ShouldMemoryProtect(bufHdr))
+ {
+ if (mprotect(BufHdrGetBlock(bufHdr), BLCKSZ, protectionLevel))
+ {
+ ereport(ERROR,
+ (errmsg("unable to set memory level to %d, error %d, "
+ "block size %d, ptr %ld", protectionLevel,
+ errno, BLCKSZ, (long int) BufHdrGetBlock(bufHdr;
+ }
+ }
+}
+#endif
+
+static inline void ReleaseContentLock(volatile BufferDesc *buf)
+{
+ LWLockRelease(BufferDescriptorGetContentLock(buf));
+
+#ifdef MPROTECT_BUFFERS
+ /* make the buffer read-only after releasing content lock */
+ if (!LWLockHeldByMe(BufferDescriptorGetContentLock(buf)))
+ BufferMProtect(buf, PROT_READ);
+#endif
+}
+
+
+static inline void AcquireContentLock(volatile BufferDesc *buf, LWLockMode mode)
+{
+#ifdef MPROTECT_BUFFERS
+ const bool newAcquisition =
+ !LWLockHeldByMe(BufferDescriptorGetContentLock(buf));
+
+ LWLockAcquire(BufferDescriptorGetContentLock(buf), mode);
+
+ /* new acquisition of content lock, allow read/write memory access */
+ if (newAcquisition)
+ BufferMProtect(buf, PROT_READ|PROT_WRITE);
+#else
+ LWLockAcquire(BufferDescriptorGetContentLock(buf), mode);
+#endif
+}
+
+static inline bool ConditionalAcquireContentLock(volatile BufferDesc *buf,
+ LWLockMode mode)
+{
+#ifdef MPROTECT_BUFFERS
+ const bool newAcquisition =
+ !LWLockHeldByMe(BufferDescriptorGetContentLock(buf));
+
+ if (LWLockConditionalAcquire(BufferDescriptorGetContentLock(buf), mode))
+ {
+ /* new
Re: Shared buffer access rule violations?
Asim R P writes:
> On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane wrote:
>> Asim R P writes:
>>> One can find several PageInit() calls with no content lock held. See,
>>> for example:
>>> fill_seq_with_data()
>> That would be for a relation that no one else can even see yet, no?
> Yes, when the sequence is being created. No, when the sequence is
> being reset, in ResetSequence().
ResetSequence creates a new relfilenode, which no one else will be able
to see until it commits, so the case is effectively the same as for
creation.
>>> vm_readbuf()
>>> fsm_readbuf()
>> In these cases I'd imagine that the I/O completion interlock is what
>> is preventing other backends from accessing the buffer.
> What is I/O completion interlock?
Oh ... the RBM_ZERO_ON_ERROR action should be done under the I/O lock,
but the ReadBuffer caller isn't holding that lock anymore, so I see your
point here. Probably, nobody's noticed because it's a corner case that
shouldn't happen under normal use, but it's not safe. I think what we
want is more like
if (PageIsNew(BufferGetPage(buf)))
{
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
if (PageIsNew(BufferGetPage(buf)))
PageInit(BufferGetPage(buf), BLCKSZ, 0);
UnlockReleaseBuffer(buf);
}
to ensure that the page is initialized once and only once, even if
several backends do this concurrently.
regards, tom lane
Re: Shared buffer access rule violations?
On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane wrote: > Asim R P writes: > >> One can find several PageInit() calls with no content lock held. See, >> for example: > >> fill_seq_with_data() > > That would be for a relation that no one else can even see yet, no? Yes, when the sequence is being created. No, when the sequence is being reset, in ResetSequence(). > >> vm_readbuf() >> fsm_readbuf() > > In these cases I'd imagine that the I/O completion interlock is what > is preventing other backends from accessing the buffer. > What is I/O completion interlock? I see no difference in initializing a visimap/fsm page and initializing a standard heap page. For standard heap pages, the code currently acquires the buffer pin as well as content lock for initialization. >> Moreover, fsm_vacuum_page() performs >> "PageGetContents(page))->fp_next_slot = 0;" without content lock. > > That field is just a hint, IIRC, and the possibility of a torn read > is explicitly not worried about. Yes, that's a hint. And ignoring torn page possibility doesn't result in checksum failures because fsm_read() passes RMB_ZERO_ON_ERROR to buffer manager. The page will be zeroed out in the event of checksum failure. Asim
Re: Shared buffer access rule violations?
Asim R P writes: > In order to make changes to a shared buffer, one must hold a pin on it > and the content lock in exclusive mode. This rule seems to be > followed in most of the places but there are a few exceptions. > One can find several PageInit() calls with no content lock held. See, > for example: > fill_seq_with_data() That would be for a relation that no one else can even see yet, no? > vm_readbuf() > fsm_readbuf() In these cases I'd imagine that the I/O completion interlock is what is preventing other backends from accessing the buffer. > Moreover, fsm_vacuum_page() performs > "PageGetContents(page))->fp_next_slot = 0;" without content lock. That field is just a hint, IIRC, and the possibility of a torn read is explicitly not worried about. > There may be more but I want to know if these can be treated as > violations before moving ahead. These specific things don't sound like bugs, though possibly I'm missing something. Perhaps more comments would be in order. regards, tom lane
