'const struct eth_header', which requires 2 byte alignment

Signed-off-by: Patrick Venture <venture@google.com>
---
 hw/net/npcm7xx_emc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
index e06f652629..11ed4a9e6a 100644
--- a/hw/net/npcm7xx_emc.c
+++ b/hw/net/npcm7xx_emc.c
@@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc)
 static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t *buf,
                                 size_t len, const char **fail_reason)
 {
-    eth_pkt_types_e pkt_type = get_eth_packet_type(PKT_GET_ETH_HDR(buf));
+    struct eth_header eth_hdr = {};
+    eth_pkt_types_e pkt_type;
+
+    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
+           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
+    pkt_type = get_eth_packet_type(&eth_hdr);
 
     switch (pkt_type) {
     case ETH_PKT_BCAST:
-- 
2.48.1.658.g4767266eb4-goog

On Thu, 27 Feb 2025 at 15:40, Patrick Venture <venture@google.com> wrote:
>
> 'const struct eth_header', which requires 2 byte alignment
>
> Signed-off-by: Patrick Venture <venture@google.com>
> ---
>  hw/net/npcm7xx_emc.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
> index e06f652629..11ed4a9e6a 100644
> --- a/hw/net/npcm7xx_emc.c
> +++ b/hw/net/npcm7xx_emc.c
> @@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc)
>  static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t *buf,
>                                  size_t len, const char **fail_reason)
>  {
> -    eth_pkt_types_e pkt_type = get_eth_packet_type(PKT_GET_ETH_HDR(buf));
> +    struct eth_header eth_hdr = {};
> +    eth_pkt_types_e pkt_type;
> +
> +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
> +    pkt_type = get_eth_packet_type(&eth_hdr);

Maybe better to mark struct eth_header as QEMU_PACKED?
Compare commit f8b94b4c5201 ("net: mark struct ip_header as
QEMU_PACKED"). The handling of these header structs in eth.h
is in general pretty suspect IMHO. We do the same
"get_eth_packet_type(PKT_GET_ETH_HDR(buf))" in other devices,
so this isn't just this device's bug.

thanks
-- PMM

On Thu, Feb 27, 2025 at 7:52 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 27 Feb 2025 at 15:40, Patrick Venture <venture@google.com> wrote:
> >
> > 'const struct eth_header', which requires 2 byte alignment
> >
> > Signed-off-by: Patrick Venture <venture@google.com>
> > ---
> >  hw/net/npcm7xx_emc.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
> > index e06f652629..11ed4a9e6a 100644
> > --- a/hw/net/npcm7xx_emc.c
> > +++ b/hw/net/npcm7xx_emc.c
> > @@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc)
> >  static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t
> *buf,
> >                                  size_t len, const char **fail_reason)
> >  {
> > -    eth_pkt_types_e pkt_type =
> get_eth_packet_type(PKT_GET_ETH_HDR(buf));
> > +    struct eth_header eth_hdr = {};
> > +    eth_pkt_types_e pkt_type;
> > +
> > +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
> > +    pkt_type = get_eth_packet_type(&eth_hdr);
>
> Maybe better to mark struct eth_header as QEMU_PACKED?
> Compare commit f8b94b4c5201 ("net: mark struct ip_header as
> QEMU_PACKED"). The handling of these header structs in eth.h
> is in general pretty suspect IMHO. We do the same
> "get_eth_packet_type(PKT_GET_ETH_HDR(buf))" in other devices,
> so this isn't just this device's bug.
>
> thanks
>

Roger that. We saw this in the two NICs we happened to be testing that day,
and yeah, I grepped and just figured that those other NICs were doing
something with their buffer allocations that we didn't. I'll give
QEMU_PACKED  whirl.


> -- PMM
>

On Thu, 27 Feb 2025 at 15:55, Patrick Venture <venture@google.com> wrote:
>
>
>
> On Thu, Feb 27, 2025 at 7:52 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Thu, 27 Feb 2025 at 15:40, Patrick Venture <venture@google.com> wrote:
>> >
>> > 'const struct eth_header', which requires 2 byte alignment
>> >
>> > Signed-off-by: Patrick Venture <venture@google.com>
>> > ---
>> >  hw/net/npcm7xx_emc.c | 7 ++++++-
>> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
>> > index e06f652629..11ed4a9e6a 100644
>> > --- a/hw/net/npcm7xx_emc.c
>> > +++ b/hw/net/npcm7xx_emc.c
>> > @@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc)
>> >  static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t *buf,
>> >                                  size_t len, const char **fail_reason)
>> >  {
>> > -    eth_pkt_types_e pkt_type = get_eth_packet_type(PKT_GET_ETH_HDR(buf));
>> > +    struct eth_header eth_hdr = {};
>> > +    eth_pkt_types_e pkt_type;
>> > +
>> > +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
>> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>> > +    pkt_type = get_eth_packet_type(&eth_hdr);
>>
>> Maybe better to mark struct eth_header as QEMU_PACKED?
>> Compare commit f8b94b4c5201 ("net: mark struct ip_header as
>> QEMU_PACKED"). The handling of these header structs in eth.h
>> is in general pretty suspect IMHO. We do the same
>> "get_eth_packet_type(PKT_GET_ETH_HDR(buf))" in other devices,
>> so this isn't just this device's bug.

> Roger that. We saw this in the two NICs we happened to be testing that day, and yeah, I grepped and just figured that those other NICs were doing something with their buffer allocations that we didn't. I'll give QEMU_PACKED  whirl.

You might find you need to make some fixes to other
devices to get the QEMU_PACKED change to compile (do an
all-targets build to test that). For instance for the
ip_header change I had to first fix virtio-net.c in commit
5814c0846793715. The kind of thing that will need fixing is
if there are places where code takes the address of the
h_proto field and puts it into a uint16_t* : the compiler
will complain about that. A quick grep suggests that the
rocker_of_dpa.c code might be doing something like this, but
hopefully that's it.

thanks
-- PMM

On Thu, Feb 27, 2025 at 8:01 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 27 Feb 2025 at 15:55, Patrick Venture <venture@google.com> wrote:
> >
> >
> >
> > On Thu, Feb 27, 2025 at 7:52 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >>
> >> On Thu, 27 Feb 2025 at 15:40, Patrick Venture <venture@google.com>
> wrote:
> >> >
> >> > 'const struct eth_header', which requires 2 byte alignment
> >> >
> >> > Signed-off-by: Patrick Venture <venture@google.com>
> >> > ---
> >> >  hw/net/npcm7xx_emc.c | 7 ++++++-
> >> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
> >> > index e06f652629..11ed4a9e6a 100644
> >> > --- a/hw/net/npcm7xx_emc.c
> >> > +++ b/hw/net/npcm7xx_emc.c
> >> > @@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc)
> >> >  static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t
> *buf,
> >> >                                  size_t len, const char **fail_reason)
> >> >  {
> >> > -    eth_pkt_types_e pkt_type =
> get_eth_packet_type(PKT_GET_ETH_HDR(buf));
> >> > +    struct eth_header eth_hdr = {};
> >> > +    eth_pkt_types_e pkt_type;
> >> > +
> >> > +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
> >> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
> >> > +    pkt_type = get_eth_packet_type(&eth_hdr);
> >>
> >> Maybe better to mark struct eth_header as QEMU_PACKED?
> >> Compare commit f8b94b4c5201 ("net: mark struct ip_header as
> >> QEMU_PACKED"). The handling of these header structs in eth.h
> >> is in general pretty suspect IMHO. We do the same
> >> "get_eth_packet_type(PKT_GET_ETH_HDR(buf))" in other devices,
> >> so this isn't just this device's bug.
>
> > Roger that. We saw this in the two NICs we happened to be testing that
> day, and yeah, I grepped and just figured that those other NICs were doing
> something with their buffer allocations that we didn't. I'll give
> QEMU_PACKED  whirl.
>
> You might find you need to make some fixes to other
> devices to get the QEMU_PACKED change to compile (do an
> all-targets build to test that). For instance for the
> ip_header change I had to first fix virtio-net.c in commit
> 5814c0846793715. The kind of thing that will need fixing is
> if there are places where code takes the address of the
> h_proto field and puts it into a uint16_t* : the compiler
> will complain about that. A quick grep suggests that the
> rocker_of_dpa.c code might be doing something like this, but
> hopefully that's it.
>

Thanks for the head's up.

>
> thanks
> -- PMM
>

On Thu, Feb 27, 2025 at 8:08 AM Patrick Venture <venture@google.com> wrote:

>
>
> On Thu, Feb 27, 2025 at 8:01 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On Thu, 27 Feb 2025 at 15:55, Patrick Venture <venture@google.com> wrote:
>> >
>> >
>> >
>> > On Thu, Feb 27, 2025 at 7:52 AM Peter Maydell <peter.maydell@linaro.org>
>> wrote:
>> >>
>> >> On Thu, 27 Feb 2025 at 15:40, Patrick Venture <venture@google.com>
>> wrote:
>> >> >
>> >> > 'const struct eth_header', which requires 2 byte alignment
>> >> >
>> >> > Signed-off-by: Patrick Venture <venture@google.com>
>> >> > ---
>> >> >  hw/net/npcm7xx_emc.c | 7 ++++++-
>> >> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
>> >> > index e06f652629..11ed4a9e6a 100644
>> >> > --- a/hw/net/npcm7xx_emc.c
>> >> > +++ b/hw/net/npcm7xx_emc.c
>> >> > @@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc)
>> >> >  static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t
>> *buf,
>> >> >                                  size_t len, const char
>> **fail_reason)
>> >> >  {
>> >> > -    eth_pkt_types_e pkt_type =
>> get_eth_packet_type(PKT_GET_ETH_HDR(buf));
>> >> > +    struct eth_header eth_hdr = {};
>> >> > +    eth_pkt_types_e pkt_type;
>> >> > +
>> >> > +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
>> >> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>> >> > +    pkt_type = get_eth_packet_type(&eth_hdr);
>> >>
>> >> Maybe better to mark struct eth_header as QEMU_PACKED?
>> >> Compare commit f8b94b4c5201 ("net: mark struct ip_header as
>> >> QEMU_PACKED"). The handling of these header structs in eth.h
>> >> is in general pretty suspect IMHO. We do the same
>> >> "get_eth_packet_type(PKT_GET_ETH_HDR(buf))" in other devices,
>> >> so this isn't just this device's bug.
>>
>> > Roger that. We saw this in the two NICs we happened to be testing that
>> day, and yeah, I grepped and just figured that those other NICs were doing
>> something with their buffer allocations that we didn't. I'll give
>> QEMU_PACKED  whirl.
>>
>> You might find you need to make some fixes to other
>> devices to get the QEMU_PACKED change to compile (do an
>> all-targets build to test that). For instance for the
>> ip_header change I had to first fix virtio-net.c in commit
>> 5814c0846793715. The kind of thing that will need fixing is
>> if there are places where code takes the address of the
>> h_proto field and puts it into a uint16_t* : the compiler
>> will complain about that. A quick grep suggests that the
>> rocker_of_dpa.c code might be doing something like this, but
>> hopefully that's it.
>>
>
Ok, so digging, and I see that vlanhdr is used similarly in the
rocker_of_dpa.c code, so, without trying to bit off the yak shave of fixing
all ethernet headers, but in reality ethernet packets are packed
structures, should we just make them all packed and bite that bullet?


>
> Thanks for the head's up.
>
>>
>> thanks
>> -- PMM
>>
>

On Thu, 27 Feb 2025 at 18:12, Patrick Venture <venture@google.com> wrote:
>
>
>
> On Thu, Feb 27, 2025 at 8:08 AM Patrick Venture <venture@google.com> wrote:
>>
>>
>>
>> On Thu, Feb 27, 2025 at 8:01 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> On Thu, 27 Feb 2025 at 15:55, Patrick Venture <venture@google.com> wrote:
>>> >
>>> >
>>> >
>>> > On Thu, Feb 27, 2025 at 7:52 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>> >>
>>> >> On Thu, 27 Feb 2025 at 15:40, Patrick Venture <venture@google.com> wrote:
>>> >> >
>>> >> > 'const struct eth_header', which requires 2 byte alignment
>>> >> >
>>> >> > Signed-off-by: Patrick Venture <venture@google.com>
>>> >> > ---
>>> >> >  hw/net/npcm7xx_emc.c | 7 ++++++-
>>> >> >  1 file changed, 6 insertions(+), 1 deletion(-)
>>> >> >
>>> >> > diff --git a/hw/net/npcm7xx_emc.c b/hw/net/npcm7xx_emc.c
>>> >> > index e06f652629..11ed4a9e6a 100644
>>> >> > --- a/hw/net/npcm7xx_emc.c
>>> >> > +++ b/hw/net/npcm7xx_emc.c
>>> >> > @@ -424,7 +424,12 @@ static bool emc_can_receive(NetClientState *nc)
>>> >> >  static bool emc_receive_filter1(NPCM7xxEMCState *emc, const uint8_t *buf,
>>> >> >                                  size_t len, const char **fail_reason)
>>> >> >  {
>>> >> > -    eth_pkt_types_e pkt_type = get_eth_packet_type(PKT_GET_ETH_HDR(buf));
>>> >> > +    struct eth_header eth_hdr = {};
>>> >> > +    eth_pkt_types_e pkt_type;
>>> >> > +
>>> >> > +    memcpy(&eth_hdr, PKT_GET_ETH_HDR(buf),
>>> >> > +           (sizeof(eth_hdr) > len) ? len : sizeof(eth_hdr));
>>> >> > +    pkt_type = get_eth_packet_type(&eth_hdr);
>>> >>
>>> >> Maybe better to mark struct eth_header as QEMU_PACKED?
>>> >> Compare commit f8b94b4c5201 ("net: mark struct ip_header as
>>> >> QEMU_PACKED"). The handling of these header structs in eth.h
>>> >> is in general pretty suspect IMHO. We do the same
>>> >> "get_eth_packet_type(PKT_GET_ETH_HDR(buf))" in other devices,
>>> >> so this isn't just this device's bug.
>>>
>>> > Roger that. We saw this in the two NICs we happened to be testing that day, and yeah, I grepped and just figured that those other NICs were doing something with their buffer allocations that we didn't. I'll give QEMU_PACKED  whirl.
>>>
>>> You might find you need to make some fixes to other
>>> devices to get the QEMU_PACKED change to compile (do an
>>> all-targets build to test that). For instance for the
>>> ip_header change I had to first fix virtio-net.c in commit
>>> 5814c0846793715. The kind of thing that will need fixing is
>>> if there are places where code takes the address of the
>>> h_proto field and puts it into a uint16_t* : the compiler
>>> will complain about that. A quick grep suggests that the
>>> rocker_of_dpa.c code might be doing something like this, but
>>> hopefully that's it.
>
>
> Ok, so digging, and I see that vlanhdr is used similarly in the rocker_of_dpa.c code, so, without trying to bit off the yak shave of fixing all ethernet headers, but in reality ethernet packets are packed structures, should we just make them all packed and bite that bullet?

If you want to do all of them that's probably the long term
right thing. But the patchset structure would be a series
of "fix X that assumes struct A is not packed", "fix Y
that assumes struct A is not packed", "mark struct A packed",
"fix Z that assumes struct B is not packed", "mark struct B
packed", etc -- so I don't mind if you stop partway through
without doing all of them. (After all, that's exactly what
I did with only doing ip_header :-))

thanks
-- PMM