subject:"sr\: get\/drop reference to device in revalidate and check

Re: sr: get/drop reference to device in revalidate and check_events

2018-04-12 Thread Martin K. Petersen


Jens,

> We can't just use scsi_cd() to get the scsi_cd structure, we have
> to grab a live reference to the device. For both callbacks, we're
> not inside an open where we already hold a reference to the device.

Applied to 4.17/scsi-fixes, thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: sr: get/drop reference to device in revalidate and check_events

2018-04-11 Thread Jan Kara

On Wed 11-04-18 10:23:30, Jens Axboe wrote:
> We can't just use scsi_cd() to get the scsi_cd structure, we have
> to grab a live reference to the device. For both callbacks, we're
> not inside an open where we already hold a reference to the device.
> 
> This fixes device removal/addition under concurrent device access,
> which otherwise could result in the below oops.
> 
> NULL pointer dereference at 0010
> PGD 0 P4D 0 
> Oops:  [#1] PREEMPT SMP
> Modules linked in:
> sr 12:0:0:0: [sr2] scsi-1 drive
>  scsi_debug crc_t10dif crct10dif_generic crct10dif_common nvme nvme_core 
> sb_edac xl
> sr 12:0:0:0: Attached scsi CD-ROM sr2
>  sr_mod cdrom btrfs xor zstd_decompress zstd_compress xxhash lzo_compress 
> zlib_defc
> sr 12:0:0:0: Attached scsi generic sg7 type 5
>  igb ahci libahci i2c_algo_bit libata dca [last unloaded: crc_t10dif]
> CPU: 43 PID: 4629 Comm: systemd-udevd Not tainted 4.16.0+ #650
> Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016
> RIP: 0010:sr_block_revalidate_disk+0x23/0x190 [sr_mod]
> RSP: 0018:883ff357bb58 EFLAGS: 00010292
> RAX: a00b07d0 RBX: 883ff3058000 RCX: 883ff357bb66
> RDX: 0003 RSI: 7530 RDI: 881fea631000
> RBP:  R08: 881fe4d38400 R09: 
> R10:  R11: 01b6 R12: 085d
> R13: 085d R14: 883ffd9b3790 R15: 
> FS:  7f7dc8e6d8c0() GS:883fff34() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 0010 CR3: 003ffda98005 CR4: 003606e0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> Call Trace:
>  ? __invalidate_device+0x48/0x60
>  check_disk_change+0x4c/0x60
>  sr_block_open+0x16/0xd0 [sr_mod]
>  __blkdev_get+0xb9/0x450
>  ? iget5_locked+0x1c0/0x1e0
>  blkdev_get+0x11e/0x320
>  ? bdget+0x11d/0x150
>  ? _raw_spin_unlock+0xa/0x20
>  ? bd_acquire+0xc0/0xc0
>  do_dentry_open+0x1b0/0x320
>  ? inode_permission+0x24/0xc0
>  path_openat+0x4e6/0x1420
>  ? cpumask_any_but+0x1f/0x40
>  ? flush_tlb_mm_range+0xa0/0x120
>  do_filp_open+0x8c/0xf0
>  ? __seccomp_filter+0x28/0x230
>  ? _raw_spin_unlock+0xa/0x20
>  ? __handle_mm_fault+0x7d6/0x9b0
>  ? list_lru_add+0xa8/0xc0
>  ? _raw_spin_unlock+0xa/0x20
>  ? __alloc_fd+0xaf/0x160
>  ? do_sys_open+0x1a6/0x230
>  do_sys_open+0x1a6/0x230
>  do_syscall_64+0x5a/0x100
>  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
> 
> Signed-off-by: Jens Axboe 

Looks good to me. You can add:

Reviewed-by: Jan Kara 

Honza

> 
> ---
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 0cf25d789d05..3f3cb72e0c0c 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -587,18 +587,28 @@ static int sr_block_ioctl(struct block_device *bdev, 
> fmode_t mode, unsigned cmd,
>  static unsigned int sr_block_check_events(struct gendisk *disk,
> unsigned int clearing)
>  {
> - struct scsi_cd *cd = scsi_cd(disk);
> + unsigned int ret = 0;
> + struct scsi_cd *cd;
>  
> - if (atomic_read(>device->disk_events_disable_depth))
> + cd = scsi_cd_get(disk);
> + if (!cd)
>   return 0;
>  
> - return cdrom_check_events(>cdi, clearing);
> + if (!atomic_read(>device->disk_events_disable_depth))
> + ret = cdrom_check_events(>cdi, clearing);
> +
> + scsi_cd_put(cd);
> + return ret;
>  }
>  
>  static int sr_block_revalidate_disk(struct gendisk *disk)
>  {
> - struct scsi_cd *cd = scsi_cd(disk);
>   struct scsi_sense_hdr sshdr;
> + struct scsi_cd *cd;
> +
> + cd = scsi_cd_get(disk);
> + if (!cd)
> + return -ENXIO;
>  
>   /* if the unit is not ready, nothing more to do */
>   if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, ))
> @@ -607,6 +617,7 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
>   sr_cd_check(>cdi);
>   get_sectorsize(cd);
>  out:
> + scsi_cd_put(cd);
>   return 0;
>  }
>  
> 
> -- 
> Jens Axboe
> 
-- 
Jan Kara 
SUSE Labs, CR

Re: sr: get/drop reference to device in revalidate and check_events

2018-04-11 Thread Lee Duncan

On 04/11/2018 09:23 AM, Jens Axboe wrote:
> We can't just use scsi_cd() to get the scsi_cd structure, we have
> to grab a live reference to the device. For both callbacks, we're
> not inside an open where we already hold a reference to the device.
> 
> This fixes device removal/addition under concurrent device access,
> which otherwise could result in the below oops.
> 
> NULL pointer dereference at 0010
> PGD 0 P4D 0 
> Oops:  [#1] PREEMPT SMP
> Modules linked in:
> sr 12:0:0:0: [sr2] scsi-1 drive
>  scsi_debug crc_t10dif crct10dif_generic crct10dif_common nvme nvme_core 
> sb_edac xl
> sr 12:0:0:0: Attached scsi CD-ROM sr2
>  sr_mod cdrom btrfs xor zstd_decompress zstd_compress xxhash lzo_compress 
> zlib_defc
> sr 12:0:0:0: Attached scsi generic sg7 type 5
>  igb ahci libahci i2c_algo_bit libata dca [last unloaded: crc_t10dif]
> CPU: 43 PID: 4629 Comm: systemd-udevd Not tainted 4.16.0+ #650
> Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016
> RIP: 0010:sr_block_revalidate_disk+0x23/0x190 [sr_mod]
> RSP: 0018:883ff357bb58 EFLAGS: 00010292
> RAX: a00b07d0 RBX: 883ff3058000 RCX: 883ff357bb66
> RDX: 0003 RSI: 7530 RDI: 881fea631000
> RBP:  R08: 881fe4d38400 R09: 
> R10:  R11: 01b6 R12: 085d
> R13: 085d R14: 883ffd9b3790 R15: 
> FS:  7f7dc8e6d8c0() GS:883fff34() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 0010 CR3: 003ffda98005 CR4: 003606e0
> DR0:  DR1:  DR2: 
> DR3:  DR6: fffe0ff0 DR7: 0400
> Call Trace:
>  ? __invalidate_device+0x48/0x60
>  check_disk_change+0x4c/0x60
>  sr_block_open+0x16/0xd0 [sr_mod]
>  __blkdev_get+0xb9/0x450
>  ? iget5_locked+0x1c0/0x1e0
>  blkdev_get+0x11e/0x320
>  ? bdget+0x11d/0x150
>  ? _raw_spin_unlock+0xa/0x20
>  ? bd_acquire+0xc0/0xc0
>  do_dentry_open+0x1b0/0x320
>  ? inode_permission+0x24/0xc0
>  path_openat+0x4e6/0x1420
>  ? cpumask_any_but+0x1f/0x40
>  ? flush_tlb_mm_range+0xa0/0x120
>  do_filp_open+0x8c/0xf0
>  ? __seccomp_filter+0x28/0x230
>  ? _raw_spin_unlock+0xa/0x20
>  ? __handle_mm_fault+0x7d6/0x9b0
>  ? list_lru_add+0xa8/0xc0
>  ? _raw_spin_unlock+0xa/0x20
>  ? __alloc_fd+0xaf/0x160
>  ? do_sys_open+0x1a6/0x230
>  do_sys_open+0x1a6/0x230
>  do_syscall_64+0x5a/0x100
>  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
> 
> Signed-off-by: Jens Axboe 
> 
> ---
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 0cf25d789d05..3f3cb72e0c0c 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -587,18 +587,28 @@ static int sr_block_ioctl(struct block_device *bdev, 
> fmode_t mode, unsigned cmd,
>  static unsigned int sr_block_check_events(struct gendisk *disk,
> unsigned int clearing)
>  {
> - struct scsi_cd *cd = scsi_cd(disk);
> + unsigned int ret = 0;
> + struct scsi_cd *cd;
>  
> - if (atomic_read(>device->disk_events_disable_depth))
> + cd = scsi_cd_get(disk);
> + if (!cd)
>   return 0;
>  
> - return cdrom_check_events(>cdi, clearing);
> + if (!atomic_read(>device->disk_events_disable_depth))
> + ret = cdrom_check_events(>cdi, clearing);
> +
> + scsi_cd_put(cd);
> + return ret;
>  }
>  
>  static int sr_block_revalidate_disk(struct gendisk *disk)
>  {
> - struct scsi_cd *cd = scsi_cd(disk);
>   struct scsi_sense_hdr sshdr;
> + struct scsi_cd *cd;
> +
> + cd = scsi_cd_get(disk);
> + if (!cd)
> + return -ENXIO;
>  
>   /* if the unit is not ready, nothing more to do */
>   if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, ))
> @@ -607,6 +617,7 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
>   sr_cd_check(>cdi);
>   get_sectorsize(cd);
>  out:
> + scsi_cd_put(cd);
>   return 0;
>  }
>  
> 

Reviewed-by: Lee Duncan

sr: get/drop reference to device in revalidate and check_events

2018-04-11 Thread Jens Axboe

We can't just use scsi_cd() to get the scsi_cd structure, we have
to grab a live reference to the device. For both callbacks, we're
not inside an open where we already hold a reference to the device.

This fixes device removal/addition under concurrent device access,
which otherwise could result in the below oops.

NULL pointer dereference at 0010
PGD 0 P4D 0 
Oops:  [#1] PREEMPT SMP
Modules linked in:
sr 12:0:0:0: [sr2] scsi-1 drive
 scsi_debug crc_t10dif crct10dif_generic crct10dif_common nvme nvme_core 
sb_edac xl
sr 12:0:0:0: Attached scsi CD-ROM sr2
 sr_mod cdrom btrfs xor zstd_decompress zstd_compress xxhash lzo_compress 
zlib_defc
sr 12:0:0:0: Attached scsi generic sg7 type 5
 igb ahci libahci i2c_algo_bit libata dca [last unloaded: crc_t10dif]
CPU: 43 PID: 4629 Comm: systemd-udevd Not tainted 4.16.0+ #650
Hardware name: Dell Inc. PowerEdge T630/0NT78X, BIOS 2.3.4 11/09/2016
RIP: 0010:sr_block_revalidate_disk+0x23/0x190 [sr_mod]
RSP: 0018:883ff357bb58 EFLAGS: 00010292
RAX: a00b07d0 RBX: 883ff3058000 RCX: 883ff357bb66
RDX: 0003 RSI: 7530 RDI: 881fea631000
RBP:  R08: 881fe4d38400 R09: 
R10:  R11: 01b6 R12: 085d
R13: 085d R14: 883ffd9b3790 R15: 
FS:  7f7dc8e6d8c0() GS:883fff34() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0010 CR3: 003ffda98005 CR4: 003606e0
DR0:  DR1:  DR2: 
DR3:  DR6: fffe0ff0 DR7: 0400
Call Trace:
 ? __invalidate_device+0x48/0x60
 check_disk_change+0x4c/0x60
 sr_block_open+0x16/0xd0 [sr_mod]
 __blkdev_get+0xb9/0x450
 ? iget5_locked+0x1c0/0x1e0
 blkdev_get+0x11e/0x320
 ? bdget+0x11d/0x150
 ? _raw_spin_unlock+0xa/0x20
 ? bd_acquire+0xc0/0xc0
 do_dentry_open+0x1b0/0x320
 ? inode_permission+0x24/0xc0
 path_openat+0x4e6/0x1420
 ? cpumask_any_but+0x1f/0x40
 ? flush_tlb_mm_range+0xa0/0x120
 do_filp_open+0x8c/0xf0
 ? __seccomp_filter+0x28/0x230
 ? _raw_spin_unlock+0xa/0x20
 ? __handle_mm_fault+0x7d6/0x9b0
 ? list_lru_add+0xa8/0xc0
 ? _raw_spin_unlock+0xa/0x20
 ? __alloc_fd+0xaf/0x160
 ? do_sys_open+0x1a6/0x230
 do_sys_open+0x1a6/0x230
 do_syscall_64+0x5a/0x100
 entry_SYSCALL_64_after_hwframe+0x3d/0xa2

Signed-off-by: Jens Axboe 

---

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0cf25d789d05..3f3cb72e0c0c 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -587,18 +587,28 @@ static int sr_block_ioctl(struct block_device *bdev, 
fmode_t mode, unsigned cmd,
 static unsigned int sr_block_check_events(struct gendisk *disk,
  unsigned int clearing)
 {
-   struct scsi_cd *cd = scsi_cd(disk);
+   unsigned int ret = 0;
+   struct scsi_cd *cd;
 
-   if (atomic_read(>device->disk_events_disable_depth))
+   cd = scsi_cd_get(disk);
+   if (!cd)
return 0;
 
-   return cdrom_check_events(>cdi, clearing);
+   if (!atomic_read(>device->disk_events_disable_depth))
+   ret = cdrom_check_events(>cdi, clearing);
+
+   scsi_cd_put(cd);
+   return ret;
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)
 {
-   struct scsi_cd *cd = scsi_cd(disk);
struct scsi_sense_hdr sshdr;
+   struct scsi_cd *cd;
+
+   cd = scsi_cd_get(disk);
+   if (!cd)
+   return -ENXIO;
 
/* if the unit is not ready, nothing more to do */
if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, ))
@@ -607,6 +617,7 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
sr_cd_check(>cdi);
get_sectorsize(cd);
 out:
+   scsi_cd_put(cd);
return 0;
 }
 

-- 
Jens Axboe

Re: sr: get/drop reference to device in revalidate and check_events

Re: sr: get/drop reference to device in revalidate and check_events

Re: sr: get/drop reference to device in revalidate and check_events

sr: get/drop reference to device in revalidate and check_events

4 matches

Site Navigation

Mail list logo

Footer information