[PATCH] ice: add missing reset of the mac header

[PATCH] ice: add missing reset of the mac header

[Aleksandr Loktionov]

From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>

By default skb->mac_header is not set, so reset prevents access to an
invalid pointer.

Call skb_reset_mac_header() before accessing the mac header from skb.

Signed-off-by: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_txrx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index a2cd4cf..374b17f 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -2211,6 +2211,8 @@ ice_xmit_frame_ring(struct sk_buff *skb, struct ice_tx_ring *tx_ring)
 		goto out_drop;
 
 	/* allow CONTROL frames egress from main VSI if FW LLDP disabled */
+	if (likely(!skb_mac_header_was_set(skb)))
+		skb_reset_mac_header(skb);
 	eth = (struct ethhdr *)skb_mac_header(skb);
 
 	if ((ice_is_switchdev_running(vsi->back) ||
-- 
2.52.0

Re: [PATCH] ice: add missing reset of the mac header

[Simon Horman]

On Fri, Mar 20, 2026 at 06:05:18AM +0100, Aleksandr Loktionov wrote:
> From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> 
> By default skb->mac_header is not set, so reset prevents access to an
> invalid pointer.
> 
> Call skb_reset_mac_header() before accessing the mac header from skb.
> 
> Signed-off-by: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Hi Katarzyna and Aleksandr,

I am curious:

Is this a bug? If so then it should probably have a fixes tag and
a bit more of a description around how this can happen.

If it is not a bug, then is this defensive? And if so, is it really necessary?

...

RE: [PATCH] ice: add missing reset of the mac header

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, March 20, 2026 7:05 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Wieczerzycka,
> Katarzyna <katarzyna.wieczerzycka@intel.com>
> Subject: Re: [PATCH] ice: add missing reset of the mac header
> 
> On Fri, Mar 20, 2026 at 06:05:18AM +0100, Aleksandr Loktionov wrote:
> > From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> >
> > By default skb->mac_header is not set, so reset prevents access to
> an
> > invalid pointer.
> >
> > Call skb_reset_mac_header() before accessing the mac header from
> skb.
> >
> > Signed-off-by: Katarzyna Wieczerzycka
> > <katarzyna.wieczerzycka@intel.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
> Hi Katarzyna and Aleksandr,
> 
> I am curious:
> 
> Is this a bug? If so then it should probably have a fixes tag and a
> bit more of a description around how this can happen.
> 
> If it is not a bug, then is this defensive? And if so, is it really
> necessary?
> 
> ...


Good day, Simon

I'm upstreaming Katarzyna's fix.

From my point of view, it's not just defensive code, but real bug even on latest kernel because the gap is partially closed by packet_parse_headers(), but not completely.
Sorry the patch header is malformed, I definitely need to add 
Fixes: f9f83202b726 ("ice: Allow all LLDP packets from PF to Tx")
But not sure whether to send to net, because on modern kernels I have no real call traces only theoretical conclusion.

Re: [PATCH] ice: add missing reset of the mac header

[Simon Horman]

On Fri, Mar 20, 2026 at 07:53:28PM +0000, Loktionov, Aleksandr wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Friday, March 20, 2026 7:05 PM
> > To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Wieczerzycka,
> > Katarzyna <katarzyna.wieczerzycka@intel.com>
> > Subject: Re: [PATCH] ice: add missing reset of the mac header
> > 
> > On Fri, Mar 20, 2026 at 06:05:18AM +0100, Aleksandr Loktionov wrote:
> > > From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> > >
> > > By default skb->mac_header is not set, so reset prevents access to
> > an
> > > invalid pointer.
> > >
> > > Call skb_reset_mac_header() before accessing the mac header from
> > skb.
> > >
> > > Signed-off-by: Katarzyna Wieczerzycka
> > > <katarzyna.wieczerzycka@intel.com>
> > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > 
> > Hi Katarzyna and Aleksandr,
> > 
> > I am curious:
> > 
> > Is this a bug? If so then it should probably have a fixes tag and a
> > bit more of a description around how this can happen.
> > 
> > If it is not a bug, then is this defensive? And if so, is it really
> > necessary?
> > 
> > ...
> 
> 
> Good day, Simon
> 
> I'm upstreaming Katarzyna's fix.
> 
> From my point of view, it's not just defensive code, but real bug even on latest kernel because the gap is partially closed by packet_parse_headers(), but not completely.
> Sorry the patch header is malformed, I definitely need to add 
> Fixes: f9f83202b726 ("ice: Allow all LLDP packets from PF to Tx")

Hi Aleksandr,

I can see you have been busy.

I agree adding a Fixes tag makes sense.  If you could also expand the patch
description that would be well appreciated (at least by me).

> But not sure whether to send to net, because on modern kernels I have no real call traces only theoretical conclusion.

That is a line call in my opinion.

If you include a Fixes tag, which seems sensible, then the chances are the
patch will end up being backported to stable. And if the patch is in
net-next it might not have hit Linus's tree before that happens. Which
doesn't seem ideal. So I think it would be more sensible to target the
patch at net to avoid that problem. But that's just my feeling.

Either way, please consider adding a note regarding this.
So it will be more obvious.

RE: [PATCH] ice: add missing reset of the mac header

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, March 20, 2026 7:05 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Wieczerzycka,
> Katarzyna <katarzyna.wieczerzycka@intel.com>
> Subject: Re: [PATCH] ice: add missing reset of the mac header
> 
> On Fri, Mar 20, 2026 at 06:05:18AM +0100, Aleksandr Loktionov wrote:
> > From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> >
> > By default skb->mac_header is not set, so reset prevents access to
> an
> > invalid pointer.
> >
> > Call skb_reset_mac_header() before accessing the mac header from
> skb.
> >
> > Signed-off-by: Katarzyna Wieczerzycka
> > <katarzyna.wieczerzycka@intel.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
> Hi Katarzyna and Aleksandr,
> 
> I am curious:
> 
> Is this a bug? If so then it should probably have a fixes tag and a
> bit more of a description around how this can happen.
> 
> If it is not a bug, then is this defensive? And if so, is it really
> necessary?
> 
> ...


Withdrawing this patch. The bug justification and repro are not
strong enough to meet the bar for upstream. Will drop for now.

With the best regards
Alex

Re: [Intel-wired-lan] [PATCH] ice: add missing reset of the mac header

[Alexander Lobakin]

From: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
Date: Fri, 20 Mar 2026 06:05:18 +0100

> From: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> 
> By default skb->mac_header is not set, so reset prevents access to an
> invalid pointer.
> 
> Call skb_reset_mac_header() before accessing the mac header from skb.
> 
> Signed-off-by: Katarzyna Wieczerzycka <katarzyna.wieczerzycka@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_txrx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index a2cd4cf..374b17f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> @@ -2211,6 +2211,8 @@ ice_xmit_frame_ring(struct sk_buff *skb, struct ice_tx_ring *tx_ring)
>  		goto out_drop;
>  
>  	/* allow CONTROL frames egress from main VSI if FW LLDP disabled */
> +	if (likely(!skb_mac_header_was_set(skb)))
> +		skb_reset_mac_header(skb);
>  	eth = (struct ethhdr *)skb_mac_header(skb);
>  
>  	if ((ice_is_switchdev_running(vsi->back) ||

Did you read my internal review? I've never faced a situation when the
kernel queues an skb to .ndo_start_xmit() without the header pointers
set up correctly.

I need a good repro to agree this is needed. `likely()` also doesn't fit
here (you basically slow down almost 100% of the traffic).

Thanks,
Olek