linux-kernel - Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250129222147.GC2619178@google.com>
Date: Wed, 29 Jan 2025 22:21:47 +0000
From: Eric Biggers <ebiggers@...nel.org>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: Zhu Yanjun <yanjun.zhu@...ux.dev>, linux-rdma@...r.kernel.org,
	Mustafa Ismail <mustafa.ismail@...el.com>,
	Tatyana Nikolova <tatyana.e.nikolova@...el.com>,
	Leon Romanovsky <leon@...nel.org>,
	Zhu Yanjun <zyjzyj2000@...il.com>,
	Bernard Metzler <bmt@...ich.ibm.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/6] RDMA/rxe: handle ICRC correctly on big endian systems

On Wed, Jan 29, 2025 at 05:16:51PM -0400, Jason Gunthorpe wrote:
> On Wed, Jan 29, 2025 at 12:25:37PM -0800, Eric Biggers wrote:
> >         static const u8 data[52] = {
> >                 0xf0, 0x12, 0x37, 0x5c, 0x00, 0x0e, 0x17, 0xd2, 0x0a, 0x20, 0x24, 0x87, 0xff, 0x87, 0xb1,
> >                 0xb3, 0x00, 0x0d, 0xec, 0x2a, 0x01, 0x71, 0x0a, 0x1c, 0x01, 0x5d, 0x40, 0x02, 0x38, 0xf2,
> >                 0x7a, 0x05, 0x00, 0x00, 0x00, 0x0e, 0xbb, 0x88, 0x4d, 0x85, 0xfd, 0x5c, 0xfb, 0xa4, 0x72,
> >                 0x8b, 0xc0, 0x69, 0x0e, 0xd4, 0x00, 0x00
> >         };
> >         pr_info("crcval=0x%x\n", ~crc32_le(~0,data,sizeof(data)));
> > 
> > crcval=0x5ab72596
> > 
> > So yes, the InfiniBand spec gives swab(0x5ab72596) = 0x9625B75A.
> > 
> > It's a least-significant-bit first CRC32, so bits 0 through 31 of 0x5ab72596
> > represent the coefficients of x^31 through x^0 in that order.  The byte swap to
> > 0x9625B75A reorders the coefficients to x^7 through x^0, x^15 through x^8, x^23
> > through x^16, x^31 through x^24.  (This is no longer a sequential order, so this
> > order is not usually used.)  The spec then stores the swapped value in big
> > endian order to cancel out the extra swap, resulting in the polynomial
> > coefficients being in a sequential order again.
> 
> > IMO, it's easier to think about this as storing the least-significant-bit first
> > CRC32 value using little endian byte order.
> 
> To be most clear this should be written as:
> 
>   u32 ibta_crc = swab32(~crc32_le(..)) // Gives you the IBTA defined value
>   *packet = cpu_to_be32(ibta_crc); // Puts it in the packet
> 
> It follows the spec clearly and exactly.
> 
> Yes, you can get the same net effect using le:
> 
>   u32 not_ibta_crc = ~crc32_le(..)
>   *packet = cpu_to_le32(not_ibta_crc)
> 
> It does work, but it is very hard to follow how that relates to the
> specification when the u32 is not in the spec's format anymore.
> 
> What matters here, in rxe, is how to use the Linux crc32 library to
> get exactly the value written down in the spec.
> 
> IMHO the le approach is an optimization to avoid the dobule swap, and
> it should simply be described as such in a comment:
> 
>  The crc32 library gives a byte swapped result compared to the IBTA
>  specification. swab32(~crc32_le(..)) will give values that match
>  IBTA.
> 
>  To avoid double swapping we can instead write:
>     *icrc = cpu_to_le32(~crc32_le(..))
>  The value will still be big endian on the network.
> 
> No need to talk about coefficients.

We are looking at the same spec, right?  The following is the specification for
the ICRC field:

    The resulting bits are sent in order from the bit representing the
    coefficient of the highest term of the remainder polynomial. The least
    significant bit, most significant byte first ordering of the packet does not
    apply to the ICRC field.

So it does talk about the polynomial coefficients.

It sounds like you want to add two unnecessary byteswaps to match the example in
Table 25, which misleadingly shows a byte-swapped ICRC value as a u32 without
mentioning it is byte-swapped.  I don't agree that would be clearer, but we can
do it if you prefer.

- Eric