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

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

[Benjamin Mikailenko]

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.
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.

Fixes: 2b245cb29421 ("ice: Implement transmit and NAPI support")
Signed-off-by: Benjamin Mikailenko <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
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Michal Schmidt]

[-- Attachment #1.1: Type: text/plain, Size: 2400 bytes --]

On Wed, Oct 12, 2022 at 6:03 PM Benjamin Mikailenko <
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.


> 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


> Fixes: 2b245cb29421 ("ice: Implement transmit and NAPI support")
> Signed-off-by: Benjamin Mikailenko <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
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
>
>

[-- Attachment #1.2: Type: text/html, Size: 3499 bytes --]

[-- Attachment #2: Type: text/plain, Size: 162 bytes --]

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Benjamin Mikailenko]

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

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

[Jacob Keller]

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.
>  
> 
>     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
>  

I was going to mention this. No sign of a lock or any other mechanism.
Ideally separately managed structures like this would benefit from
reference counting or such..

Still, sounds like this patch needs to be rejected per other comments.

Thanks,
Jake
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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

[Jacob Keller]

On 10/13/2022 9:51 AM, Benjamin Mikailenko wrote:
> 
> 
> 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
> 

Ok, I'll drop this from queue. What about this reported bug? Is it
something that can't happen on the in-kernel driver?

Thanks,
Jake
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan