[SRU] [OEM-A] [PATCH 0/1] Fix null pointer dereference in AMD MP2 driver

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU] [OEM-A] [PATCH 0/1] Fix null pointer dereference in AMD MP2 driver

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1775152

[Impact]
Touchpad doesn't work after boot. Roughly 1/10 fail rate.

[Test]
With the patch, I no longer see the issue in 30 times reboot.

[Fix]
Merge the "bottom halves" into the interrupt handler to prevent any data
race condition.

[Regression Potential]
Low. It's specific to a single driver, which only gets enabled in OEM-A
kernel.

Kai-Heng Feng (1):
  UBUNTU: SAUCE: i2c: Fix null pointer dereference in AMD MP2 driver

 drivers/i2c/busses/i2c-amd-pci-mp2.c | 53 +++++++++++++---------------
 drivers/i2c/busses/i2c-amd-pci-mp2.h |  2 --
 2 files changed, 24 insertions(+), 31 deletions(-)

--
2.17.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU] [OEM-A] [PATCH 1/1] UBUNTU: SAUCE: i2c: Fix null pointer dereference in AMD MP2 driver

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1775152

The eventval in private data may race in amd_mp2_irq_isr() and
amd_mp2_pci_work(). Squash the latter into the former, so we can make
sure the data is guarded by the lock in the context.

Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/i2c/busses/i2c-amd-pci-mp2.c | 53 +++++++++++++---------------
 drivers/i2c/busses/i2c-amd-pci-mp2.h |  2 --
 2 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.c b/drivers/i2c/busses/i2c-amd-pci-mp2.c
index 2266bf156853..9aa12fddf4d5 100644
--- a/drivers/i2c/busses/i2c-amd-pci-mp2.c
+++ b/drivers/i2c/busses/i2c-amd-pci-mp2.c
@@ -237,14 +237,33 @@ int amd_i2c_register_cb(struct pci_dev *dev, const struct amd_i2c_pci_ops *ops,
 }
 EXPORT_SYMBOL_GPL(amd_i2c_register_cb);
 
-static void amd_mp2_pci_work(struct work_struct *work)
+static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
 {
- struct amd_mp2_dev *privdata = mp2_dev(work);
+ struct amd_mp2_dev *privdata = dev;
+ u32 val = 0;
+ unsigned long flags;
  u32 readdata = 0;
  int i = 0;
- int sts = privdata->eventval.r.status;
- int res = privdata->eventval.r.response;
- int len = privdata->eventval.r.length;
+ int sts;
+ int res;
+ int len;
+
+ raw_spin_lock_irqsave(&privdata->lock, flags);
+ val = readl(privdata->mmio + AMD_P2C_MSG1);
+ if (val != 0) {
+ writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
+ privdata->eventval.ul = val;
+ } else {
+ val = readl(privdata->mmio + AMD_P2C_MSG2);
+ if (val != 0) {
+ writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
+ privdata->eventval.ul = val;
+ }
+ }
+
+ sts = privdata->eventval.r.status;
+ res = privdata->eventval.r.response;
+ len = privdata->eventval.r.length;
 
  if (res == command_success && sts == i2c_readcomplete_event) {
  if (privdata->ops->read_complete) {
@@ -272,26 +291,6 @@ static void amd_mp2_pci_work(struct work_struct *work)
  } else {
  dev_err(ndev_dev(privdata), "ERROR!!nothing to be handled !\n");
  }
-}
-
-static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
-{
- struct amd_mp2_dev *privdata = dev;
- u32 val = 0;
- unsigned long  flags;
-
- raw_spin_lock_irqsave(&privdata->lock, flags);
- val = readl(privdata->mmio + AMD_P2C_MSG1);
- if (val != 0) {
- writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
- privdata->eventval.ul = val;
- } else {
- val = readl(privdata->mmio + AMD_P2C_MSG2);
- if (val != 0) {
- writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
- privdata->eventval.ul = val;
- }
- }
 
  raw_spin_unlock_irqrestore(&privdata->lock, flags);
  if (!privdata->ops)
@@ -301,7 +300,6 @@ static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
  return IRQ_HANDLED;
 
  privdata->requested = false;
- schedule_delayed_work(&privdata->work, 0);
 
  return IRQ_HANDLED;
 }
@@ -536,8 +534,6 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev,
  goto err_pci_init;
  dev_dbg(&pdev->dev, "pci init done.\n");
 
- INIT_DELAYED_WORK(&privdata->work, amd_mp2_pci_work);
-
  amd_mp2_init_debugfs(privdata);
  dev_info(&pdev->dev, "MP2 device registered.\n");
  return 0;
@@ -555,7 +551,6 @@ static void amd_mp2_pci_remove(struct pci_dev *pdev)
 
  amd_mp2_deinit_debugfs(privdata);
  amd_mp2_clear_reg(privdata);
- cancel_delayed_work_sync(&privdata->work);
  free_irq(pdev->irq, privdata);
  pci_intx(pdev, 0);
  amd_mp2_pci_deinit(privdata);
diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.h b/drivers/i2c/busses/i2c-amd-pci-mp2.h
index a84389122885..f9380c494287 100644
--- a/drivers/i2c/busses/i2c-amd-pci-mp2.h
+++ b/drivers/i2c/busses/i2c-amd-pci-mp2.h
@@ -229,7 +229,6 @@ struct amd_mp2_dev {
  struct i2c_write_config write_cfg;
  union i2c_cmd_base i2c_cmd_base;
  const struct amd_i2c_pci_ops *ops;
- struct delayed_work work;
  void *i2c_dev_ctx;
  bool requested;
  raw_spinlock_t lock;
@@ -246,6 +245,5 @@ int amd_i2c_register_cb(struct pci_dev *pdev, const struct amd_i2c_pci_ops *ops,
 #define ndev_pdev(ndev) ((ndev)->pdev)
 #define ndev_name(ndev) pci_name(ndev_pdev(ndev))
 #define ndev_dev(ndev) (&ndev_pdev(ndev)->dev)
-#define mp2_dev(__work) container_of(__work, struct amd_mp2_dev, work.work)
 
 #endif
--
2.17.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [SRU] [OEM-A] [PATCH 1/1] UBUNTU: SAUCE: i2c: Fix null pointer dereference in AMD MP2 driver

Kai-Heng Feng

at 17:52, Kai-Heng Feng <[hidden email]> wrote:

> BugLink: https://bugs.launchpad.net/bugs/1775152
>
> The eventval in private data may race in amd_mp2_irq_isr() and
> amd_mp2_pci_work(). Squash the latter into the former, so we can make
> sure the data is guarded by the lock in the context.

This patch doesn't really solve the root cause. I'll send a new patch that  
really works.

Kai-Heng

>
> Signed-off-by: Kai-Heng Feng <[hidden email]>
> ---
>  drivers/i2c/busses/i2c-amd-pci-mp2.c | 53 +++++++++++++---------------
>  drivers/i2c/busses/i2c-amd-pci-mp2.h |  2 --
>  2 files changed, 24 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.c  
> b/drivers/i2c/busses/i2c-amd-pci-mp2.c
> index 2266bf156853..9aa12fddf4d5 100644
> --- a/drivers/i2c/busses/i2c-amd-pci-mp2.c
> +++ b/drivers/i2c/busses/i2c-amd-pci-mp2.c
> @@ -237,14 +237,33 @@ int amd_i2c_register_cb(struct pci_dev *dev, const  
> struct amd_i2c_pci_ops *ops,
>  }
>  EXPORT_SYMBOL_GPL(amd_i2c_register_cb);
>
> -static void amd_mp2_pci_work(struct work_struct *work)
> +static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
>  {
> - struct amd_mp2_dev *privdata = mp2_dev(work);
> + struct amd_mp2_dev *privdata = dev;
> + u32 val = 0;
> + unsigned long flags;
>   u32 readdata = 0;
>   int i = 0;
> - int sts = privdata->eventval.r.status;
> - int res = privdata->eventval.r.response;
> - int len = privdata->eventval.r.length;
> + int sts;
> + int res;
> + int len;
> +
> + raw_spin_lock_irqsave(&privdata->lock, flags);
> + val = readl(privdata->mmio + AMD_P2C_MSG1);
> + if (val != 0) {
> + writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> + privdata->eventval.ul = val;
> + } else {
> + val = readl(privdata->mmio + AMD_P2C_MSG2);
> + if (val != 0) {
> + writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> + privdata->eventval.ul = val;
> + }
> + }
> +
> + sts = privdata->eventval.r.status;
> + res = privdata->eventval.r.response;
> + len = privdata->eventval.r.length;
>
>   if (res == command_success && sts == i2c_readcomplete_event) {
>   if (privdata->ops->read_complete) {
> @@ -272,26 +291,6 @@ static void amd_mp2_pci_work(struct work_struct *work)
>   } else {
>   dev_err(ndev_dev(privdata), "ERROR!!nothing to be handled !\n");
>   }
> -}
> -
> -static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
> -{
> - struct amd_mp2_dev *privdata = dev;
> - u32 val = 0;
> - unsigned long  flags;
> -
> - raw_spin_lock_irqsave(&privdata->lock, flags);
> - val = readl(privdata->mmio + AMD_P2C_MSG1);
> - if (val != 0) {
> - writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> - privdata->eventval.ul = val;
> - } else {
> - val = readl(privdata->mmio + AMD_P2C_MSG2);
> - if (val != 0) {
> - writel(0, privdata->mmio + AMD_P2C_MSG_INTEN);
> - privdata->eventval.ul = val;
> - }
> - }
>
>   raw_spin_unlock_irqrestore(&privdata->lock, flags);
>   if (!privdata->ops)
> @@ -301,7 +300,6 @@ static irqreturn_t amd_mp2_irq_isr(int irq, void *dev)
>   return IRQ_HANDLED;
>
>   privdata->requested = false;
> - schedule_delayed_work(&privdata->work, 0);
>
>   return IRQ_HANDLED;
>  }
> @@ -536,8 +534,6 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev,
>   goto err_pci_init;
>   dev_dbg(&pdev->dev, "pci init done.\n");
>
> - INIT_DELAYED_WORK(&privdata->work, amd_mp2_pci_work);
> -
>   amd_mp2_init_debugfs(privdata);
>   dev_info(&pdev->dev, "MP2 device registered.\n");
>   return 0;
> @@ -555,7 +551,6 @@ static void amd_mp2_pci_remove(struct pci_dev *pdev)
>
>   amd_mp2_deinit_debugfs(privdata);
>   amd_mp2_clear_reg(privdata);
> - cancel_delayed_work_sync(&privdata->work);
>   free_irq(pdev->irq, privdata);
>   pci_intx(pdev, 0);
>   amd_mp2_pci_deinit(privdata);
> diff --git a/drivers/i2c/busses/i2c-amd-pci-mp2.h  
> b/drivers/i2c/busses/i2c-amd-pci-mp2.h
> index a84389122885..f9380c494287 100644
> --- a/drivers/i2c/busses/i2c-amd-pci-mp2.h
> +++ b/drivers/i2c/busses/i2c-amd-pci-mp2.h
> @@ -229,7 +229,6 @@ struct amd_mp2_dev {
>   struct i2c_write_config write_cfg;
>   union i2c_cmd_base i2c_cmd_base;
>   const struct amd_i2c_pci_ops *ops;
> - struct delayed_work work;
>   void *i2c_dev_ctx;
>   bool requested;
>   raw_spinlock_t lock;
> @@ -246,6 +245,5 @@ int amd_i2c_register_cb(struct pci_dev *pdev, const  
> struct amd_i2c_pci_ops *ops,
>  #define ndev_pdev(ndev) ((ndev)->pdev)
>  #define ndev_name(ndev) pci_name(ndev_pdev(ndev))
>  #define ndev_dev(ndev) (&ndev_pdev(ndev)->dev)
> -#define mp2_dev(__work) container_of(__work, struct amd_mp2_dev,  
> work.work)
>
>  #endif
> --
> 2.17.0
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team