Page MenuHomeFreeBSD

Fix alignment issues on MIPS
ClosedPublic

Authored by br on Sep 16 2016, 3:51 PM.
Tags
None
Referenced Files
F87167924: D7905.diff
Sun, Jun 30, 1:54 AM
F87101878: D7905.id21590.diff
Sat, Jun 29, 9:24 AM
Unknown Object (File)
Sun, Jun 23, 5:53 PM
Unknown Object (File)
May 25 2024, 9:29 PM
Unknown Object (File)
May 24 2024, 9:17 PM
Unknown Object (File)
May 8 2024, 1:52 PM
Unknown Object (File)
Apr 13 2024, 9:34 PM
Unknown Object (File)
Apr 13 2024, 8:48 PM
Subscribers

Details

Summary

MIPS required the pointers to be aligned

Test Plan

5520 GEOM_ELI tests passed successfully

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

br retitled this revision from to Fix alignment issues on MIPS.
br updated this object.
br edited the test plan for this revision. (Show Details)
br added a reviewer: pjd.

I think we should commit this, with appropriate #ifdef guards. Maybe __mips_n64?

sys/geom/eli/g_eli.h
251

This scares me. Seems like you'd be changing the size of the metadata on the disk if you did this.

sys/geom/eli/g_eli_integrity.c
448

sizeof(uintptr_t) maybe?

456

what's wrong with the roundup macro?

sys/geom/eli/g_eli.h
251

This is why I said above in #ifdef guards, and wondering about an appropriate macro to use. Because of this geli does not work today on mips64, so we can either end up with a new on-disk geli format for mips64 or have geli explicitly not supported on mips64.

sys/geom/eli/g_eli.h
251

So md5 requires that we hash into an aligned buffer?

Do we have the same issue with ARM?

I think we should commit this, with appropriate #ifdef guards. Maybe __mips_n64?

probably __mips_n64 || __mips_o64

include Makefile change to allow module build

Longer term, I think we should see if we can bump G_ELI_VERSION and apply this change on all architectures, in addition to switching away from md5.

sys/geom/eli/g_eli.h
251

Yes md5 requires aligned md_hash. Not sure about ARM

sys/geom/eli/g_eli.h
251

Alternately we could memcpy the md_hash in.

yes, you right.
it looks like per-byte copy works. I'll try run all the tests

sys/geom/eli/g_eli.h
359

FYI I would expect that alignment of at least 4 is mandated by all of our ABIs.

sbin/geom/class/eli/geom_eli.c
670

I'm slightly surprised we'd end up with a misaligned sector[], even if it's not explicitly guaranteed. However, if we do need to explicitly specify alignment there should be no problem (for this stack variable) doing it for all architectures.

sbin/geom/class/eli/geom_eli.c
670

We should do it for all architectures. There's no harm, and it may help.

sys/geom/eli/g_eli.h
359

Why not make this an array of uint32_t?

sys/geom/eli/g_eli_integrity.c
448

We can do this for all archs. There's no harm.

456

Ditto.

But honestly, the kernel malloc should returned a properly aligned buffer. Have you verified that it actually doesn't?

sbin/geom/class/eli/geom_eli.c
670

Probably something like __aligned(__alignof(struct g_eli_metadata))

sbin/geom/class/eli/geom_eli.c
670

__alignof(struct g_eli_metadata) is 1, but we need 4

sys/geom/eli/g_eli_integrity.c
456

p is adjusted pointer:

if (bp->bio_cmd == BIO_READ) {
    data = bp->bio_driver2;
    p = auth + sc->sc_alen * nsec;
} else {
    data = malloc(size, M_ELI, M_WAITOK);
    p = data + encr_secsize * nsec;
}
p = (char *)roundup((uintptr_t)p, sizeof(uintptr_t));

Sometimes p comes not aligned after this.

This revision was automatically updated to reflect the committed changes.