Fix a crash with multipath activated. It happends when ANA log
page is larger than MDTS and because of that ANA is disabled.
The driver then tries to access unallocated buffer when connecting
to a nvme target. The signature is as follows:
[ 300.433586] nvme nvme0: ANA log page size (8208) larger than MDTS (8192).
[ 300.435387] nvme nvme0: disabling ANA support.
[ 300.437835] nvme nvme0: creating 4 I/O queues.
[ 300.459132] nvme nvme0: new ctrl: NQN "nqn.0.0.0", addr 10.91.0.1:8009
[ 300.464609] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
[ 300.466342] #PF error: [normal kernel read fault]
[ 300.467385] PGD 0 P4D 0
[ 300.467987] Oops: 0000 [#1] SMP PTI
[ 300.468787] CPU: 3 PID: 50 Comm: kworker/u8:1 Not tainted 5.0.20kalray+ #4
[ 300.470264] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
[ 300.471532] Workqueue: nvme-wq nvme_scan_work [nvme_core]
[ 300.472724] RIP: 0010:nvme_parse_ana_log+0x21/0x140 [nvme_core]
[ 300.474038] Code: 45 01 d2 d8 48 98 c3 66 90 0f 1f 44 00 00 41 57 41 56 41 55 41 54 55 53 48 89 fb 48 83 ec 08 48 8b af 20 0a 00 00 48 89 34 24 <66> 83 7d 08 00 0f 84 c6 00 00 00 44 8b 7d 14 49 89 d5 8b 55 10 48
[ 300.477374] RSP: 0018:ffffa50e80fd7cb8 EFLAGS: 00010296
[ 300.478334] RAX: 0000000000000001 RBX: ffff9130f1872258 RCX: 0000000000000000
[ 300.479784] RDX: ffffffffc06c4c30 RSI: ffff9130edad4280 RDI: ffff9130f1872258
[ 300.481488] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000044
[ 300.483203] R10: 0000000000000220 R11: 0000000000000040 R12: ffff9130f18722c0
[ 300.484928] R13: ffff9130f18722d0 R14: ffff9130edad4280 R15: ffff9130f18722c0
[ 300.486626] FS: 0000000000000000(0000) GS:ffff9130f7b80000(0000) knlGS:0000000000000000
[ 300.488538] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 300.489907] CR2: 0000000000000008 CR3: 00000002365e6000 CR4: 00000000000006e0
[ 300.491612] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 300.493303] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 300.494991] Call Trace:
[ 300.495645] nvme_mpath_add_disk+0x5c/0xb0 [nvme_core]
[ 300.496880] nvme_validate_ns+0x2ef/0x550 [nvme_core]
[ 300.498105] ? nvme_identify_ctrl.isra.45+0x6a/0xb0 [nvme_core]
[ 300.499539] nvme_scan_work+0x2b4/0x370 [nvme_core]
[ 300.500717] ? __switch_to_asm+0x35/0x70
[ 300.501663] process_one_work+0x171/0x380
[ 300.502340] worker_thread+0x49/0x3f0
[ 300.503079] kthread+0xf8/0x130
[ 300.503795] ? max_active_store+0x80/0x80
[ 300.504690] ? kthread_bind+0x10/0x10
[ 300.505502] ret_from_fork+0x35/0x40
[ 300.506280] Modules linked in: nvme_tcp nvme_rdma rdma_cm iw_cm ib_cm ib_core nvme_fabrics nvme_core xt_physdev ip6table_raw ip6table_mangle ip6table_filter ip6_tables xt_comment iptable_nat nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 xt_CHECKSUM iptable_mangle iptable_filter veth ebtable_filter ebtable_nat ebtables iptable_raw vxlan ip6_udp_tunnel udp_tunnel sunrpc joydev pcspkr virtio_balloon br_netfilter bridge stp llc ip_tables xfs libcrc32c ata_generic pata_acpi virtio_net virtio_console net_failover virtio_blk failover ata_piix serio_raw libata virtio_pci virtio_ring virtio
[ 300.514984] CR2: 0000000000000008
[ 300.515569] ---[ end trace faa2eefad7e7f218 ]---
[ 300.516354] RIP: 0010:nvme_parse_ana_log+0x21/0x140 [nvme_core]
[ 300.517330] Code: 45 01 d2 d8 48 98 c3 66 90 0f 1f 44 00 00 41 57 41 56 41 55 41 54 55 53 48 89 fb 48 83 ec 08 48 8b af 20 0a 00 00 48 89 34 24 <66> 83 7d 08 00 0f 84 c6 00 00 00 44 8b 7d 14 49 89 d5 8b 55 10 48
[ 300.520353] RSP: 0018:ffffa50e80fd7cb8 EFLAGS: 00010296
[ 300.521229] RAX: 0000000000000001 RBX: ffff9130f1872258 RCX: 0000000000000000
[ 300.522399] RDX: ffffffffc06c4c30 RSI: ffff9130edad4280 RDI: ffff9130f1872258
[ 300.523560] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000044
[ 300.524734] R10: 0000000000000220 R11: 0000000000000040 R12: ffff9130f18722c0
[ 300.525915] R13: ffff9130f18722d0 R14: ffff9130edad4280 R15: ffff9130f18722c0
[ 300.527084] FS: 0000000000000000(0000) GS:ffff9130f7b80000(0000) knlGS:0000000000000000
[ 300.528396] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 300.529440] CR2: 0000000000000008 CR3: 00000002365e6000 CR4: 00000000000006e0
[ 300.530739] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 300.531989] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 300.533264] Kernel panic - not syncing: Fatal exception
[ 300.534338] Kernel Offset: 0x17c00000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[ 300.536227] ---[ end Kernel panic - not syncing: Fatal exception ]---
Signed-off-by: Marta Rybczynska <[email protected]>
Tested-by: Jean-Baptiste Riaux <[email protected]>
---
drivers/nvme/host/multipath.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 499acf0..5ba982b 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -14,7 +14,8 @@
inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
{
- return multipath && ctrl->subsys && (ctrl->subsys->cmic & (1 << 3));
+ return multipath && ctrl->ana_log_buf && ctrl->subsys &&
+ (ctrl->subsys->cmic & (1 << 3));
}
/*
@@ -614,7 +615,10 @@ int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
{
int error;
- if (!nvme_ctrl_use_ana(ctrl))
+ /* check if multipath is enabled and we have the capability */
+ if (!multipath)
+ return 0;
+ if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) != 0))
return 0;
ctrl->anacap = id->anacap;
--
1.8.3.1
On 7/5/2019 5:05 PM, Marta Rybczynska wrote:
> Fix a crash with multipath activated. It happends when ANA log
> page is larger than MDTS and because of that ANA is disabled.
> The driver then tries to access unallocated buffer when connecting
> to a nvme target. The signature is as follows:
>
> [ 300.433586] nvme nvme0: ANA log page size (8208) larger than MDTS (8192).
> [ 300.435387] nvme nvme0: disabling ANA support.
> [ 300.437835] nvme nvme0: creating 4 I/O queues.
> [ 300.459132] nvme nvme0: new ctrl: NQN "nqn.0.0.0", addr 10.91.0.1:8009
> [ 300.464609] BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> [ 300.466342] #PF error: [normal kernel read fault]
> [ 300.467385] PGD 0 P4D 0
> [ 300.467987] Oops: 0000 [#1] SMP PTI
> [ 300.468787] CPU: 3 PID: 50 Comm: kworker/u8:1 Not tainted 5.0.20kalray+ #4
> [ 300.470264] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> [ 300.471532] Workqueue: nvme-wq nvme_scan_work [nvme_core]
> [ 300.472724] RIP: 0010:nvme_parse_ana_log+0x21/0x140 [nvme_core]
> [ 300.474038] Code: 45 01 d2 d8 48 98 c3 66 90 0f 1f 44 00 00 41 57 41 56 41 55 41 54 55 53 48 89 fb 48 83 ec 08 48 8b af 20 0a 00 00 48 89 34 24 <66> 83 7d 08 00 0f 84 c6 00 00 00 44 8b 7d 14 49 89 d5 8b 55 10 48
> [ 300.477374] RSP: 0018:ffffa50e80fd7cb8 EFLAGS: 00010296
> [ 300.478334] RAX: 0000000000000001 RBX: ffff9130f1872258 RCX: 0000000000000000
> [ 300.479784] RDX: ffffffffc06c4c30 RSI: ffff9130edad4280 RDI: ffff9130f1872258
> [ 300.481488] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000044
> [ 300.483203] R10: 0000000000000220 R11: 0000000000000040 R12: ffff9130f18722c0
> [ 300.484928] R13: ffff9130f18722d0 R14: ffff9130edad4280 R15: ffff9130f18722c0
> [ 300.486626] FS: 0000000000000000(0000) GS:ffff9130f7b80000(0000) knlGS:0000000000000000
> [ 300.488538] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 300.489907] CR2: 0000000000000008 CR3: 00000002365e6000 CR4: 00000000000006e0
> [ 300.491612] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 300.493303] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 300.494991] Call Trace:
> [ 300.495645] nvme_mpath_add_disk+0x5c/0xb0 [nvme_core]
> [ 300.496880] nvme_validate_ns+0x2ef/0x550 [nvme_core]
> [ 300.498105] ? nvme_identify_ctrl.isra.45+0x6a/0xb0 [nvme_core]
> [ 300.499539] nvme_scan_work+0x2b4/0x370 [nvme_core]
> [ 300.500717] ? __switch_to_asm+0x35/0x70
> [ 300.501663] process_one_work+0x171/0x380
> [ 300.502340] worker_thread+0x49/0x3f0
> [ 300.503079] kthread+0xf8/0x130
> [ 300.503795] ? max_active_store+0x80/0x80
> [ 300.504690] ? kthread_bind+0x10/0x10
> [ 300.505502] ret_from_fork+0x35/0x40
> [ 300.506280] Modules linked in: nvme_tcp nvme_rdma rdma_cm iw_cm ib_cm ib_core nvme_fabrics nvme_core xt_physdev ip6table_raw ip6table_mangle ip6table_filter ip6_tables xt_comment iptable_nat nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 xt_CHECKSUM iptable_mangle iptable_filter veth ebtable_filter ebtable_nat ebtables iptable_raw vxlan ip6_udp_tunnel udp_tunnel sunrpc joydev pcspkr virtio_balloon br_netfilter bridge stp llc ip_tables xfs libcrc32c ata_generic pata_acpi virtio_net virtio_console net_failover virtio_blk failover ata_piix serio_raw libata virtio_pci virtio_ring virtio
> [ 300.514984] CR2: 0000000000000008
> [ 300.515569] ---[ end trace faa2eefad7e7f218 ]---
> [ 300.516354] RIP: 0010:nvme_parse_ana_log+0x21/0x140 [nvme_core]
> [ 300.517330] Code: 45 01 d2 d8 48 98 c3 66 90 0f 1f 44 00 00 41 57 41 56 41 55 41 54 55 53 48 89 fb 48 83 ec 08 48 8b af 20 0a 00 00 48 89 34 24 <66> 83 7d 08 00 0f 84 c6 00 00 00 44 8b 7d 14 49 89 d5 8b 55 10 48
> [ 300.520353] RSP: 0018:ffffa50e80fd7cb8 EFLAGS: 00010296
> [ 300.521229] RAX: 0000000000000001 RBX: ffff9130f1872258 RCX: 0000000000000000
> [ 300.522399] RDX: ffffffffc06c4c30 RSI: ffff9130edad4280 RDI: ffff9130f1872258
> [ 300.523560] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000044
> [ 300.524734] R10: 0000000000000220 R11: 0000000000000040 R12: ffff9130f18722c0
> [ 300.525915] R13: ffff9130f18722d0 R14: ffff9130edad4280 R15: ffff9130f18722c0
> [ 300.527084] FS: 0000000000000000(0000) GS:ffff9130f7b80000(0000) knlGS:0000000000000000
> [ 300.528396] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 300.529440] CR2: 0000000000000008 CR3: 00000002365e6000 CR4: 00000000000006e0
> [ 300.530739] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 300.531989] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 300.533264] Kernel panic - not syncing: Fatal exception
> [ 300.534338] Kernel Offset: 0x17c00000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
> [ 300.536227] ---[ end Kernel panic - not syncing: Fatal exception ]---
>
> Signed-off-by: Marta Rybczynska <[email protected]>
> Tested-by: Jean-Baptiste Riaux <[email protected]>
> ---
> drivers/nvme/host/multipath.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 499acf0..5ba982b 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -14,7 +14,8 @@
>
> inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
> {
> - return multipath && ctrl->subsys && (ctrl->subsys->cmic & (1 << 3));
> + return multipath && ctrl->ana_log_buf && ctrl->subsys &&
> + (ctrl->subsys->cmic & (1 << 3));
> }
>
> /*
> @@ -614,7 +615,10 @@ int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> {
> int error;
>
> - if (!nvme_ctrl_use_ana(ctrl))
> + /* check if multipath is enabled and we have the capability */
> + if (!multipath)
> + return 0;
> + if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) != 0))
shouldn't it be:
if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) == 0))
or
if (!ctrl->subsys || !(ctrl->subsys->cmic & (1 << 3)))
Otherwise, you don't really do any initialization and return 0 in case you have the capability, right ?
> return 0;
>
> ctrl->anacap = id->anacap;
On Sat, Jul 06, 2019 at 01:06:44PM +0300, Max Gurtovoy wrote:
>> + /* check if multipath is enabled and we have the capability */
>> + if (!multipath)
>> + return 0;
>> + if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) != 0))
>
> shouldn't it be:
>
> if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) == 0))
>
> or
>
> if (!ctrl->subsys || !(ctrl->subsys->cmic & (1 << 3)))
>
>
> Otherwise, you don't really do any initialization and return 0 in case you have the capability, right ?
Yes. FYI, my idea how to fix this would be something like:
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index a9a927677970..cdb3e5baa329 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -12,11 +12,6 @@ module_param(multipath, bool, 0444);
MODULE_PARM_DESC(multipath,
"turn on native support for multiple controllers per subsystem");
-inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
-{
- return multipath && ctrl->subsys && (ctrl->subsys->cmic & (1 << 3));
-}
-
/*
* If multipathing is enabled we need to always use the subsystem instance
* number for numbering our devices to avoid conflicts between subsystems that
@@ -622,7 +617,7 @@ int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
{
int error;
- if (!nvme_ctrl_use_ana(ctrl))
+ if (!multipath || !ctrl->subsys || !(ctrl->subsys->cmic & (1 << 3)))
return 0;
ctrl->anacap = id->anacap;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 716a876119c8..14eca76bec5c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -485,7 +485,10 @@ extern const struct attribute_group *nvme_ns_id_attr_groups[];
extern const struct block_device_operations nvme_ns_head_ops;
#ifdef CONFIG_NVME_MULTIPATH
-bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl);
+static inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
+{
+ return ctrl->ana_log_buf != NULL;
+}
void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
struct nvme_ctrl *ctrl, int *flags);
void nvme_failover_req(struct request *req);
----- On 9 Jul, 2019, at 23:29, Christoph Hellwig [email protected] wrote:
> On Sat, Jul 06, 2019 at 01:06:44PM +0300, Max Gurtovoy wrote:
>>> + /* check if multipath is enabled and we have the capability */
>>> + if (!multipath)
>>> + return 0;
>>> + if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) != 0))
>>
>> shouldn't it be:
>>
>> if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) == 0))
>>
>> or
>>
>> if (!ctrl->subsys || !(ctrl->subsys->cmic & (1 << 3)))
>>
>>
>> Otherwise, you don't really do any initialization and return 0 in case you have
>> the capability, right ?
>
> Yes. FYI, my idea how to fix this would be something like:
Thanks both, error when changing the condition on my side. I submit the next
version very soon.
Christoph, why would you like to put the use_ana function in the header?
It isn't used anywhere else outside of that file.
Regards,
Marta
On 7/10/2019 12:29 AM, Christoph Hellwig wrote:
> On Sat, Jul 06, 2019 at 01:06:44PM +0300, Max Gurtovoy wrote:
>>> + /* check if multipath is enabled and we have the capability */
>>> + if (!multipath)
>>> + return 0;
>>> + if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) != 0))
>> shouldn't it be:
>>
>> if (!ctrl->subsys || ((ctrl->subsys->cmic & (1 << 3)) == 0))
>>
>> or
>>
>> if (!ctrl->subsys || !(ctrl->subsys->cmic & (1 << 3)))
>>
>>
>> Otherwise, you don't really do any initialization and return 0 in case you have the capability, right ?
> Yes. FYI, my idea how to fix this would be something like:
>
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index a9a927677970..cdb3e5baa329 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -12,11 +12,6 @@ module_param(multipath, bool, 0444);
> MODULE_PARM_DESC(multipath,
> "turn on native support for multiple controllers per subsystem");
>
> -inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
> -{
> - return multipath && ctrl->subsys && (ctrl->subsys->cmic & (1 << 3));
> -}
> -
> /*
> * If multipathing is enabled we need to always use the subsystem instance
> * number for numbering our devices to avoid conflicts between subsystems that
> @@ -622,7 +617,7 @@ int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> {
> int error;
>
> - if (!nvme_ctrl_use_ana(ctrl))
> + if (!multipath || !ctrl->subsys || !(ctrl->subsys->cmic & (1 << 3)))
> return 0;
>
> ctrl->anacap = id->anacap;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 716a876119c8..14eca76bec5c 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -485,7 +485,10 @@ extern const struct attribute_group *nvme_ns_id_attr_groups[];
> extern const struct block_device_operations nvme_ns_head_ops;
>
> #ifdef CONFIG_NVME_MULTIPATH
> -bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl);
> +static inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
> +{
> + return ctrl->ana_log_buf != NULL;
> +}
> void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
> struct nvme_ctrl *ctrl, int *flags);
> void nvme_failover_req(struct request *req);
Yes this looks good.
On Wed, Jul 10, 2019 at 07:26:46AM +0200, Marta Rybczynska wrote:
> Christoph, why would you like to put the use_ana function in the header?
> It isn't used anywhere else outside of that file.
nvme_ctrl_use_ana has a single caller in core.c as well.
----- On 10 Jul, 2019, at 18:38, Christoph Hellwig [email protected] wrote:
> On Wed, Jul 10, 2019 at 07:26:46AM +0200, Marta Rybczynska wrote:
>> Christoph, why would you like to put the use_ana function in the header?
>> It isn't used anywhere else outside of that file.
>
> nvme_ctrl_use_ana has a single caller in core.c as well.
Thanks Christoph, I'm testing a version similar to yours.
Marta