[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[Heinrich Schuchardt]

sandbox_defconfig does not compile using GCC 9.2.1:

net/net.c: In function ‘net_process_received_packet’:
net/net.c:1288:23: error: taking address of packed member of ‘struct
ip_udp_hdr’ may result in an unaligned pointer value
[-Werror=address-of-packed-member]
 1288 |    sumptr = (ushort *)&(ip->udp_src);
      |                       ^~~~~~~~~~~~~~

Avoid the error by using a u8 pointer instead of an u16 pointer and
in-lining ntohs().

Simplify the checksumming of the last message byte.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
	reword commit message
	add a comment
	simplify checksumming of last byte
---
 net/net.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/net/net.c b/net/net.c
index ded86e7456..bcb9d5f07e 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1274,7 +1274,7 @@ void net_process_received_packet(uchar *in_packet, int len)
 #ifdef CONFIG_UDP_CHECKSUM
 		if (ip->udp_xsum != 0) {
 			ulong   xsum;
-			ushort *sumptr;
+			u8 *sumptr;
 			ushort  sumlen;

 			xsum  = ip->ip_p;
@@ -1285,22 +1285,16 @@ void net_process_received_packet(uchar *in_packet, int len)
 			xsum += (ntohl(ip->ip_dst.s_addr) >>  0) & 0x0000ffff;

 			sumlen = ntohs(ip->udp_len);
-			sumptr = (ushort *)&(ip->udp_src);
+			sumptr = (u8 *)&ip->udp_src;

 			while (sumlen > 1) {
-				ushort sumdata;
-
-				sumdata = *sumptr++;
-				xsum += ntohs(sumdata);
+				/* inlined ntohs() to avoid alignment errors */
+				xsum += (sumptr[0] << 8) + sumptr[1];
+				sumptr += 2;
 				sumlen -= 2;
 			}
-			if (sumlen > 0) {
-				ushort sumdata;
-
-				sumdata = *(unsigned char *)sumptr;
-				sumdata = (sumdata << 8) & 0xff00;
-				xsum += sumdata;
-			}
+			if (sumlen > 0)
+				xsum += (sumptr[0] << 8) + sumptr[0];
 			while ((xsum >> 16) != 0) {
 				xsum = (xsum & 0x0000ffff) +
 				       ((xsum >> 16) & 0x0000ffff);
--
2.24.0.rc1

[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[Tom Rini]

On Tue, Nov 05, 2019 at 12:48:19PM +0100, Heinrich Schuchardt wrote:

> sandbox_defconfig does not compile using GCC 9.2.1:
> 
> net/net.c: In function ‘net_process_received_packet’:
> net/net.c:1288:23: error: taking address of packed member of ‘struct
> ip_udp_hdr’ may result in an unaligned pointer value
> [-Werror=address-of-packed-member]
>  1288 |    sumptr = (ushort *)&(ip->udp_src);
>       |                       ^~~~~~~~~~~~~~
> 
> Avoid the error by using a u8 pointer instead of an u16 pointer and
> in-lining ntohs().
> 
> Simplify the checksumming of the last message byte.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
> 	reword commit message
> 	add a comment
> 	simplify checksumming of last byte

If I follow what Simon was saying yesterday, the whole message framing
is wrong.  The problem isn't that gcc 9.2 is showing a warning, the
problem is that gcc 9.2 is showing that we have a problem (in terms of
what can/can't happen in real life networking terms), which you're
correcting.  Simon, can you please suggest a commit message here if
you're still not quite happy, as you understand the underlying problem
well it seems.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20191105/67a41290/attachment.sig>

[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[Simon Goldschmidt]

On Tue, Nov 5, 2019 at 2:52 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Nov 05, 2019 at 12:48:19PM +0100, Heinrich Schuchardt wrote:
>
> > sandbox_defconfig does not compile using GCC 9.2.1:
> >
> > net/net.c: In function ‘net_process_received_packet’:
> > net/net.c:1288:23: error: taking address of packed member of ‘struct
> > ip_udp_hdr’ may result in an unaligned pointer value
> > [-Werror=address-of-packed-member]
> >  1288 |    sumptr = (ushort *)&(ip->udp_src);
> >       |                       ^~~~~~~~~~~~~~
> >
> > Avoid the error by using a u8 pointer instead of an u16 pointer and
> > in-lining ntohs().
> >
> > Simplify the checksumming of the last message byte.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> > v2:
> >       reword commit message
> >       add a comment
> >       simplify checksumming of last byte
>
> If I follow what Simon was saying yesterday, the whole message framing
> is wrong.  The problem isn't that gcc 9.2 is showing a warning, the
> problem is that gcc 9.2 is showing that we have a problem (in terms of
> what can/can't happen in real life networking terms), which you're
> correcting.  Simon, can you please suggest a commit message here if
> you're still not quite happy, as you understand the underlying problem
> well it seems.  Thanks!

Well, we do have an error and GCC 9.2 shows this. I don't know why other
versions don't issue this warning.

This new commit message might still concentrate too much on the GCC version,
but I think it's ok. I just wanted to prevent someone reading this in the
future and taking it as a hint that the attribute 'packed' can be removed
(which in turn might procude bugs on some platforms).

So:
Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[Joe Hershberger]

On Tue, Nov 5, 2019 at 5:49 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> sandbox_defconfig does not compile using GCC 9.2.1:
>
> net/net.c: In function ‘net_process_received_packet’:
> net/net.c:1288:23: error: taking address of packed member of ‘struct
> ip_udp_hdr’ may result in an unaligned pointer value
> [-Werror=address-of-packed-member]
>  1288 |    sumptr = (ushort *)&(ip->udp_src);
>       |                       ^~~~~~~~~~~~~~
>
> Avoid the error by using a u8 pointer instead of an u16 pointer and
> in-lining ntohs().

Seems reasonable.

> Simplify the checksumming of the last message byte.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[Heinrich Schuchardt]

On 11/6/19 12:07 AM, Joe Hershberger wrote:
> On Tue, Nov 5, 2019 at 5:49 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> sandbox_defconfig does not compile using GCC 9.2.1:
>>
>> net/net.c: In function ‘net_process_received_packet’:
>> net/net.c:1288:23: error: taking address of packed member of ‘struct
>> ip_udp_hdr’ may result in an unaligned pointer value
>> [-Werror=address-of-packed-member]
>>   1288 |    sumptr = (ushort *)&(ip->udp_src);
>>        |                       ^~~~~~~~~~~~~~
>>
>> Avoid the error by using a u8 pointer instead of an u16 pointer and
>> in-lining ntohs().
>
> Seems reasonable.
>
>> Simplify the checksumming of the last message byte.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>

Hello Joe,

this patch did not yet make it into
https://gitlab.denx.de/u-boot/custodians/u-boot-net/commits/master

Is there something that needs to be changed?

Best regards

Heinrich

[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[Tom Rini]

On Thu, Dec 05, 2019 at 08:16:15AM +0100, Heinrich Schuchardt wrote:
> On 11/6/19 12:07 AM, Joe Hershberger wrote:
> > On Tue, Nov 5, 2019 at 5:49 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > 
> > > sandbox_defconfig does not compile using GCC 9.2.1:
> > > 
> > > net/net.c: In function ‘net_process_received_packet’:
> > > net/net.c:1288:23: error: taking address of packed member of ‘struct
> > > ip_udp_hdr’ may result in an unaligned pointer value
> > > [-Werror=address-of-packed-member]
> > >   1288 |    sumptr = (ushort *)&(ip->udp_src);
> > >        |                       ^~~~~~~~~~~~~~
> > > 
> > > Avoid the error by using a u8 pointer instead of an u16 pointer and
> > > in-lining ntohs().
> > 
> > Seems reasonable.
> > 
> > > Simplify the checksumming of the last message byte.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > 
> > Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> > 
> 
> Hello Joe,
> 
> this patch did not yet make it into
> https://gitlab.denx.de/u-boot/custodians/u-boot-net/commits/master
> 
> Is there something that needs to be changed?

And perhaps we need to split the net PR in to bug fixes to get now and
stuff to put in to -next, for the next window?  Or is this one of the
changes that causes size overflow?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20191205/8ee0fcb3/attachment.sig>

[U-Boot] [PATCH v2 1/1] net: avoid address-of-packed-member error

[Joe Hershberger]

On Thu, Dec 5, 2019 at 1:19 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/6/19 12:07 AM, Joe Hershberger wrote:
> > On Tue, Nov 5, 2019 at 5:49 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> sandbox_defconfig does not compile using GCC 9.2.1:
> >>
> >> net/net.c: In function ‘net_process_received_packet’:
> >> net/net.c:1288:23: error: taking address of packed member of ‘struct
> >> ip_udp_hdr’ may result in an unaligned pointer value
> >> [-Werror=address-of-packed-member]
> >>   1288 |    sumptr = (ushort *)&(ip->udp_src);
> >>        |                       ^~~~~~~~~~~~~~
> >>
> >> Avoid the error by using a u8 pointer instead of an u16 pointer and
> >> in-lining ntohs().
> >
> > Seems reasonable.
> >
> >> Simplify the checksumming of the last message byte.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >
> > Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> >
>
> Hello Joe,
>
> this patch did not yet make it into
> https://gitlab.denx.de/u-boot/custodians/u-boot-net/commits/master
>
> Is there something that needs to be changed?

No, it is among the patches I'm currently testing [1].

[1] - https://github.com/jhershbe/u-boot/commits/travis_ci/test_1184510

Cheers,
-Joe