Skip to content

Commit

Permalink
phase in new CRC logic for loading (phase 1)
Browse files Browse the repository at this point in the history
Summary:
Transition to the correct way to compute the CRC32C checksum.

# Initial Value and Final XOR
The standard initial value for CRC32C is `0xFFFFFFFF` (not `0x04C11DB7`). Also the final XOR with `0xFFFFFFFF` was missing.

CRC32C uses the polynomial `0x1EDC6F41`. Using the standard CRC32's `0x04C11DB7` polynomial as the initial value does not set that to the checksum's polynomial.

# Argument Order
Both the built-ins `__builtin_ia32_crc32di` and `_mm_crc32_u64` take (crc, v) not (v, crc). The high 32-bits from the 64-bit first argument will be ignored, which means we're not verifying those bits.

```
fbclock_crc64(0x12345678, 0xffffffff) = 0x399d2062
fbclock_crc64(0xffffffff, 0x12345678) = 0x399d2062
fbclock_crc64(0x1234567812345678, 0xffffffff) = 0x399d2062
fbclock_crc64(0xffffffff, 0x1234567812345678) = 0xc3e97656
```

This means we need to swap the order of the arguments for `fbclock_crc64`.

# Rollout
To roll this fix out, we need to do it in three phases. Each phase must be fully rolled out before the next one can land.
1. Update loading to ensure all logic reading the CRC accepts both the old and new one.
2. Update storing to ensure all logic storing the CRC is using the new one.
3. Removed the deprecated second CRC check on loading.

NOTE: This phase must be fully rolled out before landing the next phase.

Reviewed By: abulimov

Differential Revision: D43413687

fbshipit-source-id: 7145fe8ed1b3f19e0dd91ffee0495a2511e1e305
  • Loading branch information
Greg Daniel authored and facebook-github-bot committed Jan 30, 2024
1 parent 381ca61 commit 0db5072
Showing 1 changed file with 20 additions and 2 deletions.
22 changes: 20 additions & 2 deletions fbclock/fbclock.c
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,33 @@ struct phc_time_res {
int64_t delay; // mean delay of several requests
};

static inline uint64_t fbclock_clockdata_crc(fbclock_clockdata* value) {
static inline uint64_t fbclock_clockdata_crc_DEPRECATED(
fbclock_clockdata* value) {
// TODO: Remove deprecated function after phase is rolled out.
uint64_t counter = fbclock_crc64(value->ingress_time_ns, 0x04C11DB7);
counter = fbclock_crc64(value->error_bound_ns, counter);
counter = fbclock_crc64(value->holdover_multiplier_ns, counter);
return counter;
}

static inline uint64_t fbclock_clockdata_crc(fbclock_clockdata* value) {
uint64_t counter = fbclock_crc64(0xFFFFFFFF, value->ingress_time_ns);
counter = fbclock_crc64(counter, value->error_bound_ns);
counter = fbclock_crc64(counter, value->holdover_multiplier_ns);
return counter ^ 0xFFFFFFFF;
}

// fbclock_clockdata_store_data is used in shmem.go to store timing data
int fbclock_clockdata_store_data(uint32_t fd, fbclock_clockdata* data) {
fbclock_shmdata* shmp = mmap(
NULL, FBCLOCK_SHMDATA_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
if (shmp == MAP_FAILED) {
return FBCLOCK_E_SHMEM_MAP_FAILED;
}
uint64_t crc = fbclock_clockdata_crc(data);
uint64_t crc = fbclock_clockdata_crc_DEPRECATED(data);
// TODO: Switch to storing using new crc function after all readers are
// updated.
// uint64_t crc = fbclock_clockdata_crc(data);
memcpy(&shmp->data, data, FBCLOCK_CLOCKDATA_SIZE);
atomic_store(&shmp->crc, crc);
munmap(shmp, FBCLOCK_SHMDATA_SIZE);
Expand All @@ -92,6 +104,12 @@ int fbclock_clockdata_load_data(
fbclock_debug_print("reading clock data took %d tries\n", i + 1);
return FBCLOCK_E_NO_ERROR;
}
// TODO: Remove deprecated block after phase is rolled out.
uint64_t our_crc_DEPRECATED = fbclock_clockdata_crc_DEPRECATED(data);
if (our_crc_DEPRECATED == crc) {
fbclock_debug_print("reading clock data took %d tries (old)\n", i + 1);
return FBCLOCK_E_NO_ERROR;
}
}
fbclock_debug_print(
"failed to read clock data after %d tries\n", FBCLOCK_MAX_READ_TRIES);
Expand Down

0 comments on commit 0db5072

Please sign in to comment.