Patchwork [1/4] sctp: Fix crc32c calculations on big-endian arhes.

login
register
mail settings
Submitter Vlad Yasevich
Date Jan. 22, 2009, 10:36 p.m.
Message ID <1232663768-4028-2-git-send-email-vladislav.yasevich@hp.com>
Download mbox | patch
Permalink /patch/19910/
State Accepted
Delegated to: David Miller
Headers show

Comments

Vlad Yasevich - Jan. 22, 2009, 10:36 p.m.
crc32c algorithm provides a byteswaped result.  On little-endian
arches, the result ends up in big-endian/network byte order.
On big-endinan arches, the result ends up in little-endian
order and needs to be byte swapped again.  Thus calling cpu_to_le32
gives the right output.

Tested-by: Jukka Taimisto <jukka.taimisto@mail.suomi.net>
Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
 include/net/sctp/checksum.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Herbert Xu - Jan. 23, 2009, 5:19 a.m.
Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
>
> diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> index b799fb2..2fec3c3 100644
> --- a/include/net/sctp/checksum.h
> +++ b/include/net/sctp/checksum.h
> @@ -79,5 +79,5 @@ static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32)
> 
> static inline __be32 sctp_end_cksum(__be32 crc32)
> {
> -       return ~crc32;
> +       return (__force __be32)~cpu_to_le32((__force u32)crc32);
> }

Ouch, surely there is a better way to do this?

Cheers,
Herbert Xu - Jan. 23, 2009, 5:28 a.m.
On Fri, Jan 23, 2009 at 04:19:56PM +1100, Herbert Xu wrote:
> Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
> >
> > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> > index b799fb2..2fec3c3 100644
> > --- a/include/net/sctp/checksum.h
> > +++ b/include/net/sctp/checksum.h
> > @@ -79,5 +79,5 @@ static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32)
> > 
> > static inline __be32 sctp_end_cksum(__be32 crc32)
> > {
> > -       return ~crc32;
> > +       return (__force __be32)~cpu_to_le32((__force u32)crc32);
> > }
> 
> Ouch, surely there is a better way to do this?

In fact this looks wrong.  Has this code actually been tested
on big-endian?

Cheers,
Herbert Xu - Jan. 23, 2009, 9:24 a.m.
On Fri, Jan 23, 2009 at 04:28:49PM +1100, Herbert Xu wrote:
>
> > > static inline __be32 sctp_end_cksum(__be32 crc32)
> > > {
> > > -       return ~crc32;
> > > +       return (__force __be32)~cpu_to_le32((__force u32)crc32);
> > > }
> > 
> > Ouch, surely there is a better way to do this?
> 
> In fact this looks wrong.  Has this code actually been tested
> on big-endian?

Reading this again it does seem to do the right thing as it's
using the raw crc32c interface as opposed to crypto crc32c.

However, I suggest that we change it as follows:

1) Make sh->csum __le32 since we're using crc32c_le.
2) Change all intermediate values in sctp/checksum.h to u32.
3) Make sctp_end_cksum return __le32 and have it do

	return cpu_to_le32(~crc);

Cheers,
Vlad Yasevich - Jan. 23, 2009, 2:52 p.m.
Herbert Xu wrote:
> On Fri, Jan 23, 2009 at 04:28:49PM +1100, Herbert Xu wrote:
>>>> static inline __be32 sctp_end_cksum(__be32 crc32)
>>>> {
>>>> -       return ~crc32;
>>>> +       return (__force __be32)~cpu_to_le32((__force u32)crc32);
>>>> }
>>> Ouch, surely there is a better way to do this?
>> In fact this looks wrong.  Has this code actually been tested
>> on big-endian?

Yes, the code was tested by me and the person who reported it
as listed in the commit log. :)

> 
> Reading this again it does seem to do the right thing as it's
> using the raw crc32c interface as opposed to crypto crc32c.
> 
> However, I suggest that we change it as follows:
> 
> 1) Make sh->csum __le32 since we're using crc32c_le.
> 2) Change all intermediate values in sctp/checksum.h to u32.
> 3) Make sctp_end_cksum return __le32 and have it do
> 
> 	return cpu_to_le32(~crc);
> 

I'll give it some thought.  It would clean up all the __force
casts, but it will be a little misleading to define a packet
checksum as little endian. :)

-vlad

> Cheers,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Jan. 23, 2009, 10:17 p.m.
On Fri, Jan 23, 2009 at 09:52:44AM -0500, Vlad Yasevich wrote:
>
> I'll give it some thought.  It would clean up all the __force
> casts, but it will be a little misleading to define a packet
> checksum as little endian. :)

Not at all! Just because something is on the network doesn't
mean that it's big-endian.  In this case, we're using the little-
endian CRC32C implementation for the checksum so it makes perfect
sense for it to be marked as __le32.

Network-order just has to be fixed in endian, i.e., either big
or small, you just need to stick with the one you choose regardless
of the endianness of the host.

Cheers,

Patch

diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
index b799fb2..2fec3c3 100644
--- a/include/net/sctp/checksum.h
+++ b/include/net/sctp/checksum.h
@@ -79,5 +79,5 @@  static inline __be32 sctp_update_cksum(__u8 *buffer, __u16 length, __be32 crc32)
 
 static inline __be32 sctp_end_cksum(__be32 crc32)
 {
-	return ~crc32;
+	return (__force __be32)~cpu_to_le32((__force u32)crc32);
 }