Message-ID: <86162cd5-8d5e-4f75-94e0-842684cd432a@intel.com>
Date: Fri, 23 Aug 2024 12:53:35 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Vladimir Oltean <olteanv@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: lib/packing.c behaving weird if buffer length is not multiple of
 4 with QUIRK_LSW32_IS_FIRST



On 8/22/2024 6:41 PM, Jacob Keller wrote:
> 
> 
> On 8/21/2024 4:41 PM, Jacob Keller wrote:
>>
>>
>> On 8/21/2024 1:21 PM, Vladimir Oltean wrote:
>>> On Wed, Aug 21, 2024 at 12:12:00PM -0700, Jacob Keller wrote:
>>>> Ok. I'll investigate this, and I will send the two fixes for lib/packing
>>>> in my series to implement the support in ice. That would help on our end
>>>> with managing the changes since it avoids an interdependence between
>>>> multiple series in flight.
>>>
>>> There's one patch in there which replaces the packing(PACK) call with a
>>> dedicated pack() function, and packing(UNPACK) with unpack(). The idea
>>> being that it helps with const correctness. I still have some mixed
>>> feelings about this, because a multiplexed packing() call is in some
>>> ways more flexible, but apparently others felt bad enough about the
>>> packing() API to tell me about it, and that stuck with me.
>>>
>>> I'm mentioning it because if you're going to use the API, you could at
>>> least consider using the const-correct form, so that there's one less
>>> driver to refactor later.
>>
>> Yep! I've got those patches in my series now. Though I should note that
>> I did not include any of the patches for the other drivers. I'll CC you
>> when I send the series out, though it may likely go through our
>> Intel-Wired-LAN tree first.
>>
>> I've refactored your self tests into KUnit tests as well!
>>
> 
> I was writing additional tests and I think I ran into another issue with
> QUIRK_MSB_ON_THE_RIGHT, when the bit offsets are not aligned to a byte
> boundary:
> 
> When trying to unpack 0x1122334455667788 from the buffer between offsets
> 106-43, the calculation appears to completely break.
> 
> When packing:
> 
>> [18:34:50] box_bit_width = 3
>> [18:34:50] box_start_bit = 2
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 2
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 5
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 3
>> [18:34:50] new_box_start_bit = 1
>> [18:34:50] new_box_end_bit = -3
>> [18:34:50]     # packing_test_unpack: EXPECTATION FAILED at lib/packing_test.c:264
>> [18:34:50]     Expected uval == params->uval, but
>> [18:34:50]         uval == 1234605616436508544 (0x1122334455667780)
>> [18:34:50]         params->uval == 1234605616436508552 (0x1122334455667788)
>> [18:34:50] [FAILED] msb right, 16 bytes, non-aligned
>> [18:34:50] # packing_test_unpack: pass:19 fail:1 skip:0 total:20
> 
> Notice that the box end bit is now negative. Specifically this is
> because the width is smaller than the start bit, so subtraction underflows.
> 
> When unpacking:
>> [18:34:50] box_bit_width = 3
>> [18:34:50] box_start_bit = 2
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 2
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 8
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 0
>> [18:34:50] new_box_start_bit = 7
>> [18:34:50] new_box_end_bit = 0
>> [18:34:50] box_bit_width = 5
>> [18:34:50] box_start_bit = 7
>> [18:34:50] box_end_bit = 3
>> [18:34:50] new_box_start_bit = 1
>> [18:34:50] new_box_end_bit = -3
>> [18:34:50]     # packing_test_unpack: EXPECTATION FAILED at lib/packing_test.c:264
>> [18:34:50]     Expected uval == params->uval, but
>> [18:34:50]         uval == 1234605616436508544 (0x1122334455667780)
>> [18:34:50]         params->uval == 1234605616436508552 (0x1122334455667788)
>> [18:34:50] [FAILED] msb right, 16 bytes, non-aligned
>> [18:34:50] # packing_test_unpack: pass:19 fail:1 skip:0 total:20
> 
> Specifically, it looks like we basically fail to calculate valid new box
> offsets.
> 
> What's weird to me is that when the box width is larger than the start
> bit position, we just calculate the same exact offsets, so I don't see
> why the existing calculations are there at all. Something is obviously
> wrong here.
> 

Specifically this is making me question:

Does QUIRK_MSB_ON_THE_RIGHT mean that the msb of each bit field is on
the right? or does it mean that every byte in the buffer has its 8 bits
flipped? It seems to document that it applies to the bits within the
byte, but not to the byte ordering.

The code seems to be trying to do a mix of different things though, and
my attempts at fixing it haven't worked properly yet.

I think I was able to get pack() to behave correctly, but unpacking
seems to still be erratic. I tried adding a few more tests but so far
haven't figured out what is wrong with the unpacking code.

I think the attempt to re-use adjust_for_msb_right on both unpacking and
packing isn't working correctly. When unpacking, I think we end up
masking the wrong bits.

With the attached patch, I was able to get my test case for packing
fixed, but the following couple of tests fail:

>         {
>                 .desc = "msb right, 16 bytes, non-aligned",
>                 PBUF(0x00, 0x00, 0x00, 0x91, 0x88, 0x59, 0x44, 0xd5,
>                      0xcc, 0x3d, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00),
>                 .uval = 0x1122334455667788,
>                 .start_bit = 106,
>                 .end_bit = 43,
>                 .quirks = QUIRK_MSB_ON_THE_RIGHT,
>         },

packing for this test works with the attached patch, but fails to unpack:

 > [12:49:53]     # packing_test_unpack: EXPECTATION FAILED at
lib/packing_test.c:282
> [12:49:53]     Expected uval == params->uval, but
> [12:49:53]         uval == 1234605616436508544 (0x1122334455667780)
> [12:49:53]         params->uval == 1234605616436508552 (0x1122334455667788)


the last few bits don't seem to be included properly.

I also tried a test case with all bits of the u64 set:

>         {
>                 .desc = "msb right, 16 bytes, non-aligned, 0xff",
>                 PBUF(0x00, 0x00, 0xe0, 0xff, 0xff, 0xff, 0xff, 0xff,
>                      0xff, 0xff, 0x1f, 0x00, 0x00, 0x00, 0x00, 0x00),
>                 .uval = 0xffffffffffffffff,
>                 .start_bit = 106,
>                 .end_bit = 43,
>                 .quirks = QUIRK_MSB_ON_THE_RIGHT,
>         },


> [12:49:53]     # packing_test_unpack: EXPECTATION FAILED at lib/packing_test.c:282
> [12:49:53]     Expected uval == params->uval, but
> [12:49:53]         uval == 2305843009213693944 (0x1ffffffffffffff8)
> [12:49:53]         params->uval == -1 (0xffffffffffffffff)

In this case, again it seems like the bits on the tail end partial bytes
don't get unpacked properly.

This test also passes packing with the attempted fix.

I so far think the likely issue is with the way we handle the box and
mask offsets. Somehow we must be shifting things in a strange way that
causes us to discard bits, but only when we deal with the
QUIRK_MSB_ON_THE_RIGHT.

Thanks,
Jake
View attachment "maybe-fixed-packing.patch" of type "text/plain" (1388 bytes)

Powered by blists - more mailing lists