* [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event()
@ 2017-04-19 7:39 Russell Currey
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Russell Currey @ 2017-04-19 7:39 UTC (permalink / raw)
To: linuxppc-dev; +Cc: aik, Russell Currey
eeh_handle_special_event() is called when an EEH event is detected but
can't be narrowed down to a specific PE. This function looks through
every PE to find one in an erroneous state, then calls the regular event
handler eeh_handle_normal_event() once it knows which PE has an error.
However, if eeh_handle_normal_event() found that the PE cannot possibly
be recovered, it will free it, rendering the passed PE stale.
This leads to a use after free in eeh_handle_special_event() as it attempts to
clear the "recovering" state on the PE after eeh_handle_normal_event() returns.
Thus, make sure the PE is valid when attempting to clear state in
eeh_handle_special_event().
Cc: <stable@vger.kernel.org> #3.10+
Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
V2: check a specific return path instead of looking at the PE itself
V3: use a bool instead of a non-specific int return
---
arch/powerpc/kernel/eeh_driver.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index b94887165a10..e50d1470714f 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -724,7 +724,7 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
*/
#define MAX_WAIT_FOR_RECOVERY 300
-static void eeh_handle_normal_event(struct eeh_pe *pe)
+static bool eeh_handle_normal_event(struct eeh_pe *pe)
{
struct pci_bus *frozen_bus;
struct eeh_dev *edev, *tmp;
@@ -736,7 +736,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
if (!frozen_bus) {
pr_err("%s: Cannot find PCI bus for PHB#%x-PE#%x\n",
__func__, pe->phb->global_number, pe->addr);
- return;
+ return false;
}
eeh_pe_update_time_stamp(pe);
@@ -870,7 +870,7 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
pr_info("EEH: Notify device driver to resume\n");
eeh_pe_dev_traverse(pe, eeh_report_resume, NULL);
- return;
+ return false;
excess_failures:
/*
@@ -915,8 +915,12 @@ static void eeh_handle_normal_event(struct eeh_pe *pe)
pci_lock_rescan_remove();
pci_hp_remove_devices(frozen_bus);
pci_unlock_rescan_remove();
+
+ /* The passed PE should no longer be used */
+ return true;
}
}
+ return false;
}
static void eeh_handle_special_event(void)
@@ -982,7 +986,14 @@ static void eeh_handle_special_event(void)
*/
if (rc == EEH_NEXT_ERR_FROZEN_PE ||
rc == EEH_NEXT_ERR_FENCED_PHB) {
- eeh_handle_normal_event(pe);
+ /*
+ * eeh_handle_normal_event() can make the PE stale if it
+ * determines that the PE cannot possibly be recovered.
+ * Don't modify the PE state if that's the case.
+ */
+ if (eeh_handle_normal_event(pe))
+ continue;
+
eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
} else {
pci_lock_rescan_remove();
--
2.12.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions
2017-04-19 7:39 [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event() Russell Currey
@ 2017-04-19 7:39 ` Russell Currey
2017-04-19 23:48 ` Gavin Shan
` (3 more replies)
2017-04-19 23:49 ` [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event() Gavin Shan
2017-05-03 22:18 ` [v3, " Michael Ellerman
2 siblings, 4 replies; 10+ messages in thread
From: Russell Currey @ 2017-04-19 7:39 UTC (permalink / raw)
To: linuxppc-dev; +Cc: aik, Russell Currey
Remove unnecessary tags in eeh_handle_normal_event(), and add function
comments for eeh_handle_normal_event() and eeh_handle_special_event().
The only functional difference is that in the case of a PE reaching the
maximum number of failures, rather than one message telling you of this
and suggesting you reseat the device, there are two separate messages.
Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
V3: new. Thanks to Alexey for the suggestions.
---
arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++++++++++++++------------
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index e50d1470714f..c405c79e50cd 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c
@@ -724,6 +724,15 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
*/
#define MAX_WAIT_FOR_RECOVERY 300
+/**
+ * eeh_handle_normal_event - Handle EEH events on a specific PE
+ * @pe: EEH PE
+ *
+ * Attempts to recover the given PE. If recovery fails or the PE has failed
+ * too many times, remove the PE.
+ *
+ * Returns true if @pe should no longer be used, else false.
+ */
static bool eeh_handle_normal_event(struct eeh_pe *pe)
{
struct pci_bus *frozen_bus;
@@ -741,8 +750,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
eeh_pe_update_time_stamp(pe);
pe->freeze_count++;
- if (pe->freeze_count > eeh_max_freezes)
- goto excess_failures;
+ if (pe->freeze_count > eeh_max_freezes) {
+ pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
+ "last hour and has been permanently disabled.\n",
+ pe->phb->global_number, pe->addr,
+ pe->freeze_count);
+ goto hard_fail;
+ }
pr_warn("EEH: This PCI device has failed %d times in the last hour\n",
pe->freeze_count);
@@ -872,25 +886,16 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
return false;
-excess_failures:
+hard_fail:
/*
* About 90% of all real-life EEH failures in the field
* are due to poorly seated PCI cards. Only 10% or so are
* due to actual, failed cards.
*/
- pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
- "last hour and has been permanently disabled.\n"
- "Please try reseating or replacing it.\n",
- pe->phb->global_number, pe->addr,
- pe->freeze_count);
- goto perm_error;
-
-hard_fail:
pr_err("EEH: Unable to recover from failure from PHB#%x-PE#%x.\n"
"Please try reseating or replacing it\n",
pe->phb->global_number, pe->addr);
-perm_error:
eeh_slot_error_detail(pe, EEH_LOG_PERM);
/* Notify all devices that they're about to go down. */
@@ -923,6 +928,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
return false;
}
+/**
+ * eeh_handle_special_event - Handle EEH events without a specific failing PE
+ *
+ * Called when an EEH event is detected but can't be narrowed down to a
+ * specific PE. Iterates through possible failures and handles them as
+ * necessary.
+ */
static void eeh_handle_special_event(void)
{
struct eeh_pe *pe, *phb_pe;
--
2.12.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
@ 2017-04-19 23:48 ` Gavin Shan
2017-04-20 1:03 ` Russell Currey
2017-04-20 0:36 ` Andrew Donnellan
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Gavin Shan @ 2017-04-19 23:48 UTC (permalink / raw)
To: Russell Currey; +Cc: linuxppc-dev, aik
On Wed, Apr 19, 2017 at 05:39:27PM +1000, Russell Currey wrote:
>Remove unnecessary tags in eeh_handle_normal_event(), and add function
>comments for eeh_handle_normal_event() and eeh_handle_special_event().
>
>The only functional difference is that in the case of a PE reaching the
>maximum number of failures, rather than one message telling you of this
>and suggesting you reseat the device, there are two separate messages.
>
>Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>Signed-off-by: Russell Currey <ruscur@russell.cc>
>---
>V3: new. Thanks to Alexey for the suggestions.
>---
> arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++++++++++++++------------
> 1 file changed, 24 insertions(+), 12 deletions(-)
>
>diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
>index e50d1470714f..c405c79e50cd 100644
>--- a/arch/powerpc/kernel/eeh_driver.c
>+++ b/arch/powerpc/kernel/eeh_driver.c
>@@ -724,6 +724,15 @@ static int eeh_reset_device(struct eeh_pe *pe, struct pci_bus *bus,
> */
> #define MAX_WAIT_FOR_RECOVERY 300
>
>+/**
>+ * eeh_handle_normal_event - Handle EEH events on a specific PE
>+ * @pe: EEH PE
>+ *
>+ * Attempts to recover the given PE. If recovery fails or the PE has failed
>+ * too many times, remove the PE.
>+ *
>+ * Returns true if @pe should no longer be used, else false.
>+ */
I think this bit of comments would be part of PATCH[1/2]? Also, the
comments needn't to be in any document as it's static one. I guess
you might not want it to show in stable branches as PATCH[1/2] has
been tagged as stable. It's fine if that's the case.
> static bool eeh_handle_normal_event(struct eeh_pe *pe)
> {
> struct pci_bus *frozen_bus;
>@@ -741,8 +750,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
>
> eeh_pe_update_time_stamp(pe);
> pe->freeze_count++;
>- if (pe->freeze_count > eeh_max_freezes)
>- goto excess_failures;
>+ if (pe->freeze_count > eeh_max_freezes) {
>+ pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
>+ "last hour and has been permanently disabled.\n",
>+ pe->phb->global_number, pe->addr,
>+ pe->freeze_count);
>+ goto hard_fail;
>+ }
> pr_warn("EEH: This PCI device has failed %d times in the last hour\n",
> pe->freeze_count);
>
>@@ -872,25 +886,16 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
>
> return false;
>
>-excess_failures:
>+hard_fail:
> /*
> * About 90% of all real-life EEH failures in the field
> * are due to poorly seated PCI cards. Only 10% or so are
> * due to actual, failed cards.
> */
This bit of comments apply to "excess_failures" only, so it would
be moved together with the pr_err(). Frankly speaking, I don't see
the benebit of the cleanup. "excess_failure" in the original code
indicates the case (excessive failures) explicitly, which is nice.
However, it's not a big deal.
>- pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
>- "last hour and has been permanently disabled.\n"
>- "Please try reseating or replacing it.\n",
>- pe->phb->global_number, pe->addr,
>- pe->freeze_count);
>- goto perm_error;
>-
>-hard_fail:
> pr_err("EEH: Unable to recover from failure from PHB#%x-PE#%x.\n"
> "Please try reseating or replacing it\n",
> pe->phb->global_number, pe->addr);
>
>-perm_error:
We will have the message from above pr_err() for "perm_error" case, but
we don't have that in original code.
> eeh_slot_error_detail(pe, EEH_LOG_PERM);
>
> /* Notify all devices that they're about to go down. */
>@@ -923,6 +928,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
> return false;
> }
>
>+/**
>+ * eeh_handle_special_event - Handle EEH events without a specific failing PE
>+ *
>+ * Called when an EEH event is detected but can't be narrowed down to a
>+ * specific PE. Iterates through possible failures and handles them as
>+ * necessary.
>+ */
> static void eeh_handle_special_event(void)
> {
> struct eeh_pe *pe, *phb_pe;
Thanks,
Gavin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event()
2017-04-19 7:39 [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event() Russell Currey
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
@ 2017-04-19 23:49 ` Gavin Shan
2017-05-03 22:18 ` [v3, " Michael Ellerman
2 siblings, 0 replies; 10+ messages in thread
From: Gavin Shan @ 2017-04-19 23:49 UTC (permalink / raw)
To: Russell Currey; +Cc: linuxppc-dev, aik
On Wed, Apr 19, 2017 at 05:39:26PM +1000, Russell Currey wrote:
>eeh_handle_special_event() is called when an EEH event is detected but
>can't be narrowed down to a specific PE. This function looks through
>every PE to find one in an erroneous state, then calls the regular event
>handler eeh_handle_normal_event() once it knows which PE has an error.
>
>However, if eeh_handle_normal_event() found that the PE cannot possibly
>be recovered, it will free it, rendering the passed PE stale.
>This leads to a use after free in eeh_handle_special_event() as it attempts to
>clear the "recovering" state on the PE after eeh_handle_normal_event() returns.
>
>Thus, make sure the PE is valid when attempting to clear state in
>eeh_handle_special_event().
>
>Cc: <stable@vger.kernel.org> #3.10+
>Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>Signed-off-by: Russell Currey <ruscur@russell.cc>
Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
2017-04-19 23:48 ` Gavin Shan
@ 2017-04-20 0:36 ` Andrew Donnellan
2017-04-20 1:24 ` Gavin Shan
2017-05-03 22:18 ` [v3, " Michael Ellerman
3 siblings, 0 replies; 10+ messages in thread
From: Andrew Donnellan @ 2017-04-20 0:36 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev; +Cc: aik
On 19/04/17 17:39, Russell Currey wrote:
> Remove unnecessary tags in eeh_handle_normal_event(), and add function
> comments for eeh_handle_normal_event() and eeh_handle_special_event().
>
> The only functional difference is that in the case of a PE reaching the
> maximum number of failures, rather than one message telling you of this
> and suggesting you reseat the device, there are two separate messages.
>
> Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions
2017-04-19 23:48 ` Gavin Shan
@ 2017-04-20 1:03 ` Russell Currey
2017-04-20 1:26 ` Gavin Shan
0 siblings, 1 reply; 10+ messages in thread
From: Russell Currey @ 2017-04-20 1:03 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev, aik
On Thu, 2017-04-20 at 09:48 +1000, Gavin Shan wrote:
> On Wed, Apr 19, 2017 at 05:39:27PM +1000, Russell Currey wrote:
> > Remove unnecessary tags in eeh_handle_normal_event(), and add function
> > comments for eeh_handle_normal_event() and eeh_handle_special_event().
> >
> > The only functional difference is that in the case of a PE reaching the
> > maximum number of failures, rather than one message telling you of this
> > and suggesting you reseat the device, there are two separate messages.
> >
> > Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> > V3: new. Thanks to Alexey for the suggestions.
> > ---
> > arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++++++++++++++------------
> > 1 file changed, 24 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/eeh_driver.c
> > b/arch/powerpc/kernel/eeh_driver.c
> > index e50d1470714f..c405c79e50cd 100644
> > --- a/arch/powerpc/kernel/eeh_driver.c
> > +++ b/arch/powerpc/kernel/eeh_driver.c
> > @@ -724,6 +724,15 @@ static int eeh_reset_device(struct eeh_pe *pe, struct
> > pci_bus *bus,
> > */
> > #define MAX_WAIT_FOR_RECOVERY 300
> >
> > +/**
> > + * eeh_handle_normal_event - Handle EEH events on a specific PE
> > + * @pe: EEH PE
> > + *
> > + * Attempts to recover the given PE. If recovery fails or the PE has
> > failed
> > + * too many times, remove the PE.
> > + *
> > + * Returns true if @pe should no longer be used, else false.
> > + */
>
> I think this bit of comments would be part of PATCH[1/2]? Also, the
> comments needn't to be in any document as it's static one. I guess
> you might not want it to show in stable branches as PATCH[1/2] has
> been tagged as stable. It's fine if that's the case.
Yeah, I asked mpe about this and he said it's easier to get things into stable
if they are purely fixes.
>
> > static bool eeh_handle_normal_event(struct eeh_pe *pe)
> > {
> > struct pci_bus *frozen_bus;
> > @@ -741,8 +750,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
> >
> > eeh_pe_update_time_stamp(pe);
> > pe->freeze_count++;
> > - if (pe->freeze_count > eeh_max_freezes)
> > - goto excess_failures;
> > + if (pe->freeze_count > eeh_max_freezes) {
> > + pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
> > + "last hour and has been permanently disabled.\n",
> > + pe->phb->global_number, pe->addr,
> > + pe->freeze_count);
> > + goto hard_fail;
> > + }
> > pr_warn("EEH: This PCI device has failed %d times in the last hour\n",
> > pe->freeze_count);
> >
> > @@ -872,25 +886,16 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
> >
> > return false;
> >
> > -excess_failures:
> > +hard_fail:
> > /*
> > * About 90% of all real-life EEH failures in the field
> > * are due to poorly seated PCI cards. Only 10% or so are
> > * due to actual, failed cards.
> > */
>
> This bit of comments apply to "excess_failures" only, so it would
> be moved together with the pr_err(). Frankly speaking, I don't see
> the benebit of the cleanup. "excess_failure" in the original code
> indicates the case (excessive failures) explicitly, which is nice.
> However, it's not a big deal.
It applies to anything mentioning "reseating or replacing", which used to be two
print statements but with this patch is only one.
>
> > - pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
> > - "last hour and has been permanently disabled.\n"
> > - "Please try reseating or replacing it.\n",
> > - pe->phb->global_number, pe->addr,
> > - pe->freeze_count);
> > - goto perm_error;
> > -
> > -hard_fail:
> > pr_err("EEH: Unable to recover from failure from PHB#%x-PE#%x.\n"
> > "Please try reseating or replacing it\n",
> > pe->phb->global_number, pe->addr);
> >
> > -perm_error:
>
> We will have the message from above pr_err() for "perm_error" case, but
> we don't have that in original code.
Yes, there's a slight difference here. I chose to print two messages in the
excess failures case, one stating that the failure as been hit and then also
printing the general permanent failure message. I don't think it makes much of
a difference, and it saves a tag. I definitely like only having one goto in the
function.
Thanks for the review.
>
> > eeh_slot_error_detail(pe, EEH_LOG_PERM);
> >
> > /* Notify all devices that they're about to go down. */
> > @@ -923,6 +928,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
> > return false;
> > }
> >
> > +/**
> > + * eeh_handle_special_event - Handle EEH events without a specific failing
> > PE
> > + *
> > + * Called when an EEH event is detected but can't be narrowed down to a
> > + * specific PE. Iterates through possible failures and handles them as
> > + * necessary.
> > + */
> > static void eeh_handle_special_event(void)
> > {
> > struct eeh_pe *pe, *phb_pe;
>
> Thanks,
> Gavin
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
2017-04-19 23:48 ` Gavin Shan
2017-04-20 0:36 ` Andrew Donnellan
@ 2017-04-20 1:24 ` Gavin Shan
2017-05-03 22:18 ` [v3, " Michael Ellerman
3 siblings, 0 replies; 10+ messages in thread
From: Gavin Shan @ 2017-04-20 1:24 UTC (permalink / raw)
To: Russell Currey; +Cc: linuxppc-dev, aik
On Wed, Apr 19, 2017 at 05:39:27PM +1000, Russell Currey wrote:
>Remove unnecessary tags in eeh_handle_normal_event(), and add function
>comments for eeh_handle_normal_event() and eeh_handle_special_event().
>
>The only functional difference is that in the case of a PE reaching the
>maximum number of failures, rather than one message telling you of this
>and suggesting you reseat the device, there are two separate messages.
>
>Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>Signed-off-by: Russell Currey <ruscur@russell.cc>
Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions
2017-04-20 1:03 ` Russell Currey
@ 2017-04-20 1:26 ` Gavin Shan
0 siblings, 0 replies; 10+ messages in thread
From: Gavin Shan @ 2017-04-20 1:26 UTC (permalink / raw)
To: Russell Currey; +Cc: Gavin Shan, linuxppc-dev, aik
On Thu, Apr 20, 2017 at 11:03:57AM +1000, Russell Currey wrote:
>On Thu, 2017-04-20 at 09:48 +1000, Gavin Shan wrote:
>> On Wed, Apr 19, 2017 at 05:39:27PM +1000, Russell Currey wrote:
>> > Remove unnecessary tags in eeh_handle_normal_event(), and add function
>> > comments for eeh_handle_normal_event() and eeh_handle_special_event().
>> >
>> > The only functional difference is that in the case of a PE reaching the
>> > maximum number of failures, rather than one message telling you of this
>> > and suggesting you reseat the device, there are two separate messages.
>> >
>> > Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> > Signed-off-by: Russell Currey <ruscur@russell.cc>
>> > ---
>> > V3: new. Thanks to Alexey for the suggestions.
>> > ---
>> > arch/powerpc/kernel/eeh_driver.c | 36 ++++++++++++++++++++++++------------
>> > 1 file changed, 24 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/arch/powerpc/kernel/eeh_driver.c
>> > b/arch/powerpc/kernel/eeh_driver.c
>> > index e50d1470714f..c405c79e50cd 100644
>> > --- a/arch/powerpc/kernel/eeh_driver.c
>> > +++ b/arch/powerpc/kernel/eeh_driver.c
>> > @@ -724,6 +724,15 @@ static int eeh_reset_device(struct eeh_pe *pe, struct
>> > pci_bus *bus,
>> > */
>> > #define MAX_WAIT_FOR_RECOVERY 300
>> >
>> > +/**
>> > + * eeh_handle_normal_event - Handle EEH events on a specific PE
>> > + * @pe: EEH PE
>> > + *
>> > + * Attempts to recover the given PE. If recovery fails or the PE has
>> > failed
>> > + * too many times, remove the PE.
>> > + *
>> > + * Returns true if @pe should no longer be used, else false.
>> > + */
>>
>> I think this bit of comments would be part of PATCH[1/2]? Also, the
>> comments needn't to be in any document as it's static one. I guess
>> you might not want it to show in stable branches as PATCH[1/2] has
>> been tagged as stable. It's fine if that's the case.
>
>Yeah, I asked mpe about this and he said it's easier to get things into stable
>if they are purely fixes.
>
>>
>> > static bool eeh_handle_normal_event(struct eeh_pe *pe)
>> > {
>> > struct pci_bus *frozen_bus;
>> > @@ -741,8 +750,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
>> >
>> > eeh_pe_update_time_stamp(pe);
>> > pe->freeze_count++;
>> > - if (pe->freeze_count > eeh_max_freezes)
>> > - goto excess_failures;
>> > + if (pe->freeze_count > eeh_max_freezes) {
>> > + pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
>> > + "last hour and has been permanently disabled.\n",
>> > + pe->phb->global_number, pe->addr,
>> > + pe->freeze_count);
>> > + goto hard_fail;
>> > + }
>> > pr_warn("EEH: This PCI device has failed %d times in the last hour\n",
>> > pe->freeze_count);
>> >
>> > @@ -872,25 +886,16 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
>> >
>> > return false;
>> >
>> > -excess_failures:
>> > +hard_fail:
>> > /*
>> > * About 90% of all real-life EEH failures in the field
>> > * are due to poorly seated PCI cards. Only 10% or so are
>> > * due to actual, failed cards.
>> > */
>>
>> This bit of comments apply to "excess_failures" only, so it would
>> be moved together with the pr_err(). Frankly speaking, I don't see
>> the benebit of the cleanup. "excess_failure" in the original code
>> indicates the case (excessive failures) explicitly, which is nice.
>> However, it's not a big deal.
>
>It applies to anything mentioning "reseating or replacing", which used to be two
> print statements but with this patch is only one.
>
>>
>> > - pr_err("EEH: PHB#%x-PE#%x has failed %d times in the\n"
>> > - "last hour and has been permanently disabled.\n"
>> > - "Please try reseating or replacing it.\n",
>> > - pe->phb->global_number, pe->addr,
>> > - pe->freeze_count);
>> > - goto perm_error;
>> > -
>> > -hard_fail:
>> > pr_err("EEH: Unable to recover from failure from PHB#%x-PE#%x.\n"
>> > "Please try reseating or replacing it\n",
>> > pe->phb->global_number, pe->addr);
>> >
>> > -perm_error:
>>
>> We will have the message from above pr_err() for "perm_error" case, but
>> we don't have that in original code.
>
>Yes, there's a slight difference here. I chose to print two messages in the
>excess failures case, one stating that the failure as been hit and then also
>printing the general permanent failure message. I don't think it makes much of
>a difference, and it saves a tag. I definitely like only having one goto in the
>function.
>
>Thanks for the review.
>
Yeah, avoiding unnecessary goto is always nice. I give my RB in another
reply.
Thanks,
Gavin
>>
>> > eeh_slot_error_detail(pe, EEH_LOG_PERM);
>> >
>> > /* Notify all devices that they're about to go down. */
>> > @@ -923,6 +928,13 @@ static bool eeh_handle_normal_event(struct eeh_pe *pe)
>> > return false;
>> > }
>> >
>> > +/**
>> > + * eeh_handle_special_event - Handle EEH events without a specific failing
>> > PE
>> > + *
>> > + * Called when an EEH event is detected but can't be narrowed down to a
>> > + * specific PE. Iterates through possible failures and handles them as
>> > + * necessary.
>> > + */
>> > static void eeh_handle_special_event(void)
>> > {
>> > struct eeh_pe *pe, *phb_pe;
>>
>> Thanks,
>> Gavin
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [v3, 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event()
2017-04-19 7:39 [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event() Russell Currey
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
2017-04-19 23:49 ` [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event() Gavin Shan
@ 2017-05-03 22:18 ` Michael Ellerman
2 siblings, 0 replies; 10+ messages in thread
From: Michael Ellerman @ 2017-05-03 22:18 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev; +Cc: aik, Russell Currey
On Wed, 2017-04-19 at 07:39:26 UTC, Russell Currey wrote:
> eeh_handle_special_event() is called when an EEH event is detected but
> can't be narrowed down to a specific PE. This function looks through
> every PE to find one in an erroneous state, then calls the regular event
> handler eeh_handle_normal_event() once it knows which PE has an error.
>
> However, if eeh_handle_normal_event() found that the PE cannot possibly
> be recovered, it will free it, rendering the passed PE stale.
> This leads to a use after free in eeh_handle_special_event() as it attempts to
> clear the "recovering" state on the PE after eeh_handle_normal_event() returns.
>
> Thus, make sure the PE is valid when attempting to clear state in
> eeh_handle_special_event().
>
> Cc: <stable@vger.kernel.org> #3.10+
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/daeba2956f32f91f3493788ff6ee02
cheers
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [v3, 2/2] powerpc/eeh: Clean up and document event handling functions
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
` (2 preceding siblings ...)
2017-04-20 1:24 ` Gavin Shan
@ 2017-05-03 22:18 ` Michael Ellerman
3 siblings, 0 replies; 10+ messages in thread
From: Michael Ellerman @ 2017-05-03 22:18 UTC (permalink / raw)
To: Russell Currey, linuxppc-dev; +Cc: aik, Russell Currey
On Wed, 2017-04-19 at 07:39:27 UTC, Russell Currey wrote:
> Remove unnecessary tags in eeh_handle_normal_event(), and add function
> comments for eeh_handle_normal_event() and eeh_handle_special_event().
>
> The only functional difference is that in the case of a PE reaching the
> maximum number of failures, rather than one message telling you of this
> and suggesting you reseat the device, there are two separate messages.
>
> Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/c0b64978f09195e00d6649ca0ad024
cheers
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-03 22:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-19 7:39 [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event() Russell Currey
2017-04-19 7:39 ` [PATCH v3 2/2] powerpc/eeh: Clean up and document event handling functions Russell Currey
2017-04-19 23:48 ` Gavin Shan
2017-04-20 1:03 ` Russell Currey
2017-04-20 1:26 ` Gavin Shan
2017-04-20 0:36 ` Andrew Donnellan
2017-04-20 1:24 ` Gavin Shan
2017-05-03 22:18 ` [v3, " Michael Ellerman
2017-04-19 23:49 ` [PATCH v3 1/2] powerpc/eeh: Avoid use after free in eeh_handle_special_event() Gavin Shan
2017-05-03 22:18 ` [v3, " Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).