On 11/8/2024 3:24 AM, Vladimir Oltean wrote:
> On Thu, Nov 07, 2024 at 11:50:34AM -0800, Jacob Keller wrote:
>> +#define DECLARE_PACKED_FIELDS_S(name, buffer_sz) \
>> + const size_t __ ## name ## _buffer_sz __pf_section_s = buffer_sz; \
>> + const struct packed_field_s name[] __pf_section_s
>
> This will need sorting out - how to make this declaration macro
> compatible with the "static" keyword. The obvious way (to group the
> field array and the buffer size into a structure) doesn't work. It loses
> the ARRAY_SIZE() of the fields, which is important to the pack_fields()
> and unpack_fields() wrappers.
>
Yea. I didn't see any static warnings on my setup but i forgot to check
with sparse.
> Maybe a different tool is needed for checking that no packed fields
> exceed the buffer size? Forcing the buffer size be a symbol just because
> that's what modpost works with seems unnatural.
>
True, I could move that check to a different spot.
> If we knew the position of the largest field array element in C, and if
> we enforced that all pack_fields() use a strong type (size included) for
> the packed buffer, we'd have all information inside the pack_fields()
> macro, because we only need to compare the largest field against the
> buffer size. We could just have that part as a BUILD_BUG_ON() wrapped
> inside the pack_fields() macro itself. And have the compile-time checks
> spill over between C and modpost...
>
I think thats reasonable.
> Not to mention, pack_fields() would have one argument less: pbuflen.
>
Yea, I think enforcing the sized type like that via structure is a
reasonable restriction.
>> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
>> index ada3a36cc4bc..013bf4be2642 100644
>> --- a/scripts/mod/modpost.h
>> +++ b/scripts/mod/modpost.h
>> @@ -160,6 +160,19 @@ static inline bool is_valid_name(struct elf_info *elf, Elf_Sym *sym)
>> return !is_mapping_symbol(name);
>> }
>>
>> +/* This is based on the hash algorithm from gdbm, via tdb */
>> +static inline unsigned int tdb_hash(const char *name)
>> +{
>> + unsigned value; /* Used to compute the hash value. */
>> + unsigned i; /* Used to cycle through random values. */
>> +
>> + /* Set the initial value from the key size. */
>> + for (value = 0x238F13AF * strlen(name), i = 0; name[i]; i++)
>> + value = (value + (((unsigned char *)name)[i] << (i*5 % 24)));
>> +
>> + return (1103515243 * value + 12345);
>> +}
>> +
>
> Code movement should be in separate changes.
>
Sure.
>> diff --git a/lib/packing_test.c b/lib/packing_test.c
>> index b38ea43c03fd..ff5be660de01 100644
>> --- a/lib/packing_test.c
>> +++ b/lib/packing_test.c
>
> I appreciate the test.
>
Yea. I figured the addition of a test is good, though I didn't see the
need to compound it with tests for every combination of the flags, since
we cover those with pack() and unpack() tests.
>> @@ -396,9 +396,70 @@ static void packing_test_unpack(struct kunit *test)
>> KUNIT_EXPECT_EQ(test, uval, params->uval);
>> }
>>
>> +#define PACKED_BUF_SIZE 8
>> +
>> +typedef struct __packed { u8 buf[PACKED_BUF_SIZE]; } packed_buf_t;
>> +
>> +struct test_data {
>> + u8 field1;
>> + u16 field2;
>> + u32 field3;
>> + u16 field4;
>> + u8 field5;
>> + u16 field6;
>
> If you group the u8s with the u8s and with the odd u16, and the
> remaining two u16s together, you should get a structure with less
> padding.
>
I kept them as ordered by the order they appear in the packed data, but
yes re-ordering is safe, and does safe a few bytes.
>> +};
>> +
>> +DECLARE_PACKED_FIELDS_S(test_fields, sizeof(packed_buf_t)) = {
>> + PACKED_FIELD(63, 61, struct test_data, field1),
>> + PACKED_FIELD(60, 52, struct test_data, field2),
>> + PACKED_FIELD(51, 28, struct test_data, field3),
>> + PACKED_FIELD(27, 14, struct test_data, field4),
>> + PACKED_FIELD(13, 9, struct test_data, field5),
>> + PACKED_FIELD(8, 0, struct test_data, field6),
>> +};
>> +
>> +static void packing_test_pack_fields(struct kunit *test)
>> +{
>> + const struct test_data data = {
>> + .field1 = 0x2,
>> + .field2 = 0x100,
>> + .field3 = 0xF00050,
>> + .field4 = 0x7D3,
>> + .field5 = 0x9,
>> + .field6 = 0x10B,
>> + };
>> + packed_buf_t buf = {};
>
> Reverse Christmas tree (should come after "expect").
>
Good point. I'll fix these.
>> +enum element_order {
>> + FIRST_ELEMENT,
>> + SECOND_ELEMENT,
>> + ASCENDING_ORDER,
>> + DESCENDING_ORDER,
>> +};
>
> If you still keep this for next versions, this should go at the top,
> before function definitions.
>
Sure.
>> +
>> +static void check_packed_field_array(const struct field_symbol *f)
>> +{
>> + struct packed_field_elem previous_elem = {};
>> + size_t field_size = field_type_to_size(f->type);
>> + enum element_order order = FIRST_ELEMENT;
>> + void *data_ptr;
>> + int count;
>
> Stored as signed, printed as unsigned.
>
>> +
>> + /* check that the data is a multiple of the size */
>> + if (f->data_size % field_size != 0) {
>> + error("symbol %s of module %s has size %zu which is not a multiple of the field size (%zu)\n",
>> + f->name, f->mod->name, f->data_size, field_size);
>> + return;
>> + }
>> +
>> + data_ptr = f->data;
>> + count = 0;
>
> Initialization be wrapped into the declaration.
>
>> +
>> + while (data_ptr < f->data + f->data_size) {
>> + struct packed_field_elem elem = {};
>> +
>> + get_field_contents(data_ptr, f->type, &elem);
>> +
>> + if (elem.startbit < elem.endbit)
>> + error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") must be larger than endbit (%" PRIu64 ")\n",
>> + f->name, f->mod->name, count, elem.startbit,
>> + elem.endbit);
>> +
>> + if (elem.startbit >= BITS_PER_BYTE * f->buffer_size)
>> + error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") puts field outsize of the packed buffer size (%" PRIu64 ")\n",
>> + f->name, f->mod->name, count, elem.startbit,
>> + f->buffer_size);
>> +
>> + if (elem.startbit - elem.endbit >= BITS_PER_BYTE * elem.size)
>> + error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") and endbit (%" PRIu64 ") indicate a field of width (%" PRIu64 ") which does not fit into the field size (%" PRIu64 ")\n",
>> + f->name, f->mod->name, count, elem.startbit,
>> + elem.endbit, elem.startbit - elem.endbit,
>> + elem.size);
>
> elem.size is in bytes but the field width is in bits. The error message
> is confusing because it's not clear what is wrong.
>
True, I can convert everything to bits.
>> +
>> + if (elem.size != 1 && elem.size != 2 && elem.size != 4 && elem.size != 8)
>> + error("\"%s\" [%s.ko] element %u size (%" PRIu64 ") must be 1, 2, 4, or 8\n",
>> + f->name, f->mod->name, count, elem.size);
>> +
>> + switch (order) {
>> + case FIRST_ELEMENT:
>> + order = SECOND_ELEMENT;
>> + break;
>> + case SECOND_ELEMENT:
>> + order = previous_elem.startbit < elem.startbit ?
>> + ASCENDING_ORDER : DESCENDING_ORDER;
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + switch (order) {
>> + case ASCENDING_ORDER:
>
> I don't see why ASCENDING_ORDER and DESCENDING_ORDER are handled as part
> of a different switch/case statement.
>
I wanted to check both start/end bit at the SECOND_FIELD case, because I
had naively assumed i was covering overlap with that... :D Should have
thought through it more.
>> + if (previous_elem.startbit >= elem.startbit)
>> + error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element startbit is %" PRIu64 "\n",
>> + f->name, f->mod->name, count,
>> + elem.startbit, previous_elem.startbit);
>> + if (previous_elem.endbit >= elem.endbit)
>> + error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in ascending order, but previous element endbit is %" PRIu64 "\n",
>> + f->name, f->mod->name, count, elem.endbit,
>> + previous_elem.endbit);
>> +
>> + break;
>> + case DESCENDING_ORDER:
>> + if (previous_elem.startbit <= elem.startbit)
>> + error("\"%s\" [%s.ko] element %u startbit (%" PRIu64 ") expected to be arranged in descending order, but previous element startbit is %" PRIu64 "\n",
>> + f->name, f->mod->name, count,
>> + elem.startbit, previous_elem.startbit);
>> + if (previous_elem.endbit <= elem.endbit)
>> + error("\"%s\" [%s.ko] element %u endbit (%" PRIu64 ") expected to be arranged in descending order, but previous element endbit is %" PRIu64 "\n",
>> + f->name, f->mod->name, count,
>> + elem.endbit, previous_elem.endbit);
>> + break;
>
> The end goal was never to enforce ascending or descending order of the
> start and end of the fields. The point was to enforce that the fields do
> not overlap, which this does _not_ do. The rule for detecting overlap of
> intervals [a, b] and [c, d] is that max(a, c) <= min(b, d).
>
I do think that keeping them ordered is a good thing too.
> Enforcing ascending/descending order is just a way of reducing the
> complexity of the overlap check from O(n^2) to O(n). But the overlap
> check itself is missing.
>
Yea, you're right.
PACKED_FIELD(64, 30, ...),
PACKED_FIELD(60, 24, ...),
which would pass the test, but which overlaps and is wrong.
If I put the overlap check to a check outside of the switch, then just
keep the ascending/descending checks in the same switch case we should
be correct?
