Shared buffer access rule violations?

Asim R P <apraveen@pivotal.io> 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

On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Asim R P <apraveen@pivotal.io> 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

Asim R P <apraveen@pivotal.io> writes:

On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Asim R P <apraveen@pivotal.io> 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

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

On Tue, Aug 7, 2018 at 6:43 PM, Asim R P <apraveen@pivotal.io> 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

On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan <pg@bowt.ie> 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

On Thu, Aug 9, 2018 at 12:59 PM Asim R P <apraveen@pivotal.io> wrote:

On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan <pg@bowt.ie> 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

Thomas Munro <thomas.munro@enterprisedb.com> writes:

On Thu, Aug 9, 2018 at 12:59 PM Asim R P <apraveen@pivotal.io> wrote:

On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan <pg@bowt.ie> 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

Hi,

On 2019-01-13 19:52:32 -0500, Tom Lane wrote:

Thomas Munro <thomas.munro@enterprisedb.com> writes:

On Thu, Aug 9, 2018 at 12:59 PM Asim R P <apraveen@pivotal.io> wrote:

On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan <pg@bowt.ie> 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