[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1214762490.6037.9.camel@brick>
Date: Sun, 29 Jun 2008 11:01:30 -0700
From: Harvey Harrison <harvey.harrison@...il.com>
To: Thomas Graf <tgraf@...g.ch>
Cc: David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: em_cmp.c use unaligned access helpers
On Sun, 2008-06-29 at 11:54 +0200, Thomas Graf wrote:
> * David Miller <davem@...emloft.net> 2008-06-27 20:16
> > From: Harvey Harrison <harvey.harrison@...il.com>
> > Date: Wed, 25 Jun 2008 14:14:00 -0700
> >
> > > Both locations are loading a big-endian value in cpu-endianness. The
> > > be32/be16_to_cpu immediately afterwards seems suspect.
> > >
> > > Signed-off-by: Harvey Harrison <harvey.harrison@...il.com>
> >
> > Smells like a bug, Thomas can you take a close look at this?
>
> It's not a bug, the u16|u32 pointed to by ptr can be at any offset in the
> skb buffer and is not necessarly in big endian, it can be both. Only the
> user knows so he has to specify a flag if the value should be converted
> from be to cpu.
OK, but be16/32_to_cpu is a no-op on be-arches, so there is a bug here on
big-endian machines as they won't switch it back, your simplified patch
is actually a bugfix in that case.
>
> I think the code is correct, although it could be simplified to:
>
> case TCF_EM_ALIGN_U16:
> if (!cmp_needs_transformation(cmp))
> val = get_unaligned_le16(ptr);
> else
> val = get_unaligned_be16(ptr);
>
> I wasn't aware of these flags when I wrote the code initially.
I introduced them recently, your patch is probably applicable to
2.6.26 as a bugfix.
Reviewed-by: Harvey Harrison <harvey.harrison@...il.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists