* [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
@ 2025-02-27 15:42 Patrick Venture
2025-02-28 5:54 ` Andrew Jeffery
2025-03-03 11:17 ` Philippe Mathieu-Daudé
0 siblings, 2 replies; 5+ messages in thread
From: Patrick Venture @ 2025-02-27 15:42 UTC (permalink / raw)
To: peter.maydell, clg, steven_lee, leetroy, jamin_lin, andrew, joel
Cc: jasowang, qemu-arm, qemu-devel, Patrick Venture
eth_hdr requires 2 byte alignment
Signed-off-by: Patrick Venture <venture@google.com>
---
hw/net/ftgmac100.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 1f524d7a01..a33aaa01ee 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -989,12 +989,16 @@ static void ftgmac100_high_write(void *opaque, hwaddr addr,
static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
{
unsigned mcast_idx;
+ struct eth_header eth_hdr = {};
if (s->maccr & FTGMAC100_MACCR_RX_ALL) {
return 1;
}
- switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
+ memcpy(ð_hdr, PKT_GET_ETH_HDR(buf),
+ (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
+
+ switch (get_eth_packet_type(ð_hdr)) {
case ETH_PKT_BCAST:
if (!(s->maccr & FTGMAC100_MACCR_RX_BROADPKT)) {
return 0;
@@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
{
FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
FTGMAC100Desc bd;
+ struct eth_header eth_hdr = {};
uint32_t flags = 0;
uint64_t addr;
uint32_t crc;
@@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
uint32_t buf_len;
size_t size = len;
uint32_t first = FTGMAC100_RXDES0_FRS;
- uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
+ uint16_t proto;
+
+ memcpy(ð_hdr, PKT_GET_ETH_HDR(buf),
+ (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
+ proto = be16_to_cpu(eth_hdr.h_proto);
int max_frame_size = ftgmac100_max_frame_size(s, proto);
if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN))
@@ -1061,7 +1070,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
flags |= FTGMAC100_RXDES0_FTL;
}
- switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
+ switch (get_eth_packet_type(ð_hdr)) {
case ETH_PKT_BCAST:
flags |= FTGMAC100_RXDES0_BROADCAST;
break;
--
2.48.1.658.g4767266eb4-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
2025-02-27 15:42 [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment Patrick Venture
@ 2025-02-28 5:54 ` Andrew Jeffery
2025-02-28 18:18 ` Patrick Venture
2025-03-03 11:17 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Jeffery @ 2025-02-28 5:54 UTC (permalink / raw)
To: Patrick Venture, peter.maydell, clg, steven_lee, leetroy,
jamin_lin, joel
Cc: jasowang, qemu-arm, qemu-devel
Hi Patrick,
On Thu, 2025-02-27 at 15:42 +0000, Patrick Venture wrote:
> eth_hdr requires 2 byte alignment
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
> hw/net/ftgmac100.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> index 1f524d7a01..a33aaa01ee 100644
> --- a/hw/net/ftgmac100.c
> +++ b/hw/net/ftgmac100.c
> @@ -989,12 +989,16 @@ static void ftgmac100_high_write(void *opaque, hwaddr addr,
> static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf, size_t len)
> {
> unsigned mcast_idx;
> + struct eth_header eth_hdr = {};
>
> if (s->maccr & FTGMAC100_MACCR_RX_ALL) {
> return 1;
> }
>
> - switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> + memcpy(ð_hdr, PKT_GET_ETH_HDR(buf),
> + (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
I don't think truncating the memcpy() in this way is what we want? The
switched value may not be meaningful for small values of len.
Perhaps return an error?
> +
> + switch (get_eth_packet_type(ð_hdr)) {
> case ETH_PKT_BCAST:
> if (!(s->maccr & FTGMAC100_MACCR_RX_BROADPKT)) {
> return 0;
> @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> {
> FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> FTGMAC100Desc bd;
> + struct eth_header eth_hdr = {};
> uint32_t flags = 0;
> uint64_t addr;
> uint32_t crc;
> @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> uint32_t buf_len;
> size_t size = len;
> uint32_t first = FTGMAC100_RXDES0_FRS;
> - uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
> + uint16_t proto;
> +
> + memcpy(ð_hdr, PKT_GET_ETH_HDR(buf),
> + (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
Again here.
> + proto = be16_to_cpu(eth_hdr.h_proto);
> int max_frame_size = ftgmac100_max_frame_size(s, proto);
>
> if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN | FTGMAC100_MACCR_RXMAC_EN))
> @@ -1061,7 +1070,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> flags |= FTGMAC100_RXDES0_FTL;
> }
>
> - switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> + switch (get_eth_packet_type(ð_hdr)) {
> case ETH_PKT_BCAST:
> flags |= FTGMAC100_RXDES0_BROADCAST;
> break;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
2025-02-28 5:54 ` Andrew Jeffery
@ 2025-02-28 18:18 ` Patrick Venture
0 siblings, 0 replies; 5+ messages in thread
From: Patrick Venture @ 2025-02-28 18:18 UTC (permalink / raw)
To: Andrew Jeffery
Cc: peter.maydell, clg, steven_lee, leetroy, jamin_lin, joel,
jasowang, qemu-arm, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2923 bytes --]
On Thu, Feb 27, 2025 at 9:54 PM Andrew Jeffery <andrew@codeconstruct.com.au>
wrote:
> Hi Patrick,
>
> On Thu, 2025-02-27 at 15:42 +0000, Patrick Venture wrote:
> > eth_hdr requires 2 byte alignment
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> > hw/net/ftgmac100.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> > index 1f524d7a01..a33aaa01ee 100644
> > --- a/hw/net/ftgmac100.c
> > +++ b/hw/net/ftgmac100.c
> > @@ -989,12 +989,16 @@ static void ftgmac100_high_write(void *opaque,
> hwaddr addr,
> > static int ftgmac100_filter(FTGMAC100State *s, const uint8_t *buf,
> size_t len)
> > {
> > unsigned mcast_idx;
> > + struct eth_header eth_hdr = {};
> >
> > if (s->maccr & FTGMAC100_MACCR_RX_ALL) {
> > return 1;
> > }
> >
> > - switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> > + memcpy(ð_hdr, PKT_GET_ETH_HDR(buf),
> > + (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>
> I don't think truncating the memcpy() in this way is what we want? The
> switched value may not be meaningful for small values of len.
>
> Perhaps return an error?
>
> > +
> > + switch (get_eth_packet_type(ð_hdr)) {
> > case ETH_PKT_BCAST:
> > if (!(s->maccr & FTGMAC100_MACCR_RX_BROADPKT)) {
> > return 0;
> > @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> > {
> > FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> > FTGMAC100Desc bd;
> > + struct eth_header eth_hdr = {};
> > uint32_t flags = 0;
> > uint64_t addr;
> > uint32_t crc;
> > @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> > uint32_t buf_len;
> > size_t size = len;
> > uint32_t first = FTGMAC100_RXDES0_FRS;
> > - uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
> > + uint16_t proto;
> > +
> > + memcpy(ð_hdr, PKT_GET_ETH_HDR(buf),
> > + (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>
> Again here.
>
> > + proto = be16_to_cpu(eth_hdr.h_proto);
> > int max_frame_size = ftgmac100_max_frame_size(s, proto);
> >
> > if ((s->maccr & (FTGMAC100_MACCR_RXDMA_EN |
> FTGMAC100_MACCR_RXMAC_EN))
> > @@ -1061,7 +1070,7 @@ static ssize_t ftgmac100_receive(NetClientState
> *nc, const uint8_t *buf,
> > flags |= FTGMAC100_RXDES0_FTL;
> > }
> >
> > - switch (get_eth_packet_type(PKT_GET_ETH_HDR(buf))) {
> > + switch (get_eth_packet_type(ð_hdr)) {
> > case ETH_PKT_BCAST:
> > flags |= FTGMAC100_RXDES0_BROADCAST;
> > break;
>
>
Thanks, I've been asked to fix eth_header to be correctly packed instead
and I'm still fixing the implications of that.
[-- Attachment #2: Type: text/html, Size: 3905 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
2025-02-27 15:42 [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment Patrick Venture
2025-02-28 5:54 ` Andrew Jeffery
@ 2025-03-03 11:17 ` Philippe Mathieu-Daudé
2025-03-03 11:46 ` Peter Maydell
1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-03-03 11:17 UTC (permalink / raw)
To: Patrick Venture, peter.maydell, clg, steven_lee, leetroy,
jamin_lin, andrew, joel
Cc: jasowang, qemu-arm, qemu-devel
Hi Patrick,
On 27/2/25 16:42, Patrick Venture wrote:
> eth_hdr requires 2 byte alignment
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
> hw/net/ftgmac100.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
> @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> {
> FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> FTGMAC100Desc bd;
> + struct eth_header eth_hdr = {};
> uint32_t flags = 0;
> uint64_t addr;
> uint32_t crc;
> @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> uint32_t buf_len;
> size_t size = len;
> uint32_t first = FTGMAC100_RXDES0_FRS;
> - uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
The LD/ST API deals with unaligned fields, would that help?
uint16_t proto = lduw_be_p(&PKT_GET_ETH_HDR(buf)->h_proto);
> + uint16_t proto;
> +
> + memcpy(ð_hdr, PKT_GET_ETH_HDR(buf),
> + (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
> + proto = be16_to_cpu(eth_hdr.h_proto);
> int max_frame_size = ftgmac100_max_frame_size(s, proto);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment
2025-03-03 11:17 ` Philippe Mathieu-Daudé
@ 2025-03-03 11:46 ` Peter Maydell
0 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2025-03-03 11:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Patrick Venture, clg, steven_lee, leetroy, jamin_lin, andrew,
joel, jasowang, qemu-arm, qemu-devel
On Mon, 3 Mar 2025 at 11:17, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Patrick,
>
> On 27/2/25 16:42, Patrick Venture wrote:
> > eth_hdr requires 2 byte alignment
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> > hw/net/ftgmac100.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
>
>
> > @@ -1028,6 +1032,7 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> > {
> > FTGMAC100State *s = FTGMAC100(qemu_get_nic_opaque(nc));
> > FTGMAC100Desc bd;
> > + struct eth_header eth_hdr = {};
> > uint32_t flags = 0;
> > uint64_t addr;
> > uint32_t crc;
> > @@ -1036,7 +1041,11 @@ static ssize_t ftgmac100_receive(NetClientState *nc, const uint8_t *buf,
> > uint32_t buf_len;
> > size_t size = len;
> > uint32_t first = FTGMAC100_RXDES0_FRS;
> > - uint16_t proto = be16_to_cpu(PKT_GET_ETH_HDR(buf)->h_proto);
>
> The LD/ST API deals with unaligned fields, would that help?
>
> uint16_t proto = lduw_be_p(&PKT_GET_ETH_HDR(buf)->h_proto);
No, it doesn't, unfortunately -- I forget the details, but
if the struct is unaligned but its definition says it is
aligned then you get UB even with our accessor functions.
This is why in commit f8b94b4c520126 I had to fix this for
struct ip_header by marking it as QEMU_PACKED, even though
the actual access there was a uint8_t*.
(See also the other thread where I suggested to Patrick that
the best approach here is to mark eth_hdr as QEMU_PACKED,
and fix up anything we need to do to make that change.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-03-03 11:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27 15:42 [PATCH] hw/net: ftgmac100: copy eth_hdr for alignment Patrick Venture
2025-02-28 5:54 ` Andrew Jeffery
2025-02-28 18:18 ` Patrick Venture
2025-03-03 11:17 ` Philippe Mathieu-Daudé
2025-03-03 11:46 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).