Re: [PATCH v2] Use ctlr directly in rdac_failover_get()

[Martin K. Petersen]

Artem,

> rdac_failover_get references struct rdac_controller as
> ctlr->ms_sdev->handler_data->ctlr for no apparent reason. Besides being
> inefficient this also introduces a null-pointer dereference as
> send_mode_select() sets ctlr->ms_sdev to NULL before calling
> rdac_failover_get():

Applied to 4.12/scsi-fixes. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2] Use ctlr directly in rdac_failover_get()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig 

[PATCH v2] Use ctlr directly in rdac_failover_get()

[Artem Savkov]

rdac_failover_get references struct rdac_controller as
ctlr->ms_sdev->handler_data->ctlr for no apparent reason. Besides being
inefficient this also introduces a null-pointer dereference as
send_mode_select() sets ctlr->ms_sdev to NULL before calling
rdac_failover_get():

[   18.432550] device-mapper: multipath service-time: version 0.3.0 loaded
[   18.436124] BUG: unable to handle kernel NULL pointer dereference at 
0790
[   18.436129] IP: send_mode_select+0xca/0x560
[   18.436129] PGD 0
[   18.436130] P4D 0
[   18.436130]
[   18.436132] Oops:  [#1] SMP
[   18.436133] Modules linked in: dm_service_time sd_mod dm_multipath amdkfd 
amd_iommu_v2 radeon(+) i2c_algo_bit drm_kms_helper syscopyarea sysfillrect 
sysimgblt fb_sys_fops ttm qla2xxx drm serio_raw scsi_transport_fc bnx2 i2c_core 
dm_mirror dm_region_hash dm_log dm_mod
[   18.436143] CPU: 4 PID: 443 Comm: kworker/u16:2 Not tainted 
4.12.0-rc1.1.el7.test.x86_64 #1
[   18.436144] Hardware name: IBM BladeCenter LS22 -[79013SG]-/Server Blade, 
BIOS -[L8E164AUS-1.07]- 05/25/2011
[   18.436145] Workqueue: kmpath_rdacd send_mode_select
[   18.436146] task: 880225116a40 task.stack: c90002bd8000
[   18.436148] RIP: 0010:send_mode_select+0xca/0x560
[   18.436148] RSP: 0018:c90002bdbda8 EFLAGS: 00010246
[   18.436149] RAX:  RBX: c90002bdbe08 RCX: 88017ef04a80
[   18.436150] RDX: c90002bdbe08 RSI: 88017ef04a80 RDI: 8802248e4388
[   18.436151] RBP: c90002bdbe48 R08:  R09: 81c104c0
[   18.436151] R10: 01ff R11: 035a R12: c90002bdbdd8
[   18.436152] R13: 8802248e4390 R14: 880225152800 R15: 8802248e4400
[   18.436153] FS:  () GS:880227d0() 
knlGS:
[   18.436154] CS:  0010 DS:  ES:  CR0: 80050033
[   18.436154] CR2: 0790 CR3: 00042535b000 CR4: 06e0
[   18.436155] Call Trace:
[   18.436159]  ? rdac_activate+0x14e/0x150
[   18.436161]  ? refcount_dec_and_test+0x11/0x20
[   18.436162]  ? kobject_put+0x1c/0x50
[   18.436165]  ? scsi_dh_activate+0x6f/0xd0
[   18.436168]  process_one_work+0x149/0x360
[   18.436170]  worker_thread+0x4d/0x3c0
[   18.436172]  kthread+0x109/0x140
[   18.436173]  ? rescuer_thread+0x380/0x380
[   18.436174]  ? kthread_park+0x60/0x60
[   18.436176]  ret_from_fork+0x2c/0x40
[   18.436177] Code: 49 c7 46 20 00 00 00 00 4c 89 ef c6 07 00 0f 1f 40 00 45 
31 ed c7 45 b0 05 00 00 00 44 89 6d b4 4d 89 f5 4c 8b 75 a8 49 8b 45 20 <48> 8b 
b0 90 07 00 00 48 8b 56 10 8b 42 10 48 8d 7a 28 85 c0 0f
[   18.436192] RIP: send_mode_select+0xca/0x560 RSP: c90002bdbda8
[   18.436192] CR2: 0790
[   18.436198] ---[ end trace 40f3e4dca1ffabdd ]---
[   18.436199] Kernel panic - not syncing: Fatal exception
[   18.436222] Kernel Offset: disabled
[-- MARK -- Thu May 18 11:45:00 2017]

Fixes: 3278255 scsi_dh_rdac: switch to scsi_execute_req_flags()
Cc: sta...@vger.kernel.org
Signed-off-by: Artem Savkov 
---
 drivers/scsi/device_handler/scsi_dh_rdac.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c 
b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 3cbab87..2ceff58 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -265,18 +265,16 @@ static unsigned int rdac_failover_get(struct 
rdac_controller *ctlr,
  struct list_head *list,
  unsigned char *cdb)
 {
-   struct scsi_device *sdev = ctlr->ms_sdev;
-   struct rdac_dh_data *h = sdev->handler_data;
struct rdac_mode_common *common;
unsigned data_size;
struct rdac_queue_data *qdata;
u8 *lun_table;
 
-   if (h->ctlr->use_ms10) {
+   if (ctlr->use_ms10) {
struct rdac_pg_expanded *rdac_pg;
 
data_size = sizeof(struct rdac_pg_expanded);
-   rdac_pg = >ctlr->mode_select.expanded;
+   rdac_pg = >mode_select.expanded;
memset(rdac_pg, 0, data_size);
common = _pg->common;
rdac_pg->page_code = RDAC_PAGE_CODE_REDUNDANT_CONTROLLER + 0x40;
@@ -288,7 +286,7 @@ static unsigned int rdac_failover_get(struct 
rdac_controller *ctlr,
struct rdac_pg_legacy *rdac_pg;
 
data_size = sizeof(struct rdac_pg_legacy);
-   rdac_pg = >ctlr->mode_select.legacy;
+   rdac_pg = >mode_select.legacy;
memset(rdac_pg, 0, data_size);
common = _pg->common;
rdac_pg->page_code = RDAC_PAGE_CODE_REDUNDANT_CONTROLLER;
@@ -304,7 +302,7 @@ static unsigned int rdac_failover_get(struct 
rdac_controller *ctlr,
}
 
/* Prepare the command. */
-   if (h->ctlr->use_ms10) {
+   if (ctlr->use_ms10) {
cdb[0] = MODE_SELECT_10;