| Message ID | 20230423095454.21049-6-gakula@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Macsec fixes for CN10KB | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 18 this patch: 18 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 23 this patch: 23 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 24 this patch: 24 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 18 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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 >
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 >>
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.
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
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 --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;