Skip to content

Commit 97463b2

Browse files
gburdwkozaczuk
authored andcommitted
block: fix discard-path hangs and bounds on the BLKDISCARD ioctl
The generic BLKDISCARD ioctl submits a BIO_DISCARD and waits on bio_wait(). Drivers that do not implement discard returned ENOTBLK from their bio_cmd switch without completing the bio, so the wait hung. Complete the bio with error in the default case of nvme, ahci, ide and scsi so an unsupported discard fails cleanly instead of hanging. multiplex_strategy split a request larger than max_io_size by doing pointer arithmetic on bio_data, which is nullptr for a discard (no data payload). Forward discard bios whole and let the driver enforce its own size limit. In virtio-blk, restrict the SEG_MAX segment-count check to READ/WRITE (the only requests with a data payload) and complete the bio on failure so a waiting caller is not left hung. Reject a discard whose sector count would overflow the u32 num_sectors field or exceed the advertised max_discard_sectors, rather than truncating and discarding the wrong range.
1 parent eaf958b commit 97463b2

7 files changed

Lines changed: 49 additions & 8 deletions

File tree

drivers/ahci.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,7 @@ int port::make_request(struct bio* bio)
299299
disk_flush(bio);
300300
break;
301301
default:
302+
biodone(bio, false);
302303
return ENOTBLK;
303304
}
304305
return 0;

drivers/ide.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@ int ide_drive::make_request(struct bio* bio)
134134
}
135135
break;
136136
default:
137+
biodone(bio, false);
137138
return ENOTBLK;
138139
}
139140

drivers/nvme-queue.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,7 @@ int io_queue_pair::make_request(struct bio* bio, u32 nsid = 1)
313313

314314
default:
315315
NVME_ERROR("Operation not implemented\n");
316+
biodone(bio, false);
316317
return ENOTBLK;
317318
}
318319
return 0;

drivers/nvme.cc

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,20 @@ int driver::identify_namespace(u32 nsid)
432432

433433
int driver::make_request(bio* bio, u32 nsid)
434434
{
435+
// Reject commands this driver does not implement (e.g. BIO_DISCARD) up
436+
// front, before the block-alignment math and >> blockshift below run on a
437+
// bio whose offset/bcount are not sector counts. The per-queue path also
438+
// rejects unknown commands, but doing it here avoids the misleading
439+
// alignment error / debug-only assert on a discard request.
440+
switch (bio->bio_cmd) {
441+
case BIO_READ:
442+
case BIO_WRITE:
443+
case BIO_FLUSH:
444+
break;
445+
default:
446+
biodone(bio, false);
447+
return EOPNOTSUPP;
448+
}
435449
if (bio->bio_bcount % _ns_data[nsid]->blocksize || bio->bio_offset % _ns_data[nsid]->blocksize) {
436450
NVME_ERROR("bio request not block-aligned length=%d, offset=%d blocksize=%d\n",bio->bio_bcount, bio->bio_offset, _ns_data[nsid]->blocksize);
437451
return EINVAL;

drivers/scsi-common.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,7 @@ int scsi_common::handle_bio(u16 target, u16 lun, struct bio *bio)
310310
exec_cmd(bio);
311311
break;
312312
default:
313+
biodone(bio, false);
313314
return ENOTBLK;
314315
}
315316
return 0;

drivers/virtio-blk.cc

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -382,13 +382,6 @@ int blk::make_request(struct bio* bio)
382382
{
383383
if (!bio) return EIO;
384384

385-
if (get_guest_feature_bit(VIRTIO_BLK_F_SEG_MAX)) {
386-
if (bio->bio_bcount/mmu::page_size + 1 > _config.seg_max) {
387-
trace_virtio_blk_make_request_seg_max(bio->bio_bcount, _config.seg_max);
388-
return EIO;
389-
}
390-
}
391-
392385
/* Select a queue by CPU id so parallel CPUs use independent rings. */
393386
int qid = sched::cpu::current()->id % _num_queues;
394387

@@ -425,12 +418,38 @@ int blk::make_request(struct bio* bio)
425418
biodone(bio, false);
426419
return EINVAL;
427420
}
421+
// num_sectors is a u32 in the virtio descriptor and the device
422+
// advertises an upper bound via max_discard_sectors. Reject a
423+
// range that would overflow the field or exceed the device limit
424+
// rather than truncating it and discarding the wrong amount.
425+
{
426+
u64 nsectors = bio->bio_bcount / sector_size;
427+
u32 max_sectors = _config.max_discard_sectors ?
428+
_config.max_discard_sectors : UINT32_MAX;
429+
if (nsectors > max_sectors) {
430+
biodone(bio, false);
431+
return EINVAL;
432+
}
433+
}
428434
type = VIRTIO_BLK_T_DISCARD;
429435
break;
430436
default:
437+
biodone(bio, false);
431438
return ENOTBLK;
432439
}
433440

441+
// SEG_MAX bounds the number of data segments a request may span, so it
442+
// only applies to requests that carry a data payload (READ/WRITE).
443+
// FLUSH and DISCARD add no data SG, so skip the check for them.
444+
if ((type == VIRTIO_BLK_T_IN || type == VIRTIO_BLK_T_OUT) &&
445+
get_guest_feature_bit(VIRTIO_BLK_F_SEG_MAX)) {
446+
if (bio->bio_bcount/mmu::page_size + 1 > _config.seg_max) {
447+
trace_virtio_blk_make_request_seg_max(bio->bio_bcount, _config.seg_max);
448+
biodone(bio, false);
449+
return EIO;
450+
}
451+
}
452+
434453
auto* req = new blk_req(bio);
435454
blk_outhdr* hdr = &req->hdr;
436455
hdr->type = type;

fs/vfs/kern_physio.cc

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,11 @@ void multiplex_strategy(struct bio *bio)
107107

108108
assert(strategy != nullptr);
109109

110-
if (len <= dev->max_io_size) {
110+
// A discard request carries no data payload (bio_data is nullptr) even
111+
// though bio_bcount is non-zero, so the max_io_size data-segment limit
112+
// does not apply and splitting it would do pointer arithmetic on nullptr.
113+
// Forward it whole; the driver enforces its own discard-size limit.
114+
if (bio->bio_cmd == BIO_DISCARD || len <= dev->max_io_size) {
111115
strategy(bio);
112116
return;
113117
}

0 commit comments

Comments
 (0)