| Message ID | 20250602191017.60936-1-Ashish.Kalra@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: ccp: Fix SNP panic notifier unregistration | expand |
On 6/2/25 14:10, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > Panic notifiers are invoked with RCU read lock held and when the > SNP panic notifier tries to unregister itself from the panic > notifier callback itself it causes a deadlock as notifier > unregistration does RCU synchronization. You mean that during a panic, __sev_snp_shutdown_locked() is trying to unregister the notifier? Wouldn't it be better to check if a panic is in progress and not try to perform the unregister? Or, is snp_panic_notifier() resilient enough to just always have it registered / unregistered on module load/unload? Also, wouldn't a better check be snp_panic_notifier.next != NULL during sev_pci_exit()? Thanks, Tom > > Fix SNP panic notifier to unregister itself during module unload > if SNP is initialized. > > Fixes: 19860c3274fb ("crypto: ccp - Register SNP panic notifier only if SNP is enabled") > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> > --- > drivers/crypto/ccp/sev-dev.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c > index 8fb94c5f006a..942d93da1136 100644 > --- a/drivers/crypto/ccp/sev-dev.c > +++ b/drivers/crypto/ccp/sev-dev.c > @@ -1787,9 +1787,6 @@ static int __sev_snp_shutdown_locked(int *error, bool panic) > sev->snp_initialized = false; > dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n"); > > - atomic_notifier_chain_unregister(&panic_notifier_list, > - &snp_panic_notifier); > - > /* Reset TMR size back to default */ > sev_es_tmr_size = SEV_TMR_SIZE; > > @@ -2562,4 +2559,8 @@ void sev_pci_exit(void) > return; > > sev_firmware_shutdown(sev); > + > + if (sev->snp_initialized) > + atomic_notifier_chain_unregister(&panic_notifier_list, > + &snp_panic_notifier); > }
Hello Tom, On 6/4/2025 10:57 AM, Tom Lendacky wrote: > On 6/2/25 14:10, Ashish Kalra wrote: >> From: Ashish Kalra <ashish.kalra@amd.com> >> >> Panic notifiers are invoked with RCU read lock held and when the >> SNP panic notifier tries to unregister itself from the panic >> notifier callback itself it causes a deadlock as notifier >> unregistration does RCU synchronization. > > You mean that during a panic, __sev_snp_shutdown_locked() is trying to > unregister the notifier? Yes. This is the code path : snp_shutdown_on_panic() -> __sev_firmware_shutdown() -> __sev_snp_shutdown_locked() -> atomic_notifier_chain_unregister(.., &snp_panic_notifier) So atomic_notifier_chain_unregister() is being invoked from the panic notifier (context) itself. > > Wouldn't it be better to check if a panic is in progress and not try to > perform the unregister? Yes, actually it will be easier to do that by simply checking the panic parameter in __sev_snp_shutdown_locked(). > > Or, is snp_panic_notifier() resilient enough to just always have it > registered / unregistered on module load/unload? > For registration it makes more sense to do that only if SNP is being initialized and as part of __sev_snp_init_locked(), for unregistration see notes below. > Also, wouldn't a better check be snp_panic_notifier.next != NULL during > sev_pci_exit()? Actually i can't use snp_initialized here as it will always be false. But i also can't use snp_panic_notifier.next != NULL, because if it has already been unregistered then snp_panic_notifier.next may be non-NULL as unregister() would have chained it to the next notifier on the chain. Actually i can simply call atomic_notifier_chain_unregister() unconditionally during module unload as it handles the case of the specific notifier already unregistered and/or not added to the chain. Thanks, Ashish > > Thanks, > Tom > >> >> Fix SNP panic notifier to unregister itself during module unload >> if SNP is initialized. >> >> Fixes: 19860c3274fb ("crypto: ccp - Register SNP panic notifier only if SNP is enabled") >> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com> >> --- >> drivers/crypto/ccp/sev-dev.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c >> index 8fb94c5f006a..942d93da1136 100644 >> --- a/drivers/crypto/ccp/sev-dev.c >> +++ b/drivers/crypto/ccp/sev-dev.c >> @@ -1787,9 +1787,6 @@ static int __sev_snp_shutdown_locked(int *error, bool panic) >> sev->snp_initialized = false; >> dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n"); >> >> - atomic_notifier_chain_unregister(&panic_notifier_list, >> - &snp_panic_notifier); >> - >> /* Reset TMR size back to default */ >> sev_es_tmr_size = SEV_TMR_SIZE; >> >> @@ -2562,4 +2559,8 @@ void sev_pci_exit(void) >> return; >> >> sev_firmware_shutdown(sev); >> + >> + if (sev->snp_initialized) >> + atomic_notifier_chain_unregister(&panic_notifier_list, >> + &snp_panic_notifier); >> }
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index 8fb94c5f006a..942d93da1136 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1787,9 +1787,6 @@ static int __sev_snp_shutdown_locked(int *error, bool panic) sev->snp_initialized = false; dev_dbg(sev->dev, "SEV-SNP firmware shutdown\n"); - atomic_notifier_chain_unregister(&panic_notifier_list, - &snp_panic_notifier); - /* Reset TMR size back to default */ sev_es_tmr_size = SEV_TMR_SIZE; @@ -2562,4 +2559,8 @@ void sev_pci_exit(void) return; sev_firmware_shutdown(sev); + + if (sev->snp_initialized) + atomic_notifier_chain_unregister(&panic_notifier_list, + &snp_panic_notifier); }