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

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

[Heinrich Schuchardt]

struct ip_udp_hdr is naturally packed. There is no point in adding a
__packed attribute. With the attribute the network stack 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);
      |                       ^~~~~~~~~~~~~~

Unfortunately there was a bug in GCC 7.1 which required __packed here.
So let's avoid the error by using a uchar pointer instead of an u16
pointer.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 net/net.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/net.c b/net/net.c
index ded86e7456..e6f6d2cb45 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;
+			uchar *sumptr;
 			ushort  sumlen;

 			xsum  = ip->ip_p;
@@ -1285,13 +1285,11 @@ 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 = (uchar *)&ip->udp_src;

 			while (sumlen > 1) {
-				ushort sumdata;
-
-				sumdata = *sumptr++;
-				xsum += ntohs(sumdata);
+				xsum += (sumptr[0] << 8) + sumptr[1];
+				sumptr += 2;
 				sumlen -= 2;
 			}
 			if (sumlen > 0) {
--
2.24.0.rc1

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

[Simon Goldschmidt]

Am 04.11.2019 um 20:34 schrieb Heinrich Schuchardt:
> struct ip_udp_hdr is naturally packed. There is no point in adding a
> __packed attribute. With the attribute the network stack does not compile
> using GCC 9.2.1:

Is this commit message correct? In lwIP, we *do* need to pack all these 
network header structs as they can come in unaligned. Especially the IP 
header is normally 16-bit aligned if the incoming Ethernet frame is 
32-bit aligned (which is a must for many DMA engines).

This is also the reason why the below code works, I guess: it is rarely 
totally unaligned, but nearly always at least 16-bit aligned, so 
dereferencing the checksum pointer as aligned u16 just works.

Nevertheless, the code is formally wrong and your patch is correct. I 
just don't like the commit message saying 'packed' is not required.

> 
> 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);
>        |                       ^~~~~~~~~~~~~~
> 
> Unfortunately there was a bug in GCC 7.1 which required __packed here.
> So let's avoid the error by using a uchar pointer instead of an u16
> pointer.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   net/net.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/net/net.c b/net/net.c
> index ded86e7456..e6f6d2cb45 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;
> +			uchar *sumptr;
>   			ushort  sumlen;
> 
>   			xsum  = ip->ip_p;
> @@ -1285,13 +1285,11 @@ 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 = (uchar *)&ip->udp_src;
> 
>   			while (sumlen > 1) {
> -				ushort sumdata;
> -
> -				sumdata = *sumptr++;
> -				xsum += ntohs(sumdata);
> +				xsum += (sumptr[0] << 8) + sumptr[1];

Do we need a comment here stating why we have an open-coded copy of 
ntohs? That could keep us from getting this bug back in the future...

Regards,
Simon

> +				sumptr += 2;
>   				sumlen -= 2;
>   			}
>   			if (sumlen > 0) {
> --
> 2.24.0.rc1
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
> 

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

[Tom Rini]

On Mon, Nov 04, 2019 at 09:28:51PM +0100, Simon Goldschmidt wrote:
> Am 04.11.2019 um 20:34 schrieb Heinrich Schuchardt:
> > struct ip_udp_hdr is naturally packed. There is no point in adding a
> > __packed attribute. With the attribute the network stack does not compile
> > using GCC 9.2.1:
> 
> Is this commit message correct? In lwIP, we *do* need to pack all these
> network header structs as they can come in unaligned. Especially the IP
> header is normally 16-bit aligned if the incoming Ethernet frame is 32-bit
> aligned (which is a must for many DMA engines).
> 
> This is also the reason why the below code works, I guess: it is rarely
> totally unaligned, but nearly always at least 16-bit aligned, so
> dereferencing the checksum pointer as aligned u16 just works.
> 
> Nevertheless, the code is formally wrong and your patch is correct. I just
> don't like the commit message saying 'packed' is not required.

Perhaps we should fix the underlying code problem then?  Or does that
men "rewrite the whole file" ?

-- 
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/20191104/17264704/attachment.sig>

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

[Simon Goldschmidt]

Tom Rini <trini@konsulko.com> schrieb am Mo., 4. Nov. 2019, 22:15:

> On Mon, Nov 04, 2019 at 09:28:51PM +0100, Simon Goldschmidt wrote:
> > Am 04.11.2019 um 20:34 schrieb Heinrich Schuchardt:
> > > struct ip_udp_hdr is naturally packed. There is no point in adding a
> > > __packed attribute. With the attribute the network stack does not
> compile
> > > using GCC 9.2.1:
> >
> > Is this commit message correct? In lwIP, we *do* need to pack all these
> > network header structs as they can come in unaligned. Especially the IP
> > header is normally 16-bit aligned if the incoming Ethernet frame is
> 32-bit
> > aligned (which is a must for many DMA engines).
> >
> > This is also the reason why the below code works, I guess: it is rarely
> > totally unaligned, but nearly always at least 16-bit aligned, so
> > dereferencing the checksum pointer as aligned u16 just works.
> >
> > Nevertheless, the code is formally wrong and your patch is correct. I
> just
> > don't like the commit message saying 'packed' is not required.
>
> Perhaps we should fix the underlying code problem then?  Or does that
> men "rewrite the whole file" ?
>

This patch fixes the code problem. If there are more problems: any
assignment to an u16 pointer from an address of a packed struct issues a
warning (provided that appropriate warning settings are used). If we fix
all of these warnings (e.g. like we do here, by using alignment agnostic
byte accesses), we should be good.

I just think the misleading commit message should be fixed before giving my
RB.

Regards,
Simon

>

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

[Tom Rini]

On Mon, Nov 04, 2019 at 10:21:14PM +0100, Simon Goldschmidt wrote:
> Tom Rini <trini@konsulko.com> schrieb am Mo., 4. Nov. 2019, 22:15:
> 
> > On Mon, Nov 04, 2019 at 09:28:51PM +0100, Simon Goldschmidt wrote:
> > > Am 04.11.2019 um 20:34 schrieb Heinrich Schuchardt:
> > > > struct ip_udp_hdr is naturally packed. There is no point in adding a
> > > > __packed attribute. With the attribute the network stack does not
> > compile
> > > > using GCC 9.2.1:
> > >
> > > Is this commit message correct? In lwIP, we *do* need to pack all these
> > > network header structs as they can come in unaligned. Especially the IP
> > > header is normally 16-bit aligned if the incoming Ethernet frame is
> > 32-bit
> > > aligned (which is a must for many DMA engines).
> > >
> > > This is also the reason why the below code works, I guess: it is rarely
> > > totally unaligned, but nearly always at least 16-bit aligned, so
> > > dereferencing the checksum pointer as aligned u16 just works.
> > >
> > > Nevertheless, the code is formally wrong and your patch is correct. I
> > just
> > > don't like the commit message saying 'packed' is not required.
> >
> > Perhaps we should fix the underlying code problem then?  Or does that
> > men "rewrite the whole file" ?
> >
> 
> This patch fixes the code problem. If there are more problems: any
> assignment to an u16 pointer from an address of a packed struct issues a
> warning (provided that appropriate warning settings are used). If we fix
> all of these warnings (e.g. like we do here, by using alignment agnostic
> byte accesses), we should be good.
> 
> I just think the misleading commit message should be fixed before giving my
> RB.

Ah, I get it now I hope, 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/20191104/7a5059f9/attachment.sig>