Re: [Regression] [PATCH] iommu: Avoid crash in init_no_remapping_devices if iommu is NULL

2020-09-01 Thread Lu Baolu 

Hi Torsten,

On 9/1/20 10:41 PM, Torsten Hilbrich wrote:

On 01.09.20 04:02, Lu Baolu wrote:
[...]

This looks more like a generic issue, used-before-allocated, and should
be fixed in iommu.c instead of VT-d driver. How about

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8fd93a5b8764..a599da87eb60 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -190,6 +190,28 @@ static void dev_iommu_free(struct device *dev)
     dev->iommu = NULL;
  }

+void *dev_iommu_priv_get(struct device *dev)
+{
+   struct dev_iommu *param = dev_iommu_get(dev);
+
+   if (WARN_ON(!param))
+   return ERR_PTR(-ENOMEM);
+
+    return param->priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_get);
+
+void dev_iommu_priv_set(struct device *dev, void *priv)
+{
+   struct dev_iommu *param = dev_iommu_get(dev);
+
+   if (WARN_ON(!param))
+   return;
+
+    param->priv = priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
+


This fix would work in my case. I tested it with slight modification to
replace the inline routines in include/linux/iommu.h.

The WARN_ON was not triggered during my tests. However, looking at the
definition of dev_iommu_get this is to be expected.


So I suppose you will post a new version. :-)

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Regression] [PATCH] iommu: Avoid crash in init_no_remapping_devices if iommu is NULL

2020-09-01 Thread Lu Baolu 

hi Torsten,

On 9/1/20 10:41 PM, Torsten Hilbrich wrote:

On 01.09.20 04:02, Lu Baolu wrote:
[...]

This looks more like a generic issue, used-before-allocated, and should
be fixed in iommu.c instead of VT-d driver. How about

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8fd93a5b8764..a599da87eb60 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -190,6 +190,28 @@ static void dev_iommu_free(struct device *dev)
     dev->iommu = NULL;
  }

+void *dev_iommu_priv_get(struct device *dev)
+{
+   struct dev_iommu *param = dev_iommu_get(dev);
+
+   if (WARN_ON(!param))
+   return ERR_PTR(-ENOMEM);
+
+    return param->priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_get);
+
+void dev_iommu_priv_set(struct device *dev, void *priv)
+{
+   struct dev_iommu *param = dev_iommu_get(dev);
+
+   if (WARN_ON(!param))
+   return;
+
+    param->priv = priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
+


This fix would work in my case. I tested it with slight modification to
replace the inline routines in include/linux/iommu.h.

The WARN_ON was not triggered during my tests. However, looking at the
definition of dev_iommu_get this is to be expected.


So I suppose you will post a new version. :-)

Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Regression] [PATCH] iommu: Avoid crash in init_no_remapping_devices if iommu is NULL

2020-09-01 Thread Torsten Hilbrich 
On 01.09.20 04:02, Lu Baolu wrote:
[...]
> This looks more like a generic issue, used-before-allocated, and should
> be fixed in iommu.c instead of VT-d driver. How about
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8fd93a5b8764..a599da87eb60 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -190,6 +190,28 @@ static void dev_iommu_free(struct device *dev)
>     dev->iommu = NULL;
>  }
> 
> +void *dev_iommu_priv_get(struct device *dev)
> +{
> +   struct dev_iommu *param = dev_iommu_get(dev);
> +
> +   if (WARN_ON(!param))
> +   return ERR_PTR(-ENOMEM);
> +
> +    return param->priv;
> +}
> +EXPORT_SYMBOL_GPL(dev_iommu_priv_get);
> +
> +void dev_iommu_priv_set(struct device *dev, void *priv)
> +{
> +   struct dev_iommu *param = dev_iommu_get(dev);
> +
> +   if (WARN_ON(!param))
> +   return;
> +
> +    param->priv = priv;
> +}
> +EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
> +

This fix would work in my case. I tested it with slight modification to
replace the inline routines in include/linux/iommu.h.

The WARN_ON was not triggered during my tests. However, looking at the
definition of dev_iommu_get this is to be expected.

Regards,

Torsten
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Regression] [PATCH] iommu: Avoid crash in init_no_remapping_devices if iommu is NULL

2020-08-31 Thread Lu Baolu 

Hi Torsten,

Thank you for reporting this.

On 8/31/20 7:03 PM, Torsten Hilbrich wrote:

I noticed a kernel crash when trying to boot a v5.9-rc2 based kernel.

The crash reads as:

<1>[7.410192] BUG: kernel NULL pointer dereference, address: 
0038
<1>[7.410196] #PF: supervisor write access in kernel mode
<1>[7.410199] #PF: error_code(0x0002) - not-present page
<6>[7.410202] PGD 0 P4D 0
<4>[7.410207] Oops: 0002 [#1] SMP PTI
<4>[7.410212] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.9.0-devel+ #1
<4>[7.410215] Hardware name: LENOVO 20HGS0TW00/20HGS0TW00, BIOS N1WET46S 
(1.25s ) 03/30/2018
<4>[7.410224] RIP: 0010:intel_iommu_init+0xed0/0x1136
<4>[7.410229] Code: fe e9 61 02 00 00 bb f4 ff ff ff e9 57 02 00 00 48 63 d1 48 
c1 e2 04 48 03 50 20 48 8b 12 48 85 d2 74 0b 48 8b 92 d0 02 00 00 <48> 89 7a 38 ff c1 
e9 15 f5 ff ff 48 c7 c7 00 a5 ac a1 49 c7 c7 20
<4>[7.410236] RSP: :b14e40073dd0 EFLAGS: 00010282
<4>[7.410240] RAX: 8d0643731720 RBX:  RCX: 

<4>[7.410244] RDX:  RSI:  RDI: 

<4>[7.410247] RBP: b14e40073e90 R08: 0001 R09: 
8d0643803700
<4>[7.410250] R10: 8d064262 R11: 0016 R12: 
000b
<4>[7.410254] R13: 8d064361e650 R14: a2455d80 R15: 

<4>[7.410258] FS:  () GS:8d064748() 
knlGS:
<4>[7.410262] CS:  0010 DS:  ES:  CR0: 80050033
<4>[7.410266] CR2: 0038 CR3: 00056760a001 CR4: 
003706e0
<4>[7.410269] Call Trace:
<4>[7.410280]  ? call_rcu+0x10e/0x320
<4>[7.410286]  ? trace_hardirqs_on+0x2c/0xd0
<4>[7.410291]  ? rdinit_setup+0x2c/0x2c
<4>[7.410297]  ? e820__memblock_setup+0x8b/0x8b
<4>[7.410302]  pci_iommu_init+0x16/0x3f
<4>[7.410307]  do_one_initcall+0x46/0x1e4
<4>[7.410313]  kernel_init_freeable+0x169/0x1b2
<4>[7.410320]  ? rest_init+0x9f/0x9f
<4>[7.410325]  kernel_init+0xa/0x101
<4>[7.410329]  ret_from_fork+0x22/0x30
<4>[7.410333] Modules linked in:
<4>[7.410338] CR2: 0038
<4>[7.410344] ---[ end trace 16bcdb1e11668fcd ]---

Full kernel message is attached in kernel.log.

I tracked the problem down to the dev_iommu_priv_set call in 
init_no_remapping_devices. It seems that for one device the dev->iommu member 
is NULL.

An dev_err outputs the device as "pci :00:02.0: DMAR" which is the intel HD 
620 graphic adapter in a Lenovo T470s device.

The following patch (also attached as intel-iommu.patch) avoids this crash by 
checking the pointer.

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f8177c59d229..2d285a42db3f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4053,7 +4053,8 @@ static void __init init_no_remapping_devices(void)
 drhd->ignored = 1;
 for_each_active_dev_scope(drhd->devices,
   drhd->devices_cnt, i, dev)
-   dev_iommu_priv_set(dev, 
DUMMY_DEVICE_DOMAIN_INFO);
+   if (dev->iommu)
+   dev_iommu_priv_set(dev, 
DUMMY_DEVICE_DOMAIN_INFO);


This looks more like a generic issue, used-before-allocated, and should
be fixed in iommu.c instead of VT-d driver. How about

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8fd93a5b8764..a599da87eb60 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -190,6 +190,28 @@ static void dev_iommu_free(struct device *dev)
dev->iommu = NULL;
 }

+void *dev_iommu_priv_get(struct device *dev)
+{
+   struct dev_iommu *param = dev_iommu_get(dev);
+
+   if (WARN_ON(!param))
+   return ERR_PTR(-ENOMEM);
+
+return param->priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_get);
+
+void dev_iommu_priv_set(struct device *dev, void *priv)
+{
+   struct dev_iommu *param = dev_iommu_get(dev);
+
+   if (WARN_ON(!param))
+   return;
+
+param->priv = priv;
+}
+EXPORT_SYMBOL_GPL(dev_iommu_priv_set);
+

Best regards,
baolu


 }
 }
  }

I assume the problem might be related to the following commit introduced in 
v5.9-rc1:

commit 01b9d4e21148c89fdbab3d6b3705f9791314b631
Author: Joerg Roedel 
Date:   Thu Jun 25 15:08:25 2020 +0200

 iommu/vt-d: Use dev_iommu_priv_get/set()
 
 Remove the use of dev->archdata.iommu and use the private per-device

 pointer provided by IOMMU core code instead.

With regards,

Torsten Hilbrich


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


___
iommu mailing list
iommu@lists.linux-foundation.org

[Regression] [PATCH] iommu: Avoid crash in init_no_remapping_devices if iommu is NULL

2020-08-31 Thread Torsten Hilbrich 
I noticed a kernel crash when trying to boot a v5.9-rc2 based kernel.

The crash reads as:

<1>[7.410192] BUG: kernel NULL pointer dereference, address: 
0038
<1>[7.410196] #PF: supervisor write access in kernel mode
<1>[7.410199] #PF: error_code(0x0002) - not-present page
<6>[7.410202] PGD 0 P4D 0 
<4>[7.410207] Oops: 0002 [#1] SMP PTI
<4>[7.410212] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.9.0-devel+ #1
<4>[7.410215] Hardware name: LENOVO 20HGS0TW00/20HGS0TW00, BIOS N1WET46S 
(1.25s ) 03/30/2018
<4>[7.410224] RIP: 0010:intel_iommu_init+0xed0/0x1136
<4>[7.410229] Code: fe e9 61 02 00 00 bb f4 ff ff ff e9 57 02 00 00 48 63 
d1 48 c1 e2 04 48 03 50 20 48 8b 12 48 85 d2 74 0b 48 8b 92 d0 02 00 00 <48> 89 
7a 38 ff c1 e9 15 f5 ff ff 48 c7 c7 00 a5 ac a1 49 c7 c7 20
<4>[7.410236] RSP: :b14e40073dd0 EFLAGS: 00010282
<4>[7.410240] RAX: 8d0643731720 RBX:  RCX: 

<4>[7.410244] RDX:  RSI:  RDI: 

<4>[7.410247] RBP: b14e40073e90 R08: 0001 R09: 
8d0643803700
<4>[7.410250] R10: 8d064262 R11: 0016 R12: 
000b
<4>[7.410254] R13: 8d064361e650 R14: a2455d80 R15: 

<4>[7.410258] FS:  () GS:8d064748() 
knlGS:
<4>[7.410262] CS:  0010 DS:  ES:  CR0: 80050033
<4>[7.410266] CR2: 0038 CR3: 00056760a001 CR4: 
003706e0
<4>[7.410269] Call Trace:
<4>[7.410280]  ? call_rcu+0x10e/0x320
<4>[7.410286]  ? trace_hardirqs_on+0x2c/0xd0
<4>[7.410291]  ? rdinit_setup+0x2c/0x2c
<4>[7.410297]  ? e820__memblock_setup+0x8b/0x8b
<4>[7.410302]  pci_iommu_init+0x16/0x3f
<4>[7.410307]  do_one_initcall+0x46/0x1e4
<4>[7.410313]  kernel_init_freeable+0x169/0x1b2
<4>[7.410320]  ? rest_init+0x9f/0x9f
<4>[7.410325]  kernel_init+0xa/0x101
<4>[7.410329]  ret_from_fork+0x22/0x30
<4>[7.410333] Modules linked in:
<4>[7.410338] CR2: 0038
<4>[7.410344] ---[ end trace 16bcdb1e11668fcd ]---

Full kernel message is attached in kernel.log.

I tracked the problem down to the dev_iommu_priv_set call in 
init_no_remapping_devices. It seems that for one device the dev->iommu member 
is NULL.

An dev_err outputs the device as "pci :00:02.0: DMAR" which is the intel HD 
620 graphic adapter in a Lenovo T470s device.

The following patch (also attached as intel-iommu.patch) avoids this crash by 
checking the pointer.

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f8177c59d229..2d285a42db3f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4053,7 +4053,8 @@ static void __init init_no_remapping_devices(void)
drhd->ignored = 1;
for_each_active_dev_scope(drhd->devices,
  drhd->devices_cnt, i, dev)
-   dev_iommu_priv_set(dev, 
DUMMY_DEVICE_DOMAIN_INFO);
+   if (dev->iommu)
+   dev_iommu_priv_set(dev, 
DUMMY_DEVICE_DOMAIN_INFO);
}
}
 }

I assume the problem might be related to the following commit introduced in 
v5.9-rc1:

commit 01b9d4e21148c89fdbab3d6b3705f9791314b631
Author: Joerg Roedel 
Date:   Thu Jun 25 15:08:25 2020 +0200

iommu/vt-d: Use dev_iommu_priv_get/set()

Remove the use of dev->archdata.iommu and use the private per-device
pointer provided by IOMMU core code instead.

With regards,

Torsten Hilbrich

-- 
Dipl.-Inform.
Torsten Hilbrich
Teamleiter Bereich Entwicklung
Abteilung Network & Client Security
Divison Public Authorities
secunet Security Networks AG 
Konrad-Zuse-Platz 2-4
81829 München

Tel: +49-201-5454-3522
Fax: +49-201-5454-1327
torsten.hilbr...@secunet.com
www.secunet.com

__

secunet Security Networks AG
Sitz: Kurfürstenstraße 58, 45138 Essen, Deutschland 
Amtsgericht Essen HRB 13615
Vorstand: Axel Deininger (Vors.), Torsten Henn, Dr. Kai Martius, Thomas
  Pleines
Aufsichtsratsvorsitzender: Ralf Wintergerst
__

Oops#1 Part1
<6>[0.00] microcode: microcode updated early to revision 0xd6, date = 2020-04-27
<5>[0.00] Linux version 5.9.0-devel+ (thilbrich@ts-6) (gcc (Debian 8.3.0-6) 8.3.0, GNU ld (GNU Binutils for Debian) 2.31.1) #2 SMP Mon Aug 31 09:20:36 CEST 2020
<6>[0.00] Command line: BOOT_IMAGE=/isolinux/bzImage console=tty1 intel_iommu=on,igfx_off loglevel=7 initcall_debug
<6>[0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers'
<6>[0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers'
<6>[0.00] x86/fpu: Supporting XSAVE feature