diff mbox series

[net,5/9] octeontx2-pf: mcs: Fix NULL pointer dereferences

Message ID 20230423095454.21049-6-gakula@marvell.com
State New
Headers
Series Macsec fixes for CN10KB |

Commit Message

Geetha sowjanya April 23, 2023, 9:54 a.m. UTC 
  From: Subbaraya Sundeep <sbhatta@marvell.com>

When system is rebooted after creating macsec interface
below NULL pointer dereference crashes occurred. This
patch fixes those crashes.

[ 3324.406942] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 3324.415726] Mem abort info:
[ 3324.418510]   ESR = 0x96000006
[ 3324.421557]   EC = 0x25: DABT (current EL), IL = 32 bits
[ 3324.426865]   SET = 0, FnV = 0
[ 3324.429913]   EA = 0, S1PTW = 0
[ 3324.433047] Data abort info:
[ 3324.435921]   ISV = 0, ISS = 0x00000006
[ 3324.439748]   CM = 0, WnR = 0
....
[ 3324.575915] Call trace:
[ 3324.578353]  cn10k_mdo_del_secy+0x24/0x180
[ 3324.582440]  macsec_common_dellink+0xec/0x120
[ 3324.586788]  macsec_notify+0x17c/0x1c0
[ 3324.590529]  raw_notifier_call_chain+0x50/0x70
[ 3324.594965]  call_netdevice_notifiers_info+0x34/0x7c
[ 3324.599921]  rollback_registered_many+0x354/0x5bc
[ 3324.604616]  unregister_netdevice_queue+0x88/0x10c
[ 3324.609399]  unregister_netdev+0x20/0x30
[ 3324.613313]  otx2_remove+0x8c/0x310
[ 3324.616794]  pci_device_shutdown+0x30/0x70
[ 3324.620882]  device_shutdown+0x11c/0x204

[  966.664930] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[  966.673712] Mem abort info:
[  966.676497]   ESR = 0x96000006
[  966.679543]   EC = 0x25: DABT (current EL), IL = 32 bits
[  966.684848]   SET = 0, FnV = 0
[  966.687895]   EA = 0, S1PTW = 0
[  966.691028] Data abort info:
[  966.693900]   ISV = 0, ISS = 0x00000006
[  966.697729]   CM = 0, WnR = 0
....
[  966.833467] Call trace:
[  966.835904]  cn10k_mdo_stop+0x20/0xa0
[  966.839557]  macsec_dev_stop+0xe8/0x11c
[  966.843384]  __dev_close_many+0xbc/0x140
[  966.847298]  dev_close_many+0x84/0x120
[  966.851039]  rollback_registered_many+0x114/0x5bc
[  966.855735]  unregister_netdevice_many.part.0+0x14/0xa0
[  966.860952]  unregister_netdevice_many+0x18/0x24
[  966.865560]  macsec_notify+0x1ac/0x1c0
[  966.869303]  raw_notifier_call_chain+0x50/0x70
[  966.873738]  call_netdevice_notifiers_info+0x34/0x7c
[  966.878694]  rollback_registered_many+0x354/0x5bc
[  966.883390]  unregister_netdevice_queue+0x88/0x10c
[  966.888173]  unregister_netdev+0x20/0x30
[  966.892090]  otx2_remove+0x8c/0x310
[  966.895571]  pci_device_shutdown+0x30/0x70
[  966.899660]  device_shutdown+0x11c/0x204
[  966.903574]  __do_sys_reboot+0x208/0x290
[  966.907487]  __arm64_sys_reboot+0x20/0x30
[  966.911489]  el0_svc_handler+0x80/0x1c0
[  966.915316]  el0_svc+0x8/0x180
[  966.918362] Code: f9400000 f9400a64 91220014 f94b3403 (f9400060)
[  966.924448] ---[ end trace 341778e799c3d8d7 ]---

Fixes: c54ffc73601c ("octeontx2-pf: mcs: Introduce MACSEC hardware offloading")
Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
 drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Leon Romanovsky April 23, 2023, 4:51 p.m. UTC | #1 
On Sun, Apr 23, 2023 at 03:24:50PM +0530, Geetha sowjanya wrote:
> From: Subbaraya Sundeep <sbhatta@marvell.com>
> 
> When system is rebooted after creating macsec interface
> below NULL pointer dereference crashes occurred. This
> patch fixes those crashes.
> 
> [ 3324.406942] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [ 3324.415726] Mem abort info:
> [ 3324.418510]   ESR = 0x96000006
> [ 3324.421557]   EC = 0x25: DABT (current EL), IL = 32 bits
> [ 3324.426865]   SET = 0, FnV = 0
> [ 3324.429913]   EA = 0, S1PTW = 0
> [ 3324.433047] Data abort info:
> [ 3324.435921]   ISV = 0, ISS = 0x00000006
> [ 3324.439748]   CM = 0, WnR = 0
> ....
> [ 3324.575915] Call trace:
> [ 3324.578353]  cn10k_mdo_del_secy+0x24/0x180
> [ 3324.582440]  macsec_common_dellink+0xec/0x120
> [ 3324.586788]  macsec_notify+0x17c/0x1c0
> [ 3324.590529]  raw_notifier_call_chain+0x50/0x70
> [ 3324.594965]  call_netdevice_notifiers_info+0x34/0x7c
> [ 3324.599921]  rollback_registered_many+0x354/0x5bc
> [ 3324.604616]  unregister_netdevice_queue+0x88/0x10c
> [ 3324.609399]  unregister_netdev+0x20/0x30
> [ 3324.613313]  otx2_remove+0x8c/0x310
> [ 3324.616794]  pci_device_shutdown+0x30/0x70
> [ 3324.620882]  device_shutdown+0x11c/0x204
> 
> [  966.664930] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [  966.673712] Mem abort info:
> [  966.676497]   ESR = 0x96000006
> [  966.679543]   EC = 0x25: DABT (current EL), IL = 32 bits
> [  966.684848]   SET = 0, FnV = 0
> [  966.687895]   EA = 0, S1PTW = 0
> [  966.691028] Data abort info:
> [  966.693900]   ISV = 0, ISS = 0x00000006
> [  966.697729]   CM = 0, WnR = 0
> ....
> [  966.833467] Call trace:
> [  966.835904]  cn10k_mdo_stop+0x20/0xa0
> [  966.839557]  macsec_dev_stop+0xe8/0x11c
> [  966.843384]  __dev_close_many+0xbc/0x140
> [  966.847298]  dev_close_many+0x84/0x120
> [  966.851039]  rollback_registered_many+0x114/0x5bc
> [  966.855735]  unregister_netdevice_many.part.0+0x14/0xa0
> [  966.860952]  unregister_netdevice_many+0x18/0x24
> [  966.865560]  macsec_notify+0x1ac/0x1c0
> [  966.869303]  raw_notifier_call_chain+0x50/0x70
> [  966.873738]  call_netdevice_notifiers_info+0x34/0x7c
> [  966.878694]  rollback_registered_many+0x354/0x5bc
> [  966.883390]  unregister_netdevice_queue+0x88/0x10c
> [  966.888173]  unregister_netdev+0x20/0x30
> [  966.892090]  otx2_remove+0x8c/0x310
> [  966.895571]  pci_device_shutdown+0x30/0x70
> [  966.899660]  device_shutdown+0x11c/0x204
> [  966.903574]  __do_sys_reboot+0x208/0x290
> [  966.907487]  __arm64_sys_reboot+0x20/0x30
> [  966.911489]  el0_svc_handler+0x80/0x1c0
> [  966.915316]  el0_svc+0x8/0x180
> [  966.918362] Code: f9400000 f9400a64 91220014 f94b3403 (f9400060)
> [  966.924448] ---[ end trace 341778e799c3d8d7 ]---
> 
> Fixes: c54ffc73601c ("octeontx2-pf: mcs: Introduce MACSEC hardware offloading")
> Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
> Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
> Signed-off-by: Geetha sowjanya <gakula@marvell.com>
> ---
>  drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> index 9ec5f38d38a8..5f4402f7b03e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
> @@ -1065,6 +1065,9 @@ static int cn10k_mdo_stop(struct macsec_context *ctx)
>  	struct cn10k_mcs_txsc *txsc;
>  	int err;
>  
> +	if (!cfg)
> +		return 0;
> +
>  	txsc = cn10k_mcs_get_txsc(cfg, ctx->secy);
>  	if (!txsc)
>  		return -ENOENT;
> @@ -1146,6 +1149,9 @@ static int cn10k_mdo_del_secy(struct macsec_context *ctx)
>  	struct cn10k_mcs_cfg *cfg = pfvf->macsec_cfg;
>  	struct cn10k_mcs_txsc *txsc;
>  
> +	if (!cfg)
> +		return 0;

How did you get call to .mdo_del_secy if you didn't add any secy?

Thanks

> +
>  	txsc = cn10k_mcs_get_txsc(cfg, ctx->secy);
>  	if (!txsc)
>  		return -ENOENT;
> -- 
> 2.25.1
>
Subbaraya Sundeep April 24, 2023, 10:29 a.m. UTC | #2 
Hi,

>-----Original Message-----
>From: Leon Romanovsky <leon@kernel.org>
>Sent: Sunday, April 23, 2023 10:22 PM
>To: Geethasowjanya Akula <gakula@marvell.com>
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kuba@kernel.org;
>davem@davemloft.net; edumazet@google.com; pabeni@redhat.com;
>richardcochran@gmail.com; Sunil Kovvuri Goutham
><sgoutham@marvell.com>; Subbaraya Sundeep Bhatta
><sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>
>Subject: [EXT] Re: [net PATCH 5/9] octeontx2-pf: mcs: Fix NULL pointer
>dereferences
>
>On Sun, Apr 23, 2023 at 03:24:50PM +0530, Geetha sowjanya wrote:
>> From: Subbaraya Sundeep <sbhatta@marvell.com>
>>
>> When system is rebooted after creating macsec interface below NULL
>> pointer dereference crashes occurred. This patch fixes those crashes.
>>
>> [ 3324.406942] Unable to handle kernel NULL pointer dereference at
>> virtual address 0000000000000000 [ 3324.415726] Mem abort info:
>> [ 3324.418510]   ESR = 0x96000006
>> [ 3324.421557]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [ 3324.426865]   SET = 0, FnV = 0
>> [ 3324.429913]   EA = 0, S1PTW = 0
>> [ 3324.433047] Data abort info:
>> [ 3324.435921]   ISV = 0, ISS = 0x00000006
>> [ 3324.439748]   CM = 0, WnR = 0
>> ....
>> [ 3324.575915] Call trace:
>> [ 3324.578353]  cn10k_mdo_del_secy+0x24/0x180 [ 3324.582440]
>> macsec_common_dellink+0xec/0x120 [ 3324.586788]
>> macsec_notify+0x17c/0x1c0 [ 3324.590529]
>> raw_notifier_call_chain+0x50/0x70 [ 3324.594965]
>> call_netdevice_notifiers_info+0x34/0x7c
>> [ 3324.599921]  rollback_registered_many+0x354/0x5bc
>> [ 3324.604616]  unregister_netdevice_queue+0x88/0x10c
>> [ 3324.609399]  unregister_netdev+0x20/0x30 [ 3324.613313]
>> otx2_remove+0x8c/0x310 [ 3324.616794]  pci_device_shutdown+0x30/0x70
>[
>> 3324.620882]  device_shutdown+0x11c/0x204
>>
>> [  966.664930] Unable to handle kernel NULL pointer dereference at
>> virtual address 0000000000000000 [  966.673712] Mem abort info:
>> [  966.676497]   ESR = 0x96000006
>> [  966.679543]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [  966.684848]   SET = 0, FnV = 0
>> [  966.687895]   EA = 0, S1PTW = 0
>> [  966.691028] Data abort info:
>> [  966.693900]   ISV = 0, ISS = 0x00000006
>> [  966.697729]   CM = 0, WnR = 0
>> ....
>> [  966.833467] Call trace:
>> [  966.835904]  cn10k_mdo_stop+0x20/0xa0 [  966.839557]
>> macsec_dev_stop+0xe8/0x11c [  966.843384]
>__dev_close_many+0xbc/0x140
>> [  966.847298]  dev_close_many+0x84/0x120 [  966.851039]
>> rollback_registered_many+0x114/0x5bc
>> [  966.855735]  unregister_netdevice_many.part.0+0x14/0xa0
>> [  966.860952]  unregister_netdevice_many+0x18/0x24
>> [  966.865560]  macsec_notify+0x1ac/0x1c0 [  966.869303]
>> raw_notifier_call_chain+0x50/0x70 [  966.873738]
>> call_netdevice_notifiers_info+0x34/0x7c
>> [  966.878694]  rollback_registered_many+0x354/0x5bc
>> [  966.883390]  unregister_netdevice_queue+0x88/0x10c
>> [  966.888173]  unregister_netdev+0x20/0x30 [  966.892090]
>> otx2_remove+0x8c/0x310 [  966.895571]  pci_device_shutdown+0x30/0x70 [
>> 966.899660]  device_shutdown+0x11c/0x204 [  966.903574]
>> __do_sys_reboot+0x208/0x290 [  966.907487]
>> __arm64_sys_reboot+0x20/0x30 [  966.911489]
>> el0_svc_handler+0x80/0x1c0 [  966.915316]  el0_svc+0x8/0x180 [
>> 966.918362] Code: f9400000 f9400a64 91220014 f94b3403 (f9400060) [
>> 966.924448] ---[ end trace 341778e799c3d8d7 ]---
>>
>> Fixes: c54ffc73601c ("octeontx2-pf: mcs: Introduce MACSEC hardware
>> offloading")
>> Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
>> Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
>> Signed-off-by: Geetha sowjanya <gakula@marvell.com>
>> ---
>>  drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>> b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>> index 9ec5f38d38a8..5f4402f7b03e 100644
>> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>> @@ -1065,6 +1065,9 @@ static int cn10k_mdo_stop(struct macsec_context
>*ctx)
>>  	struct cn10k_mcs_txsc *txsc;
>>  	int err;
>>
>> +	if (!cfg)
>> +		return 0;
>> +
>>  	txsc = cn10k_mcs_get_txsc(cfg, ctx->secy);
>>  	if (!txsc)
>>  		return -ENOENT;
>> @@ -1146,6 +1149,9 @@ static int cn10k_mdo_del_secy(struct
>macsec_context *ctx)
>>  	struct cn10k_mcs_cfg *cfg = pfvf->macsec_cfg;
>>  	struct cn10k_mcs_txsc *txsc;
>>
>> +	if (!cfg)
>> +		return 0;
>
>How did you get call to .mdo_del_secy if you didn't add any secy?
>
>Thanks
>
It is because of the order of teardown in otx2_remove:
        cn10k_mcs_free(pf);
        unregister_netdev(netdev);

cn10k_mcs_free free the resources and makes cfg as NULL.
Later unregister_netdev calls mdo_del_secy and finds cfg as NULL.
Thanks for the review I will change the order and submit next version.

Sundeep

>> +
>>  	txsc = cn10k_mcs_get_txsc(cfg, ctx->secy);
>>  	if (!txsc)
>>  		return -ENOENT;
>> --
>> 2.25.1
>>
Jakub Kicinski April 25, 2023, 3:51 p.m. UTC | #3 
On Mon, 24 Apr 2023 10:29:02 +0000 Subbaraya Sundeep Bhatta wrote:
> >How did you get call to .mdo_del_secy if you didn't add any secy?
> >
> >Thanks
> >  
> It is because of the order of teardown in otx2_remove:
>         cn10k_mcs_free(pf);
>         unregister_netdev(netdev);
> 
> cn10k_mcs_free free the resources and makes cfg as NULL.
> Later unregister_netdev calls mdo_del_secy and finds cfg as NULL.
> Thanks for the review I will change the order and submit next version.

Leon, ack? Looks like the patches got "changes requested" but I see 
no other complaint.
Leon Romanovsky April 26, 2023, 6:16 a.m. UTC | #4 
On Tue, Apr 25, 2023 at 08:51:40AM -0700, Jakub Kicinski wrote:
> On Mon, 24 Apr 2023 10:29:02 +0000 Subbaraya Sundeep Bhatta wrote:
> > >How did you get call to .mdo_del_secy if you didn't add any secy?
> > >
> > >Thanks
> > >  
> > It is because of the order of teardown in otx2_remove:
> >         cn10k_mcs_free(pf);
> >         unregister_netdev(netdev);
> > 
> > cn10k_mcs_free free the resources and makes cfg as NULL.
> > Later unregister_netdev calls mdo_del_secy and finds cfg as NULL.
> > Thanks for the review I will change the order and submit next version.
> 
> Leon, ack? Looks like the patches got "changes requested" but I see 
> no other complaint.

Honestly, I was confused and didn't know what to answer, so decided to
see next version.

From one side Subbaraya said that it is possible (which was not
convincing to me, but ok, most time I'm wrong :)), from another
he said that he will submit next version.

Thanks
Leon Romanovsky April 26, 2023, 6:52 a.m. UTC | #5 
On Wed, Apr 26, 2023 at 09:16:54AM +0300, Leon Romanovsky wrote:
> On Tue, Apr 25, 2023 at 08:51:40AM -0700, Jakub Kicinski wrote:
> > On Mon, 24 Apr 2023 10:29:02 +0000 Subbaraya Sundeep Bhatta wrote:
> > > >How did you get call to .mdo_del_secy if you didn't add any secy?
> > > >
> > > >Thanks
> > > >  
> > > It is because of the order of teardown in otx2_remove:
> > >         cn10k_mcs_free(pf);
> > >         unregister_netdev(netdev);
> > > 
> > > cn10k_mcs_free free the resources and makes cfg as NULL.
> > > Later unregister_netdev calls mdo_del_secy and finds cfg as NULL.
> > > Thanks for the review I will change the order and submit next version.
> > 
> > Leon, ack? Looks like the patches got "changes requested" but I see 
> > no other complaint.
> 
> Honestly, I was confused and didn't know what to answer, so decided to
> see next version.
> 
> From one side Subbaraya said that it is possible (which was not
> convincing to me, but ok, most time I'm wrong :)), from another
> he said that he will submit next version.

Jakub,

v2 is perfectly fine.

Thanks

> 
> Thanks
diff mbox series

Patch

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
index 9ec5f38d38a8..5f4402f7b03e 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
@@ -1065,6 +1065,9 @@  static int cn10k_mdo_stop(struct macsec_context *ctx)
 	struct cn10k_mcs_txsc *txsc;
 	int err;
 
+	if (!cfg)
+		return 0;
+
 	txsc = cn10k_mcs_get_txsc(cfg, ctx->secy);
 	if (!txsc)
 		return -ENOENT;
@@ -1146,6 +1149,9 @@  static int cn10k_mdo_del_secy(struct macsec_context *ctx)
 	struct cn10k_mcs_cfg *cfg = pfvf->macsec_cfg;
 	struct cn10k_mcs_txsc *txsc;
 
+	if (!cfg)
+		return 0;
+
 	txsc = cn10k_mcs_get_txsc(cfg, ctx->secy);
 	if (!txsc)
 		return -ENOENT;