Shared buffer access rule violations?

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Shared buffer access rule violations?

Asim R P
Hi,

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()
vm_readbuf()
fsm_readbuf()

Moreover, fsm_vacuum_page() performs
"PageGetContents(page))->fp_next_slot = 0;" without content lock.

There may be more but I want to know if these can be treated as
violations before moving ahead.

Asim

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Tom Lane-2
Asim R P <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Asim R P
On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane <[hidden email]> wrote:
> Asim R P <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Tom Lane-2
Asim R P <[hidden email]> writes:
> On Tue, Jul 10, 2018 at 8:33 PM, Tom Lane <[hidden email]> wrote:
>> Asim R P <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Asim R P
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

0001-Facility-to-detect-shared-buffer-access-violations.patch (15K) Download Attachment
0002-Fix-known-but-not-problematic-buffer-access-violatio.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Peter Geoghegan-4
On Tue, Aug 7, 2018 at 6:43 PM, Asim R P <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Asim R P
On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Shared buffer access rule violations?

Thomas Munro-3
On Thu, Aug 9, 2018 at 12:59 PM Asim R P <[hidden email]> wrote:

> On Tue, Aug 7, 2018 at 7:00 PM, Peter Geoghegan <[hidden email]> 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