[U-Boot] [PATCH 1/1] net: avoid address-of-packed-member error
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
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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 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
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.
participants (3)
-
Heinrich Schuchardt -
Simon Goldschmidt -
Tom Rini