Re: [Intel-wired-lan] [net, v1] ice: Fix l2-fwd-offload toggle crash

[Benjamin Mikailenko <benjamin.mikailenko@intel.com>]

On 10/13/2022 8:25 AM, Michal Schmidt wrote:
> On Wed, Oct 12, 2022 at 6:03 PM Benjamin Mikailenko <benjamin.mikailenko@intel.com <mailto:benjamin.mikailenko@intel.com>> wrote:
> 
>     Running netperf traffic and toggling l2-fwd-offload in quick succession
>     caused the driver to crash.
> 
>     BUG: kernel NULL pointer dereference, address: 0000000000000020
>     [  861.517803] #PF: supervisor read access in kernel mode
>     [  861.517805] #PF: error_code(0x0000) - not-present page
>     [  861.517808] PGD 0 P4D 0
>     [  861.517811] Oops: 0000 [#1] PREEMPT SMP PTI
>     [  861.517815] CPU: 60 PID: 16471 Comm: netperf Kdump: loaded Tainted: G S
>     [  861.517818] Hardware name: Intel Corporation S2600WTT/S2600WTT, BIOS SE
>     [  861.517820] RIP: 0010:ice_start_xmit+0xb0/0x1420 [ice]
> 
>     This crash would happen because during l2-fwd-offload configuration,
>     ice_init_macvlan or ice_deinit_macvlan would temporarily work on Tx rings.
> 
> 
> What are ice_init_macvlan and ice_deinit_macvlan? Are they function names? I do not see such functions in the code.
>  

Yes, but l2-fwd-offload for ICE is fixed to "off" in the kernel. So no need for those functions. 
This patch should be rejected.

> 
>     At the same time, ice_start_xmit would attempt to select the correct send
>     buffer from Tx rings but reach a NULL pointer.
> 
>     Fix this by checking if ring exists before proceeding xmit. If ring does
>     not exist, return NETDEV_TX_BUSY.
> 
> 
> Isn't it still racy though?
> Shouldn't rather whatever is fiddling with the rings make sure the Tx queues are stopped first with netif_tx_stop_queue or similar?
> Michal
>  

You're right here as well. I'll have a look into stopping queues.

Thanks Michal, really appreciate the review!
Ben

> 
>     Fixes: 2b245cb29421 ("ice: Implement transmit and NAPI support")
>     Signed-off-by: Benjamin Mikailenko <benjamin.mikailenko@intel.com <mailto:benjamin.mikailenko@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 086f0b3ab68d..96bc8fad39c0 100644
>     --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
>     +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
>     @@ -2405,6 +2405,8 @@ netdev_tx_t ice_start_xmit(struct sk_buff *skb, struct net_device *netdev)
>             struct ice_tx_ring *tx_ring;
> 
>             tx_ring = vsi->tx_rings[skb->queue_mapping];
>     +       if (!tx_ring)
>     +               return NETDEV_TX_BUSY;
> 
>             /* hardware can't handle really short frames, hardware padding works
>              * beyond this point
>     -- 
>     2.34.3
> 
>     _______________________________________________
>     Intel-wired-lan mailing list
>     Intel-wired-lan@osuosl.org <mailto:Intel-wired-lan@osuosl.org>
>     https://lists.osuosl.org/mailman/listinfo/intel-wired-lan <https://lists.osuosl.org/mailman/listinfo/intel-wired-lan>
> 
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan