This repository has been archived by the owner. It is now read-only.

Correct broken unaligned load/store in armv5 #578

Merged
merged 1 commit into from Sep 6, 2016

Conversation

Projects
None yet
2 participants
@lorenzo-stoakes
Contributor

lorenzo-stoakes commented Jul 28, 2016

Description

armv5 devices and older (i.e. <= arm9 generation) require addresses that are stored to and loaded from to to be 4-byte aligned.

If this is not the case the lower 2 bits of the address are cleared and the load is performed in an unexpected order, including up to 3 bytes of data located prior to the address.

Inlined buckets are stored after their key in a page and since there is no guarantee that the key will be of a length that is a multiple of 4, it is possible for unaligned load/stores to occur when they are cast back to bucket and page pointer types.

The fix adds a new field to track whether the current architecture exhibits this issue, sets it on module load for ARM architectures, and then on bucket open, if this field is set and the address is unaligned, a byte-by-byte copy of the inlined bucket is performed.

Ref: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html

Testing

I've tested this fix on actual hardware (TS-7700) and run the full suite of tests there (albeit having to reduce some constants due to its limited RAM) - all passing.

Index out of range

Issues #327 and #430 may well be fixed by this, as the problem usually exhibits itself as an index out of range error.

This occurs because openBucket() dereferences a pointer a bucket that might not be aligned. Taking a look at the type:

type bucket struct {
    root     pgid   // page id of the bucket's root-level page
    sequence uint64 // monotonically incrementing, used by NextSequence()
}

The first field here is a page ID, which is a uint64 (declared as type pgid uint64.) When the unaligned access happens it pulls in garbage bytes from up to 3 bytes previous to the address and due to the odd rearrangement of the bytes this number tends to be very large.

Then, when the pgid is used to try to reference an address in memory (typically I've found this has occurred in page() in db.go), pgid * db.pageSize exceeds maxMapSize and hence the issue manifests as an out of range error:

type DB struct {
    ...
    data     *[maxMapSize]byte
    ...
}

...

// page retrieves a page reference from the mmap based on the current page size.
func (db *DB) page(id pgid) *page {
    pos := id * pgid(db.pageSize)
    return (*page)(unsafe.Pointer(&db.data[pos]))
}

The issue has consistently manifested this way for me prior to the fix.

See this go playground example for a repro + example of the reordering.

bucket: correct broken unaligned load/store in armv5
armv5 devices and older (i.e. <= arm9 generation) require addresses that are
stored to and loaded from to to be 4-byte aligned.

If this is not the case the lower 2 bits of the address are cleared and the load
is performed in an unexpected order, including up to 3 bytes of data located
prior to the address.

Inlined buckets are stored after their key in a page and since there is no
guarantee that the key will be of a length that is a multiple of 4, it is
possible for unaligned load/stores to occur when they are cast back to bucket
and page pointer types.

The fix adds a new field to track whether the current architecture exhibits this
issue, sets it on module load for ARM architectures, and then on bucket open, if
this field is set and the address is unaligned, a byte-by-byte copy of the
inlined bucket is performed.

Ref: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html
@lorenzo-stoakes

This comment has been minimized.

Contributor

lorenzo-stoakes commented Aug 17, 2016

hey @benbjohnson wondered if you had any thoughts on this? Any insight would be very much appreciated! :)

@benbjohnson

This comment has been minimized.

Member

benbjohnson commented Sep 5, 2016

@lorenzo-stoakes Sorry for the delay. I'm carving out time each week to do more bolt support. I will look at the change tomorrow and get back with you.

How is the change in balena-os/meta-balena#228 working?

@lorenzo-stoakes

This comment has been minimized.

Contributor

lorenzo-stoakes commented Sep 5, 2016

Hey @benbjohnson no problem, thanks very much for taking a look! The fix we merged in meta-resin seems to have resolved the issues we encountered with docker on armv5 altogether so it's looking good in practice for us!

Let me know if you have any feedback/questions/etc. about this patch!

@benbjohnson benbjohnson merged commit de82765 into boltdb:master Sep 6, 2016

@benbjohnson

This comment has been minimized.

Member

benbjohnson commented Sep 6, 2016

@lorenzo-stoakes Thanks for the good comments on there. Otherwise I'd probably forget what the heck some of that was doing in a month, haha. Merged! Thanks for digging into that issue. I like your fix.

@lorenzo-stoakes

This comment has been minimized.

Contributor

lorenzo-stoakes commented Sep 6, 2016

Great, thanks a lot @benbjohnson ! It was a subtle and odd bug and I am glad to help contribute :)

petrosagg added a commit to balena-io/docker that referenced this pull request Dec 1, 2016

bucket: correct broken unaligned load/store in armv5
armv5 devices and older (i.e. <= arm9 generation) require addresses that are
stored to and loaded from to to be 4-byte aligned.

If this is not the case the lower 2 bits of the address are cleared and the load
is performed in an unexpected order, including up to 3 bytes of data located
prior to the address.

Inlined buckets are stored after their key in a page and since there is no
guarantee that the key will be of a length that is a multiple of 4, it is
possible for unaligned load/stores to occur when they are cast back to bucket
and page pointer types.

The fix adds a new field to track whether the current architecture exhibits this
issue, sets it on module load for ARM architectures, and then on bucket open, if
this field is set and the address is unaligned, a byte-by-byte copy of the
inlined bucket is performed.

Ref: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html

Upstream-Status: Submitted boltdb/bolt#578
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.