Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibCrypto: Add changable CRC32 polynomial #24365

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MINAqwq
Copy link
Contributor

@MINAqwq MINAqwq commented May 18, 2024

Since I am currently writing an OGG library and OGG Pages use crc32 checksums with its own polynomial, I have adapted the crc32 implementation to the crc16 and crc8 implementations.

Because the polynomial is now implemented via a template I had to throw everything from the cpp file into the header to avoid a fight with the linker.

What I'm wondering now is what happens on ARM CPUs, since their built in functions don't take an external polynomial. I just left it as it was before, since I'm not that familiar with this area.

I used the cksum utility for testing and the results on the current master and my branch where the same.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 18, 2024
@nico
Copy link
Contributor

nico commented May 18, 2024

If it doesn't fit neatly (sounds like it doesn't?), maybe LibCrypto should just do the normal ethernet crc32 and the ogg crc32 polynomial could be with the ogg code? It's not like CRC32 is a ton of code.

(No strong opinion, though.)

Comment on lines +57 to +84
void CRC32<polynomial>::update(ReadonlyBytes span)
{
// FIXME: Does this require runtime checking on rpi?
// (Maybe the instruction is present on the rpi4 but not on the rpi3?)

u8 const* data = span.data();
size_t size = span.size();

while (size > 0 && (reinterpret_cast<FlatPtr>(data) & 7) != 0) {
m_state = __builtin_arm_crc32b(m_state, *data);
++data;
--size;
}

auto* data64 = reinterpret_cast<u64 const*>(data);
while (size >= 8) {
m_state = __builtin_arm_crc32d(m_state, *data64);
++data64;
size -= 8;
}

data = reinterpret_cast<u8 const*>(data64);
while (size > 0) {
m_state = __builtin_arm_crc32b(m_state, *data);
++data;
--size;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be used for the 0x04C11DB7 (ethernet, mpeg2, png, gzip, SATA) polynomial
https://developer.arm.com/documentation/dui0801/h/A64-General-Instructions/CRC32B--CRC32H--CRC32W--CRC32X
There are also instructions for the 0x1EDC6F41 (iSCSI, ext4, btrfs) polynomial
https://developer.arm.com/documentation/dui0801/h/A64-General-Instructions/CRC32CB--CRC32CH--CRC32CW--CRC32CX
Which is also the one SSE4.2 provides

For that you can use partial specialization of the template

@MINAqwq
Copy link
Contributor Author

MINAqwq commented May 18, 2024

If it doesn't fit neatly (sounds like it doesn't?), maybe LibCrypto should just do the normal ethernet crc32 and the ogg crc32 polynomial could be with the ogg code? It's not like CRC32 is a ton of code.

(No strong opinion, though.)

Yeah, you could be right.
I'll leave the commit here and build the whole ogg fun without crc for now. Maybe someone else has an idea how to implement this in a sane way.

@Poldraunic
Copy link

Now that polynomial is accessible all the time (due to it being a template argument) and is defaulted to Polynomial::Ethernet would it be possible to make it a member variable? This would allow to keep implementation in cpp file still. Class interface could look something like this:

namespace KnownPolynomials {
static constexpr u32 Ethernet = 0xEDB88320;
static constexpr u32 Ogg = 0x04C11DB7;
};

class CRC32 : public ChecksumFunction<u32> {
public:
    CRC32() = default;
    explicit CRC32(ReadonlyBytes data, u32 polynomial = KnownPolynomials::Ethernet)

private:
    u32 m_polynomial;
}

(I am a long time lurker without any contributions, excuse me if this is out of line. It just felt like a rare moment where I could offer a suggestion.)

@MINAqwq
Copy link
Contributor Author

MINAqwq commented May 24, 2024

Now that polynomial is accessible all the time (due to it being a template argument) and is defaulted to Polynomial::Ethernet would it be possible to make it a member variable? This would allow to keep implementation in cpp file still. Class interface could look something like this:

namespace KnownPolynomials {
static constexpr u32 Ethernet = 0xEDB88320;
static constexpr u32 Ogg = 0x04C11DB7;
};

class CRC32 : public ChecksumFunction<u32> {
public:
    CRC32() = default;
    explicit CRC32(ReadonlyBytes data, u32 polynomial = KnownPolynomials::Ethernet)

private:
    u32 m_polynomial;
}

(I am a long time lurker without any contributions, excuse me if this is out of line. It just felt like a rare moment where I could offer a suggestion.)

Thanks for your suggestion :3

Then it would differ from the crc16 and crc8 implementations and a lot of code parts could not be calculated at compile time. If you use a template it will create specific functions for every used template argument variation, what makes this optimization thing easier.

Comment on lines +16 to +17
static constexpr u32 Ethernet = 0xEDB88320;
static constexpr u32 Ogg = 0x04C11DB7;
Copy link
Member

@timschumi timschumi Jun 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, the "more accurate" name for these would be CRC32Reversed and CRC32Normal (the CRC32 being in contrast to CRC32C, CRC32K, etc.). We can put the actual applications as a comment if we want, but we probably shouldn't put them into the name like that to avoid confusion.

I'm not sure if we actually want to act on that, just for consideration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants