Re: [PATCH] hw/net: npcm7xx_emc: fix alignment to eth_hdr

[Patrick Venture <venture@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2651 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 3850 bytes --]