Message-ID: <IA3PR11MB89869D6FDF168BE8A17C40FEE597A@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Thu, 22 Jan 2026 10:12:54 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Qingfang Deng <dqfext@...il.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>,
	Chuanhong Guo <gch981213@...il.com>
Subject: RE: [PATCH iwl-next v1] ixgbe: fix type punning in
 ixgbe_update_flash_X550



> -----Original Message-----
> From: Qingfang Deng <dqfext@...il.com>
> Sent: Thursday, January 22, 2026 10:40 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; netdev@...r.kernel.org; Jagielski,
> Jedrzej <jedrzej.jagielski@...el.com>; Chuanhong Guo
> <gch981213@...il.com>
> Subject: Re: [PATCH iwl-next v1] ixgbe: fix type punning in
> ixgbe_update_flash_X550
> 
> On Thu, 22 Jan 2026 09:51:02 +0100, Aleksandr Loktionov wrote:
> > Add a u32 buffer array member to union ixgbe_hic_hdr2 and use it
> > directly instead of casting the union address to u32 pointer. This
> > avoids potential strict aliasing violations and makes the code more
> > explicit about the buffer usage.
> >
> > The ixgbe_host_interface_command function expects a void* buffer, so
> > providing a proper u32 array member in the union is the correct
> > approach rather than relying on pointer casting. This eliminates the
> > type punning issue where we were casting the union pointer to u32*.
> >
> > By using buffer.buf instead of &buffer, we pass the address of the
> > u32 array directly, which is semantically correct and avoids any
> > potential undefined behavior from strict aliasing rule violations.
> 
> This commit message is unnecessarily verbose, looks like AI-generated.
> The kernel is built with -fno-strict-aliasing, so it's okay to not
> follow the rule.

Thanks for the review, and agreed on the root cause.
My motivation here was the mismatch between how the buffer is defined and
how it’s consumed: the current cast‑to‑u32 * pattern felt brittle.
Making the HIC buffer naturally 4‑byte aligned is simpler and clearer for
both readers and the compiler. Separately, while x86 will typically
tolerate this, other architectures require natural alignment and may trap
or penalize unaligned 32‑bit accesses. So even if a crash hasn’t been
reported, relying on 1‑byte alignment for something treated as u32[] is
not great practice across all supported arches. This change makes the
layout explicitly safe. I’ll resend with a corrected commit message that
focuses on alignment (not strict aliasing, given the kernel is built with -fno-strict-aliasing).

ixgbe: fix unaligned u32 access in ixgbe_update_flash_X550()

ixgbe_host_interface_command() treats its buffer as a u32 array. The local
buffer we pass in was a union of byte-sized fields, which gives it 1‑byte
alignment on the stack. On strict-align architectures this can cause
unaligned 32‑bit accesses.

Add a u32 member to union ixgbe_hic_hdr2 so the object is 4‑byte aligned, and
pass the u32 member when calling ixgbe_host_interface_command().

No functional change on x86; prevents unaligned accesses on architectures
that enforce natural alignment.

Fixes: 49425dfc7451 ("ixgbe: Add support for x550em_a 10G MAC type")
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>

What do you think?

Thanks!

> What you're fixing is likely an alignment issue. (see below)
> 
> >
> > Fixes: 49425dfc7451 ("ixgbe: Add support for x550em_a 10G MAC type")
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> > Reviewed-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
> > ---
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 1 +
> > drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
> > index 61f2ef6..eb5bf3b 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h
> > @@ -2798,6 +2798,7 @@ struct ixgbe_hic_hdr2_rsp {  };
> >
> >  union ixgbe_hic_hdr2 {
> > +	u32 buf[1];
> 
> The alignment of this union was 1 byte. By adding a u32 member, you're
> effectively making it align to u32 (4 bytes).
> 
> >  	struct ixgbe_hic_hdr2_req req;
> >  	struct ixgbe_hic_hdr2_rsp rsp;
> >  };
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> > index 76d2fa3..4a0ccbf 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> > @@ -1228,7 +1228,7 @@ static int ixgbe_update_flash_X550(struct
> ixgbe_hw *hw)
> >  	buffer.req.buf_lenl = FW_SHADOW_RAM_DUMP_LEN;
> >  	buffer.req.checksum = FW_DEFAULT_CHECKSUM;
> >
> > -	status = ixgbe_host_interface_command(hw, &buffer,
> sizeof(buffer),
> > +	status = ixgbe_host_interface_command(hw, buffer.buf,
> > +sizeof(buffer),
> >  					      IXGBE_HI_COMMAND_TIMEOUT,
> false);
> 
> `buffer` is a local variable allocated on stack, and the compiler did
> not guarantee its alignment. As ixgbe_host_interface_command() casts
> `buffer` to a u32 array, this may cause an unaligned-access exception
> on some arch.
> 
> For your reference, I addressed a similar issue previously:
> https://lore.kernel.org/all/20230601015432.159066-1-dqfext@gmail.com/
> 
> Please update your message, and try not to use completely-AIGC
> phrases.
> 
> >  	return status;
> >  }
> > --
> > 2.52.0
> >
> >
> 
> Regards,
> Qingfang

Powered by blists - more mailing lists