| Message ID | 20190715143705.117908-8-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Harden list_for_each_entry_rcu() and family | expand |
On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote: > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a > reader-lock held, because the pci_mmcfg_lock is already held. Make this > known to the list macro so that it fixes new lockdep warnings that > trigger due to lockdep checks added to list_for_each_entry_rcu(). > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> Ingo takes care of most patches to this file, but FWIW, Acked-by: Bjorn Helgaas <bhelgaas@google.com> I would personally prefer if you capitalized the subject to match the "x86/PCI:" convention that's used fairly consistently in arch/x86/pci/. Also, I didn't apply this to be sure, but it looks like this might make a line or two wider than 80 columns, which I would rewrap if I were applying this. > --- > arch/x86/pci/mmconfig-shared.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > index 7389db538c30..6fa42e9c4e6f 100644 > --- a/arch/x86/pci/mmconfig-shared.c > +++ b/arch/x86/pci/mmconfig-shared.c > @@ -29,6 +29,7 @@ > static bool pci_mmcfg_running_state; > static bool pci_mmcfg_arch_init_failed; > static DEFINE_MUTEX(pci_mmcfg_lock); > +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map) > > LIST_HEAD(pci_mmcfg_list); > > @@ -54,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new) > struct pci_mmcfg_region *cfg; > > /* keep list sorted by segment and starting bus number */ > - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) { > + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) { > if (cfg->segment > new->segment || > (cfg->segment == new->segment && > cfg->start_bus >= new->start_bus)) { > @@ -118,7 +119,7 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus) > { > struct pci_mmcfg_region *cfg; > > - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) > + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) > if (cfg->segment == segment && > cfg->start_bus <= bus && bus <= cfg->end_bus) > return cfg; > -- > 2.22.0.510.g264f2c817a-goog >
On Mon, Jul 15, 2019 at 03:02:35PM -0500, Bjorn Helgaas wrote: > On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote: > > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a > > reader-lock held, because the pci_mmcfg_lock is already held. Make this > > known to the list macro so that it fixes new lockdep warnings that > > trigger due to lockdep checks added to list_for_each_entry_rcu(). > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Ingo takes care of most patches to this file, but FWIW, > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> Thanks. > I would personally prefer if you capitalized the subject to match the > "x86/PCI:" convention that's used fairly consistently in > arch/x86/pci/. > > Also, I didn't apply this to be sure, but it looks like this might > make a line or two wider than 80 columns, which I would rewrap if I > were applying this. Updated below is the patch with the nits corrected: ---8<----------------------- From 73fab09d7e33ca2110c24215f8ed428c12625dbe Mon Sep 17 00:00:00 2001 From: "Joel Fernandes (Google)" <joel@joelfernandes.org> Date: Sat, 1 Jun 2019 15:05:49 -0400 Subject: [PATCH] x86/PCI: Pass lockdep condition to pcm_mmcfg_list iterator (v1) The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a reader-lock held, because the pci_mmcfg_lock is already held. Make this known to the list macro so that it fixes new lockdep warnings that trigger due to lockdep checks added to list_for_each_entry_rcu(). Acked-by: Bjorn Helgaas <bhelgaas@google.com> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- arch/x86/pci/mmconfig-shared.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 7389db538c30..9e3250ec5a37 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -29,6 +29,7 @@ static bool pci_mmcfg_running_state; static bool pci_mmcfg_arch_init_failed; static DEFINE_MUTEX(pci_mmcfg_lock); +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map) LIST_HEAD(pci_mmcfg_list); @@ -54,7 +55,8 @@ static void list_add_sorted(struct pci_mmcfg_region *new) struct pci_mmcfg_region *cfg; /* keep list sorted by segment and starting bus number */ - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) { + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, + pci_mmcfg_lock_held()) { if (cfg->segment > new->segment || (cfg->segment == new->segment && cfg->start_bus >= new->start_bus)) { @@ -118,7 +120,8 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus) { struct pci_mmcfg_region *cfg; - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list + pci_mmcfg_lock_held()) if (cfg->segment == segment && cfg->start_bus <= bus && bus <= cfg->end_bus) return cfg;
On Tue, Jul 16, 2019 at 12:03:03AM -0400, Joel Fernandes wrote: > On Mon, Jul 15, 2019 at 03:02:35PM -0500, Bjorn Helgaas wrote: > > On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote: > > > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a > > > reader-lock held, because the pci_mmcfg_lock is already held. Make this > > > known to the list macro so that it fixes new lockdep warnings that > > > trigger due to lockdep checks added to list_for_each_entry_rcu(). > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > Ingo takes care of most patches to this file, but FWIW, > > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > > Thanks. > > > I would personally prefer if you capitalized the subject to match the > > "x86/PCI:" convention that's used fairly consistently in > > arch/x86/pci/. > > > > Also, I didn't apply this to be sure, but it looks like this might > > make a line or two wider than 80 columns, which I would rewrap if I > > were applying this. > > Updated below is the patch with the nits corrected: I am OK with this going either way, but it does depend on an earlier patch. Thanx, Paul > ---8<----------------------- > > >From 73fab09d7e33ca2110c24215f8ed428c12625dbe Mon Sep 17 00:00:00 2001 > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > Date: Sat, 1 Jun 2019 15:05:49 -0400 > Subject: [PATCH] x86/PCI: Pass lockdep condition to pcm_mmcfg_list iterator > (v1) > > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a > reader-lock held, because the pci_mmcfg_lock is already held. Make this > known to the list macro so that it fixes new lockdep warnings that > trigger due to lockdep checks added to list_for_each_entry_rcu(). > > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > arch/x86/pci/mmconfig-shared.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c > index 7389db538c30..9e3250ec5a37 100644 > --- a/arch/x86/pci/mmconfig-shared.c > +++ b/arch/x86/pci/mmconfig-shared.c > @@ -29,6 +29,7 @@ > static bool pci_mmcfg_running_state; > static bool pci_mmcfg_arch_init_failed; > static DEFINE_MUTEX(pci_mmcfg_lock); > +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map) > > LIST_HEAD(pci_mmcfg_list); > > @@ -54,7 +55,8 @@ static void list_add_sorted(struct pci_mmcfg_region *new) > struct pci_mmcfg_region *cfg; > > /* keep list sorted by segment and starting bus number */ > - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) { > + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, > + pci_mmcfg_lock_held()) { > if (cfg->segment > new->segment || > (cfg->segment == new->segment && > cfg->start_bus >= new->start_bus)) { > @@ -118,7 +120,8 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus) > { > struct pci_mmcfg_region *cfg; > > - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) > + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list > + pci_mmcfg_lock_held()) > if (cfg->segment == segment && > cfg->start_bus <= bus && bus <= cfg->end_bus) > return cfg; > -- > 2.22.0.510.g264f2c817a-goog >
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c index 7389db538c30..6fa42e9c4e6f 100644 --- a/arch/x86/pci/mmconfig-shared.c +++ b/arch/x86/pci/mmconfig-shared.c @@ -29,6 +29,7 @@ static bool pci_mmcfg_running_state; static bool pci_mmcfg_arch_init_failed; static DEFINE_MUTEX(pci_mmcfg_lock); +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map) LIST_HEAD(pci_mmcfg_list); @@ -54,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new) struct pci_mmcfg_region *cfg; /* keep list sorted by segment and starting bus number */ - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) { + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) { if (cfg->segment > new->segment || (cfg->segment == new->segment && cfg->start_bus >= new->start_bus)) { @@ -118,7 +119,7 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus) { struct pci_mmcfg_region *cfg; - list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) + list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) if (cfg->segment == segment && cfg->start_bus <= bus && bus <= cfg->end_bus) return cfg;
The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a reader-lock held, because the pci_mmcfg_lock is already held. Make this known to the list macro so that it fixes new lockdep warnings that trigger due to lockdep checks added to list_for_each_entry_rcu(). Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- arch/x86/pci/mmconfig-shared.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)