| Message ID | 1458030083-46905-1-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 15, 2016 at 09:21:23AM +0100, Hannes Reinecke wrote: > We need to check for a valid index before accessing the array > element to avoid accessing invalid memory regions. Looks fine, Reviewed-by: Christoph Hellwig <hch@lst.de> -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hannes Reinecke <hare@suse.de> writes: > We need to check for a valid index before accessing the array > element to avoid accessing invalid memory regions. Hi, Hannes, What's the motivation here? Did you witness an invalid tag being passed in? Isn't that just a bug in the caller? -Jeff > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > block/blk-mq.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 56c0a72..4ea87d5 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -544,6 +544,8 @@ EXPORT_SYMBOL(blk_mq_abort_requeue_list); > > struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) > { > + if (unlikely(tag >= tags->nr_tags)) > + return NULL; > return tags->rqs[tag]; > } > EXPORT_SYMBOL(blk_mq_tag_to_rq); -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 15, 2016 at 10:15:39AM -0400, Jeff Moyer wrote: > Hannes Reinecke <hare@suse.de> writes: > > > We need to check for a valid index before accessing the array > > element to avoid accessing invalid memory regions. > > Hi, Hannes, > > What's the motivation here? Did you witness an invalid tag being passed > in? Isn't that just a bug in the caller? The tag generally comes more or less straight from the wire. So someome should bounds check it, and doing it in one place seems easier than in every driver, and Hannes patch would allow us to remove these checks from nvme. > > -Jeff > > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > --- > > block/blk-mq.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > index 56c0a72..4ea87d5 100644 > > --- a/block/blk-mq.c > > +++ b/block/blk-mq.c > > @@ -544,6 +544,8 @@ EXPORT_SYMBOL(blk_mq_abort_requeue_list); > > > > struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) > > { > > + if (unlikely(tag >= tags->nr_tags)) > > + return NULL; > > return tags->rqs[tag]; > > } > > EXPORT_SYMBOL(blk_mq_tag_to_rq); ---end quoted text--- -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph Hellwig <hch@lst.de> writes: > On Tue, Mar 15, 2016 at 10:15:39AM -0400, Jeff Moyer wrote: >> Hannes Reinecke <hare@suse.de> writes: >> >> > We need to check for a valid index before accessing the array >> > element to avoid accessing invalid memory regions. >> >> Hi, Hannes, >> >> What's the motivation here? Did you witness an invalid tag being passed >> in? Isn't that just a bug in the caller? > > The tag generally comes more or less straight from the wire. So > someome should bounds check it, and doing it in one place seems easier > than in every driver, and Hannes patch would allow us to remove these > checks from nvme. OK, thanks for the explanation. Looks fine to me, then. Reviewed-by: Jeff Moyer <jmoyer@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/15/2016 01:21 AM, Hannes Reinecke wrote: > We need to check for a valid index before accessing the array > element to avoid accessing invalid memory regions. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > block/blk-mq.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 56c0a72..4ea87d5 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -544,6 +544,8 @@ EXPORT_SYMBOL(blk_mq_abort_requeue_list); > > struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) > { > + if (unlikely(tag >= tags->nr_tags)) > + return NULL; > return tags->rqs[tag]; > } > EXPORT_SYMBOL(blk_mq_tag_to_rq); I like adding the check, but I think we should kill the unlikely and just make it: if (tag < tags->nr_tags) return tags->rqs[tag]; return NULL; instead. I'll apply it as such.
On 03/15/2016 08:01 PM, Jens Axboe wrote: > On 03/15/2016 01:21 AM, Hannes Reinecke wrote: >> We need to check for a valid index before accessing the array >> element to avoid accessing invalid memory regions. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> block/blk-mq.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 56c0a72..4ea87d5 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -544,6 +544,8 @@ EXPORT_SYMBOL(blk_mq_abort_requeue_list); >> >> struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, >> unsigned int tag) >> { >> + if (unlikely(tag >= tags->nr_tags)) >> + return NULL; >> return tags->rqs[tag]; >> } >> EXPORT_SYMBOL(blk_mq_tag_to_rq); > > I like adding the check, but I think we should kill the unlikely and > just make it: > > if (tag < tags->nr_tags) > return tags->rqs[tag]; > > return NULL; > > instead. I'll apply it as such. > yeah, I had a discussion about the viability of the 'unlikely' flag given modern compilers. No consensus was reached, so I left it in. Removing it is fine by me, too. Cheers, Hannes
On 03/16/2016 12:03 AM, Hannes Reinecke wrote: > On 03/15/2016 08:01 PM, Jens Axboe wrote: >> On 03/15/2016 01:21 AM, Hannes Reinecke wrote: >>> We need to check for a valid index before accessing the array >>> element to avoid accessing invalid memory regions. >>> >>> Signed-off-by: Hannes Reinecke <hare@suse.com> >>> --- >>> block/blk-mq.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/block/blk-mq.c b/block/blk-mq.c >>> index 56c0a72..4ea87d5 100644 >>> --- a/block/blk-mq.c >>> +++ b/block/blk-mq.c >>> @@ -544,6 +544,8 @@ EXPORT_SYMBOL(blk_mq_abort_requeue_list); >>> >>> struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, >>> unsigned int tag) >>> { >>> + if (unlikely(tag >= tags->nr_tags)) >>> + return NULL; >>> return tags->rqs[tag]; >>> } >>> EXPORT_SYMBOL(blk_mq_tag_to_rq); >> >> I like adding the check, but I think we should kill the unlikely and >> just make it: >> >> if (tag < tags->nr_tags) >> return tags->rqs[tag]; >> >> return NULL; >> >> instead. I'll apply it as such. >> > yeah, I had a discussion about the viability of the 'unlikely' flag > given modern compilers. No consensus was reached, so I left it in. > Removing it is fine by me, too. The hope is that when you flip the case you check for, the fast and expected path is in line and doesn't require a jump.
diff --git a/block/blk-mq.c b/block/blk-mq.c index 56c0a72..4ea87d5 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -544,6 +544,8 @@ EXPORT_SYMBOL(blk_mq_abort_requeue_list); struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag) { + if (unlikely(tag >= tags->nr_tags)) + return NULL; return tags->rqs[tag]; } EXPORT_SYMBOL(blk_mq_tag_to_rq);
We need to check for a valid index before accessing the array element to avoid accessing invalid memory regions. Signed-off-by: Hannes Reinecke <hare@suse.com> --- block/blk-mq.c | 2 ++ 1 file changed, 2 insertions(+)