* sr: get/drop reference to device in revalidate and check_events
@ 2018-04-11 16:23 Jens Axboe
2018-04-11 17:10 ` Lee Duncan
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jens Axboe @ 2018-04-11 16:23 UTC (permalink / raw)
To: linux-scsi, linux-block; +Cc: Jan Kara
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 0000000000000010
PGD 0 P4D 0
Oops: 0000 [#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:ffff883ff357bb58 EFLAGS: 00010292
RAX: ffffffffa00b07d0 RBX: ffff883ff3058000 RCX: ffff883ff357bb66
RDX: 0000000000000003 RSI: 0000000000007530 RDI: ffff881fea631000
RBP: 0000000000000000 R08: ffff881fe4d38400 R09: 0000000000000000
R10: 0000000000000000 R11: 00000000000001b6 R12: 000000000800005d
R13: 000000000800005d R14: ffff883ffd9b3790 R15: 0000000000000000
FS: 00007f7dc8e6d8c0(0000) GS:ffff883fff340000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 0000003ffda98005 CR4: 00000000003606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
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 <axboe@kernel.dk>
---
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(&cd->device->disk_events_disable_depth))
+ cd = scsi_cd_get(disk);
+ if (!cd)
return 0;
- return cdrom_check_events(&cd->cdi, clearing);
+ if (!atomic_read(&cd->device->disk_events_disable_depth))
+ ret = cdrom_check_events(&cd->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, &sshdr))
@@ -607,6 +617,7 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
sr_cd_check(&cd->cdi);
get_sectorsize(cd);
out:
+ scsi_cd_put(cd);
return 0;
}
--
Jens Axboe
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: sr: get/drop reference to device in revalidate and check_events
2018-04-11 16:23 sr: get/drop reference to device in revalidate and check_events Jens Axboe
@ 2018-04-11 17:10 ` Lee Duncan
2018-04-11 17:24 ` Jan Kara
2018-04-13 2:41 ` Martin K. Petersen
2 siblings, 0 replies; 4+ messages in thread
From: Lee Duncan @ 2018-04-11 17:10 UTC (permalink / raw)
To: Jens Axboe, linux-scsi, linux-block; +Cc: Jan Kara
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 0000000000000010
> PGD 0 P4D 0
> Oops: 0000 [#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:ffff883ff357bb58 EFLAGS: 00010292
> RAX: ffffffffa00b07d0 RBX: ffff883ff3058000 RCX: ffff883ff357bb66
> RDX: 0000000000000003 RSI: 0000000000007530 RDI: ffff881fea631000
> RBP: 0000000000000000 R08: ffff881fe4d38400 R09: 0000000000000000
> R10: 0000000000000000 R11: 00000000000001b6 R12: 000000000800005d
> R13: 000000000800005d R14: ffff883ffd9b3790 R15: 0000000000000000
> FS: 00007f7dc8e6d8c0(0000) GS:ffff883fff340000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000010 CR3: 0000003ffda98005 CR4: 00000000003606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> 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 <axboe@kernel.dk>
>
> ---
>
> 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(&cd->device->disk_events_disable_depth))
> + cd = scsi_cd_get(disk);
> + if (!cd)
> return 0;
>
> - return cdrom_check_events(&cd->cdi, clearing);
> + if (!atomic_read(&cd->device->disk_events_disable_depth))
> + ret = cdrom_check_events(&cd->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, &sshdr))
> @@ -607,6 +617,7 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
> sr_cd_check(&cd->cdi);
> get_sectorsize(cd);
> out:
> + scsi_cd_put(cd);
> return 0;
> }
>
>
Reviewed-by: Lee Duncan <lduncan@suse.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sr: get/drop reference to device in revalidate and check_events
2018-04-11 16:23 sr: get/drop reference to device in revalidate and check_events Jens Axboe
2018-04-11 17:10 ` Lee Duncan
@ 2018-04-11 17:24 ` Jan Kara
2018-04-13 2:41 ` Martin K. Petersen
2 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2018-04-11 17:24 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-scsi, linux-block, 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 0000000000000010
> PGD 0 P4D 0
> Oops: 0000 [#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:ffff883ff357bb58 EFLAGS: 00010292
> RAX: ffffffffa00b07d0 RBX: ffff883ff3058000 RCX: ffff883ff357bb66
> RDX: 0000000000000003 RSI: 0000000000007530 RDI: ffff881fea631000
> RBP: 0000000000000000 R08: ffff881fe4d38400 R09: 0000000000000000
> R10: 0000000000000000 R11: 00000000000001b6 R12: 000000000800005d
> R13: 000000000800005d R14: ffff883ffd9b3790 R15: 0000000000000000
> FS: 00007f7dc8e6d8c0(0000) GS:ffff883fff340000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000010 CR3: 0000003ffda98005 CR4: 00000000003606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> 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 <axboe@kernel.dk>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>
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(&cd->device->disk_events_disable_depth))
> + cd = scsi_cd_get(disk);
> + if (!cd)
> return 0;
>
> - return cdrom_check_events(&cd->cdi, clearing);
> + if (!atomic_read(&cd->device->disk_events_disable_depth))
> + ret = cdrom_check_events(&cd->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, &sshdr))
> @@ -607,6 +617,7 @@ static int sr_block_revalidate_disk(struct gendisk *disk)
> sr_cd_check(&cd->cdi);
> get_sectorsize(cd);
> out:
> + scsi_cd_put(cd);
> return 0;
> }
>
>
> --
> Jens Axboe
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sr: get/drop reference to device in revalidate and check_events
2018-04-11 16:23 sr: get/drop reference to device in revalidate and check_events Jens Axboe
2018-04-11 17:10 ` Lee Duncan
2018-04-11 17:24 ` Jan Kara
@ 2018-04-13 2:41 ` Martin K. Petersen
2 siblings, 0 replies; 4+ messages in thread
From: Martin K. Petersen @ 2018-04-13 2:41 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-scsi, linux-block, Jan Kara
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-13 2:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-11 16:23 sr: get/drop reference to device in revalidate and check_events Jens Axboe
2018-04-11 17:10 ` Lee Duncan
2018-04-11 17:24 ` Jan Kara
2018-04-13 2:41 ` Martin K. Petersen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.