dm: gracefully fail any request beyond the end of the device
PATCH 1/1 dm: gracefully fail any request beyond the end of the device
The access beyond the end of device BUG_ON that was introduced to dm_request_fn via commit 29e4013de7ad950280e4b2208 ("dm: implement REQ_FLUSH/FUA support for request-based dm") is an overly drastic response. Use dm_kill_unmapped_request() to fail the original and clone request with -EIO.
map_request() will assign the valid target returned by dm_table_find_target() to tio->ti. But in the case where the target isn't valid tio->ti is never assigned (because map_request isn't called); so add a check for tio->ti != NULL to dm_done().
--- drivers/md/dm.c | 22 +++++++++++++++++----- 1 files changed, 17 insertions(+), 5 deletions(-)
drivers/md/dm.c +17 -5
@@ -865,7 +865,10 @@ static void dm_done(struct request *clone, int error, bool mapped) {int r = error;struct dm_rq_target_io *tio = clone->end_io_data; -dm_request_endio_fn rq_end_io = tio->ti->type->rq_end_io; +dm_request_endio_fn rq_end_io =NULL ; + +if (tio->ti) + rq_end_io = tio->ti->type->rq_end_io;if (mapped && rq_end_io) r =rq_end_io (tio->ti, clone, error, &tio->info);
@@ -1651,19 +1654,30 @@ static void dm_request_fn(struct request_queue *q)if (!rq) goto delay_and_out; + clone = rq->special; +/* always use block 0 to find the target for flushes for now */ pos =0 ;if (!(rq->cmd_flags &REQ_FLUSH )) pos =blk_rq_pos (rq); ti =dm_table_find_target (map, pos); -BUG_ON (!dm_target_is_valid (ti)); +if (!dm_target_is_valid (ti)) { +/* +* Must perform setup, that dm_done() requires, +* before calling dm_kill_unmapped_request +*/ +blk_start_request (rq); +atomic_inc (&md->pending[rq_data_dir (clone)]); +dm_get (md); +dm_kill_unmapped_request (clone, -EIO ); + goto out; + }if (ti->type->busy && ti->type->busy (ti)) goto delay_and_out;blk_start_request (rq); - clone = rq->special;atomic_inc (&md->pending[rq_data_dir (clone)]);spin_unlock (q->queue_lock);
@@ -1684,8 +1698,6 @@ delay_and_out:blk_delay_queue (q,HZ /10 ); out:dm_table_put (map); - -return ; }int dm_underlying_device_busy (struct request_queue *q)
PATCH v2 1/1 dm: gracefully fail any request beyond the end of the device
The access beyond the end of device BUG_ON that was introduced to dm_request_fn via commit 29e4013de7ad950280e4b2208 ("dm: implement REQ_FLUSH/FUA support for request-based dm") is an overly drastic response. Use dm_kill_unmapped_request() to fail the clone and original request with -EIO.
map_request() will assign the valid target returned by dm_table_find_target to tio->ti. But in the case where the target isn't valid tio->ti is never assigned (because map_request isn't called); so add a check for tio->ti != NULL to dm_done().
--- drivers/md/dm.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
v2: added a DMERR_LIMIT message to give context for the IO errors
drivers/md/dm.c +18 -5
@@ -865,7 +865,10 @@ static void dm_done(struct request *clon {int r = error;struct dm_rq_target_io *tio = clone->end_io_data; -dm_request_endio_fn rq_end_io = tio->ti->type->rq_end_io; +dm_request_endio_fn rq_end_io =NULL ; + +if (tio->ti) + rq_end_io = tio->ti->type->rq_end_io;if (mapped && rq_end_io) r =rq_end_io (tio->ti, clone, error, &tio->info);
@@ -1651,19 +1654,31 @@ static void dm_request_fn(struct requestif (!rq) goto delay_and_out; + clone = rq->special; +/* always use block 0 to find the target for flushes for now */ pos =0 ;if (!(rq->cmd_flags &REQ_FLUSH )) pos =blk_rq_pos (rq); ti =dm_table_find_target (map, pos); -BUG_ON (!dm_target_is_valid (ti)); +if (!dm_target_is_valid (ti)) { +/* +* Must perform setup, that dm_done() requires, +* before calling dm_kill_unmapped_request +*/ +DMERR_LIMIT ("request attempted access beyond the end of device" ); +blk_start_request (rq); +atomic_inc (&md->pending[rq_data_dir (clone)]); +dm_get (md); +dm_kill_unmapped_request (clone, -EIO ); + goto out; + }if (ti->type->busy && ti->type->busy (ti)) goto delay_and_out;blk_start_request (rq); - clone = rq->special;atomic_inc (&md->pending[rq_data_dir (clone)]);spin_unlock (q->queue_lock);
@@ -1684,8 +1699,6 @@ delay_and_out:blk_delay_queue (q,HZ /10 ); out:dm_table_put (map); - -return ; }int dm_underlying_device_busy (struct request_queue *q)
Quoted 14 lines · Mike Snitzer
(quoted diff) +3
-1@@ -1651,19 +1654,31 @@ static void dm_request_fn(struct request if (!rq) goto delay_and_out; + clone = rq->special; + /* always use block 0 to find the target for flushes for now */ pos = 0; if (!(rq->cmd_flags & REQ_FLUSH)) pos = blk_rq_pos(rq); ti = dm_table_find_target(map, pos); - BUG_ON(!dm_target_is_valid(ti)); + if (!dm_target_is_valid(ti)) {
Review static void dm_request_fn(struct request · line 1675
Quoted 10 lines
(quoted diff) +10
-0+ /* + * Must perform setup, that dm_done() requires, + * before calling dm_kill_unmapped_request + */ + DMERR_LIMIT("request attempted access beyond the end of device"); + blk_start_request(rq); + atomic_inc(&md->pending[rq_data_dir(clone)]); + dm_get(md); + dm_kill_unmapped_request(clone, -EIO); + goto out;
This "goto out" should be "continue" so that request_fn process next requests in the queue.
Also I think introducing a function dm_start_request() will make this part of code a little bit easier for reading. An edited patch is attached.
Signature · 1 line
Jun'ichi Nomura, NEC Corporation
drivers/md/dm.c +35 -16
@@ -865,7 +865,10 @@ static void dm_done(struct request *clone, int error, bool mapped) {int r = error;struct dm_rq_target_io *tio = clone->end_io_data; -dm_request_endio_fn rq_end_io = tio->ti->type->rq_end_io; +dm_request_endio_fn rq_end_io =NULL ; + +if (tio->ti) + rq_end_io = tio->ti->type->rq_end_io;if (mapped && rq_end_io) r =rq_end_io (tio->ti, clone, error, &tio->info);
@@ -1566,15 +1569,6 @@ static int map_request(struct dm_target *ti, struct request *clone,int r, requeued =0 ;struct dm_rq_target_io *tio = clone->end_io_data; -/* -* Hold the md reference here for the in-flight I/O. -* We can't rely on the reference count by device opener, -* because the device may be closed during the request completion -* when all bios are completed. -* See the comment in rq_completed() too. -*/ -dm_get (md); - tio->ti = ti; r = ti->type->map_rq (ti, clone, &tio->info);switch (r) {
@@ -1606,6 +1600,26 @@ static int map_request(struct dm_target *ti, struct request *clone,return requeued; } +static struct request *dm_start_request (struct mapped_device *md,struct request *orig) +{ +struct request *clone; + +blk_start_request (orig); + clone = orig->special; +atomic_inc (&md->pending[rq_data_dir (clone)]); + +/* +* Hold the md reference here for the in-flight I/O. +* We can't rely on the reference count by device opener, +* because the device may be closed during the request completion +* when all bios are completed. +* See the comment in rq_completed() too. +*/ +dm_get (md); + +return clone; +} +/* * q->request_fn for request-based dm. * Called with the queue lock held.
@@ -1635,14 +1649,21 @@ static void dm_request_fn(struct request_queue *q) pos =blk_rq_pos (rq); ti =dm_table_find_target (map, pos); -BUG_ON (!dm_target_is_valid (ti)); +if (!dm_target_is_valid (ti)) { +/* +* Must perform setup, that dm_done() requires, +* before calling dm_kill_unmapped_request +*/ +DMERR_LIMIT ("request attempted access beyond the end of device" ); + clone =dm_start_request (md, rq); +dm_kill_unmapped_request (clone, -EIO ); +continue ; + }if (ti->type->busy && ti->type->busy (ti)) goto delay_and_out; -blk_start_request (rq); - clone = rq->special; -atomic_inc (&md->pending[rq_data_dir (clone)]); + clone =dm_start_request (md, rq);spin_unlock (q->queue_lock);if (map_request (ti, clone, md))
@@ -1662,8 +1683,6 @@ delay_and_out:blk_delay_queue (q,HZ /10 ); out:dm_table_put (map); - -return ; }int dm_underlying_device_busy (struct request_queue *q)
Quoted 29 lines · Jun'ichi Nomura
Quoted 14 lines · Mike Snitzer
(quoted diff) +3
-1@@ -1651,19 +1654,31 @@ static void dm_request_fn(struct request if (!rq) goto delay_and_out; + clone = rq->special; + /* always use block 0 to find the target for flushes for now */ pos = 0; if (!(rq->cmd_flags & REQ_FLUSH)) pos = blk_rq_pos(rq); ti = dm_table_find_target(map, pos); - BUG_ON(!dm_target_is_valid(ti)); + if (!dm_target_is_valid(ti)) {Review static void dm_request_fn(struct request · line 1675
Quoted 10 lines
(quoted diff) +10
-0+ /* + * Must perform setup, that dm_done() requires, + * before calling dm_kill_unmapped_request + */ + DMERR_LIMIT("request attempted access beyond the end of device"); + blk_start_request(rq); + atomic_inc(&md->pending[rq_data_dir(clone)]); + dm_get(md); + dm_kill_unmapped_request(clone, -EIO); + goto out;This "goto out" should be "continue" so that
request_fnprocess next requests in the queue.
Quoted 3 lines
Also I think introducing a function
dm_start_request()will make this part of code a little bit easier for reading. An edited patch is attached.
Aside from the continue, matches exactly what I was going to do for v3 (based on Mike Christie's feedback -- which was to introduce dm_start_request too). Anyway, looks great.
I'll get a formal v3 posted so Alasdair can stage it.
Thanks,
Mike
PATCH v3 1/1 dm: gracefully fail any request beyond the end of the device
The access beyond the end of device BUG_ON that was introduced to dm_request_fn via commit 29e4013de7ad950280e4b2208 ("dm: implement REQ_FLUSH/FUA support for request-based dm") is an overly drastic response. Use dm_kill_unmapped_request() to fail the clone and original request with -EIO.
map_request() will assign the valid target returned by dm_table_find_target to tio->ti. But in the case where the target isn't valid tio->ti is never assigned (because map_request isn't called); so add a check for tio->ti != NULL to dm_done().
--- drivers/md/dm.c | 51 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 16 deletions(-)
v2: added a DMERR_LIMIT message to give context for the IO errors
v3: folded in Jun'ichi's changes: dm_start_request and continue
drivers/md/dm.c +35 -16
@@ -865,7 +865,10 @@ static void dm_done(struct request *clon {int r = error;struct dm_rq_target_io *tio = clone->end_io_data; -dm_request_endio_fn rq_end_io = tio->ti->type->rq_end_io; +dm_request_endio_fn rq_end_io =NULL ; + +if (tio->ti) + rq_end_io = tio->ti->type->rq_end_io;if (mapped && rq_end_io) r =rq_end_io (tio->ti, clone, error, &tio->info);
@@ -1588,15 +1591,6 @@ static int map_request(struct dm_targetint r, requeued =0 ;struct dm_rq_target_io *tio = clone->end_io_data; -/* -* Hold the md reference here for the in-flight I/O. -* We can't rely on the reference count by device opener, -* because the device may be closed during the request completion -* when all bios are completed. -* See the comment in rq_completed() too. -*/ -dm_get (md); - tio->ti = ti; r = ti->type->map_rq (ti, clone, &tio->info);switch (r) {
@@ -1628,6 +1622,26 @@ static int map_request(struct dm_targetreturn requeued; } +static struct request *dm_start_request (struct mapped_device *md,struct request *orig) +{ +struct request *clone; + +blk_start_request (orig); + clone = orig->special; +atomic_inc (&md->pending[rq_data_dir (clone)]); + +/* +* Hold the md reference here for the in-flight I/O. +* We can't rely on the reference count by device opener, +* because the device may be closed during the request completion +* when all bios are completed. +* See the comment in rq_completed() too. +*/ +dm_get (md); + +return clone; +} +/* * q->request_fn for request-based dm. * Called with the queue lock held.
@@ -1657,14 +1671,21 @@ static void dm_request_fn(struct request pos =blk_rq_pos (rq); ti =dm_table_find_target (map, pos); -BUG_ON (!dm_target_is_valid (ti)); +if (!dm_target_is_valid (ti)) { +/* +* Must perform setup, that dm_done() requires, +* before calling dm_kill_unmapped_request +*/ +DMERR_LIMIT ("request attempted access beyond the end of device" ); + clone =dm_start_request (md, rq); +dm_kill_unmapped_request (clone, -EIO ); +continue ; + }if (ti->type->busy && ti->type->busy (ti)) goto delay_and_out; -blk_start_request (rq); - clone = rq->special; -atomic_inc (&md->pending[rq_data_dir (clone)]); + clone =dm_start_request (md, rq);spin_unlock (q->queue_lock);if (map_request (ti, clone, md))
@@ -1684,8 +1705,6 @@ delay_and_out:blk_delay_queue (q,HZ /10 ); out:dm_table_put (map); - -return ; }int dm_underlying_device_busy (struct request_queue *q)