Thread: Shared buffer access rule violations?
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
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
Attachment
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