Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Jens, On Thu, Mar 30, 2017 at 07:38:26PM -0600, Jens Axboe wrote: > On 03/30/2017 05:45 PM, Minchan Kim wrote: > > On Thu, Mar 30, 2017 at 09:35:56AM -0600, Jens Axboe wrote: > >> On 03/30/2017 09:08 AM, Minchan Kim wrote: > >>> Hi Jens, > >>> > >>> It seems you miss this. > >>> Could you handle this? > >> > >> I can, but I'm a little confused. The comment talks about replacing > >> the one I merged with this one, I can't do that. I'm assuming you > >> are talking about this commit: > > > > Right. > > > >> > >> commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f > >> Author: Johannes Thumshirn> >> Date: Mon Mar 6 11:23:35 2017 +0100 > >> > >> zram: set physical queue limits to avoid array out of bounds accesses > >> > >> which is in mainline. The patch still applies, though. > > > > You mean it's already in mainline so you cannot replace but can revert. > > Right? > > If so, please revert it and merge this one. > > Let's please fold it into the other patch. That's cleaner and it makes > logical sense. Understood. > > >> Do we really REALLY need this for 4.11, or can we queue for 4.12 and > >> mark it stable? > > > > Not urgent because one in mainline fixes the problem so I'm okay > > with 4.12 but I don't want mark it as -stable. > > OK good, please resend with the two-line revert in your current > patch, and I'll get it queued up for 4.12. Yeb. If so, now that I think about it, it would be better to handle it via Andrew's tree because Andrew have been handled zram's patches and I have several pending patches based on it. So, I will send new patchset with it to Andrew. Thanks!
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Jens, On Thu, Mar 30, 2017 at 07:38:26PM -0600, Jens Axboe wrote: > On 03/30/2017 05:45 PM, Minchan Kim wrote: > > On Thu, Mar 30, 2017 at 09:35:56AM -0600, Jens Axboe wrote: > >> On 03/30/2017 09:08 AM, Minchan Kim wrote: > >>> Hi Jens, > >>> > >>> It seems you miss this. > >>> Could you handle this? > >> > >> I can, but I'm a little confused. The comment talks about replacing > >> the one I merged with this one, I can't do that. I'm assuming you > >> are talking about this commit: > > > > Right. > > > >> > >> commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f > >> Author: Johannes Thumshirn > >> Date: Mon Mar 6 11:23:35 2017 +0100 > >> > >> zram: set physical queue limits to avoid array out of bounds accesses > >> > >> which is in mainline. The patch still applies, though. > > > > You mean it's already in mainline so you cannot replace but can revert. > > Right? > > If so, please revert it and merge this one. > > Let's please fold it into the other patch. That's cleaner and it makes > logical sense. Understood. > > >> Do we really REALLY need this for 4.11, or can we queue for 4.12 and > >> mark it stable? > > > > Not urgent because one in mainline fixes the problem so I'm okay > > with 4.12 but I don't want mark it as -stable. > > OK good, please resend with the two-line revert in your current > patch, and I'll get it queued up for 4.12. Yeb. If so, now that I think about it, it would be better to handle it via Andrew's tree because Andrew have been handled zram's patches and I have several pending patches based on it. So, I will send new patchset with it to Andrew. Thanks!
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/30/2017 05:45 PM, Minchan Kim wrote: > On Thu, Mar 30, 2017 at 09:35:56AM -0600, Jens Axboe wrote: >> On 03/30/2017 09:08 AM, Minchan Kim wrote: >>> Hi Jens, >>> >>> It seems you miss this. >>> Could you handle this? >> >> I can, but I'm a little confused. The comment talks about replacing >> the one I merged with this one, I can't do that. I'm assuming you >> are talking about this commit: > > Right. > >> >> commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f >> Author: Johannes Thumshirn>> Date: Mon Mar 6 11:23:35 2017 +0100 >> >> zram: set physical queue limits to avoid array out of bounds accesses >> >> which is in mainline. The patch still applies, though. > > You mean it's already in mainline so you cannot replace but can revert. > Right? > If so, please revert it and merge this one. Let's please fold it into the other patch. That's cleaner and it makes logical sense. >> Do we really REALLY need this for 4.11, or can we queue for 4.12 and >> mark it stable? > > Not urgent because one in mainline fixes the problem so I'm okay > with 4.12 but I don't want mark it as -stable. OK good, please resend with the two-line revert in your current patch, and I'll get it queued up for 4.12. -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/30/2017 05:45 PM, Minchan Kim wrote: > On Thu, Mar 30, 2017 at 09:35:56AM -0600, Jens Axboe wrote: >> On 03/30/2017 09:08 AM, Minchan Kim wrote: >>> Hi Jens, >>> >>> It seems you miss this. >>> Could you handle this? >> >> I can, but I'm a little confused. The comment talks about replacing >> the one I merged with this one, I can't do that. I'm assuming you >> are talking about this commit: > > Right. > >> >> commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f >> Author: Johannes Thumshirn >> Date: Mon Mar 6 11:23:35 2017 +0100 >> >> zram: set physical queue limits to avoid array out of bounds accesses >> >> which is in mainline. The patch still applies, though. > > You mean it's already in mainline so you cannot replace but can revert. > Right? > If so, please revert it and merge this one. Let's please fold it into the other patch. That's cleaner and it makes logical sense. >> Do we really REALLY need this for 4.11, or can we queue for 4.12 and >> mark it stable? > > Not urgent because one in mainline fixes the problem so I'm okay > with 4.12 but I don't want mark it as -stable. OK good, please resend with the two-line revert in your current patch, and I'll get it queued up for 4.12. -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Thu, Mar 30, 2017 at 09:35:56AM -0600, Jens Axboe wrote: > On 03/30/2017 09:08 AM, Minchan Kim wrote: > > Hi Jens, > > > > It seems you miss this. > > Could you handle this? > > I can, but I'm a little confused. The comment talks about replacing > the one I merged with this one, I can't do that. I'm assuming you > are talking about this commit: Right. > > commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f > Author: Johannes Thumshirn> Date: Mon Mar 6 11:23:35 2017 +0100 > > zram: set physical queue limits to avoid array out of bounds accesses > > which is in mainline. The patch still applies, though. You mean it's already in mainline so you cannot replace but can revert. Right? If so, please revert it and merge this one. > > Do we really REALLY need this for 4.11, or can we queue for 4.12 and > mark it stable? Not urgent because one in mainline fixes the problem so I'm okay with 4.12 but I don't want mark it as -stable. Thanks!
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Thu, Mar 30, 2017 at 09:35:56AM -0600, Jens Axboe wrote: > On 03/30/2017 09:08 AM, Minchan Kim wrote: > > Hi Jens, > > > > It seems you miss this. > > Could you handle this? > > I can, but I'm a little confused. The comment talks about replacing > the one I merged with this one, I can't do that. I'm assuming you > are talking about this commit: Right. > > commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f > Author: Johannes Thumshirn > Date: Mon Mar 6 11:23:35 2017 +0100 > > zram: set physical queue limits to avoid array out of bounds accesses > > which is in mainline. The patch still applies, though. You mean it's already in mainline so you cannot replace but can revert. Right? If so, please revert it and merge this one. > > Do we really REALLY need this for 4.11, or can we queue for 4.12 and > mark it stable? Not urgent because one in mainline fixes the problem so I'm okay with 4.12 but I don't want mark it as -stable. Thanks!
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/30/2017 09:08 AM, Minchan Kim wrote: > Hi Jens, > > It seems you miss this. > Could you handle this? I can, but I'm a little confused. The comment talks about replacing the one I merged with this one, I can't do that. I'm assuming you are talking about this commit: commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f Author: Johannes ThumshirnDate: Mon Mar 6 11:23:35 2017 +0100 zram: set physical queue limits to avoid array out of bounds accesses which is in mainline. The patch still applies, though. Do we really REALLY need this for 4.11, or can we queue for 4.12 and mark it stable? -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/30/2017 09:08 AM, Minchan Kim wrote: > Hi Jens, > > It seems you miss this. > Could you handle this? I can, but I'm a little confused. The comment talks about replacing the one I merged with this one, I can't do that. I'm assuming you are talking about this commit: commit 0bc315381fe9ed9fb91db8b0e82171b645ac008f Author: Johannes Thumshirn Date: Mon Mar 6 11:23:35 2017 +0100 zram: set physical queue limits to avoid array out of bounds accesses which is in mainline. The patch still applies, though. Do we really REALLY need this for 4.11, or can we queue for 4.12 and mark it stable? -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Jens, It seems you miss this. Could you handle this? Thanks. On Thu, Mar 9, 2017 at 2:28 PM, Minchan Kimwrote: < snip> > Jens, Could you replace the one merged with this? And I don't want > to add stable mark in this patch because I feel it need enough > testing in 64K page system I don't have. ;( > > From bb73e75ab0e21016f60858fd61e7dc6a6813e359 Mon Sep 17 00:00:00 2001 > From: Minchan Kim > Date: Thu, 9 Mar 2017 14:00:40 +0900 > Subject: [PATCH] zram: handle multiple pages attached bio's bvec > > Johannes Thumshirn reported system goes the panic when using NVMe over > Fabrics loopback target with zram. > > The reason is zram expects each bvec in bio contains a single page > but nvme can attach a huge bulk of pages attached to the bio's bvec > so that zram's index arithmetic could be wrong so that out-of-bound > access makes panic. > > This patch solves the problem via removing the limit(a bvec should > contains a only single page). > > Cc: Hannes Reinecke > Reported-by: Johannes Thumshirn > Tested-by: Johannes Thumshirn > Reviewed-by: Johannes Thumshirn > Signed-off-by: Johannes Thumshirn > Signed-off-by: Minchan Kim > --- > I don't add stable mark intentionally because I think it's rather risky > without enough testing on 64K page system(ie, partial IO part). > > Thanks for the help, Johannes and Hannes!! > > drivers/block/zram/zram_drv.c | 37 ++--- > 1 file changed, 10 insertions(+), 27 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 01944419b1f3..fefdf260503a 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -137,8 +137,7 @@ static inline bool valid_io_request(struct zram *zram, > > static void update_position(u32 *index, int *offset, struct bio_vec *bvec) > { > - if (*offset + bvec->bv_len >= PAGE_SIZE) > - (*index)++; > + *index += (*offset + bvec->bv_len) / PAGE_SIZE; > *offset = (*offset + bvec->bv_len) % PAGE_SIZE; > } > > @@ -838,34 +837,20 @@ static void __zram_make_request(struct zram *zram, > struct bio *bio) > } > > bio_for_each_segment(bvec, bio, iter) { > - int max_transfer_size = PAGE_SIZE - offset; > - > - if (bvec.bv_len > max_transfer_size) { > - /* > -* zram_bvec_rw() can only make operation on a single > -* zram page. Split the bio vector. > -*/ > - struct bio_vec bv; > - > - bv.bv_page = bvec.bv_page; > - bv.bv_len = max_transfer_size; > - bv.bv_offset = bvec.bv_offset; > + struct bio_vec bv = bvec; > + unsigned int remained = bvec.bv_len; > > + do { > + bv.bv_len = min_t(unsigned int, PAGE_SIZE, remained); > if (zram_bvec_rw(zram, , index, offset, > -op_is_write(bio_op(bio))) < 0) > + op_is_write(bio_op(bio))) < 0) > goto out; > > - bv.bv_len = bvec.bv_len - max_transfer_size; > - bv.bv_offset += max_transfer_size; > - if (zram_bvec_rw(zram, , index + 1, 0, > -op_is_write(bio_op(bio))) < 0) > - goto out; > - } else > - if (zram_bvec_rw(zram, , index, offset, > -op_is_write(bio_op(bio))) < 0) > - goto out; > + bv.bv_offset += bv.bv_len; > + remained -= bv.bv_len; > > - update_position(, , ); > + update_position(, , ); > + } while (remained); > } > > bio_endio(bio); > @@ -882,8 +867,6 @@ static blk_qc_t zram_make_request(struct request_queue > *queue, struct bio *bio) > { > struct zram *zram = queue->queuedata; > > - blk_queue_split(queue, , queue->bio_split); > - > if (!valid_io_request(zram, bio->bi_iter.bi_sector, > bio->bi_iter.bi_size)) { > atomic64_inc(>stats.invalid_io); > -- > 2.7.4 > > -- Kind regards, Minchan Kim
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Jens, It seems you miss this. Could you handle this? Thanks. On Thu, Mar 9, 2017 at 2:28 PM, Minchan Kim wrote: < snip> > Jens, Could you replace the one merged with this? And I don't want > to add stable mark in this patch because I feel it need enough > testing in 64K page system I don't have. ;( > > From bb73e75ab0e21016f60858fd61e7dc6a6813e359 Mon Sep 17 00:00:00 2001 > From: Minchan Kim > Date: Thu, 9 Mar 2017 14:00:40 +0900 > Subject: [PATCH] zram: handle multiple pages attached bio's bvec > > Johannes Thumshirn reported system goes the panic when using NVMe over > Fabrics loopback target with zram. > > The reason is zram expects each bvec in bio contains a single page > but nvme can attach a huge bulk of pages attached to the bio's bvec > so that zram's index arithmetic could be wrong so that out-of-bound > access makes panic. > > This patch solves the problem via removing the limit(a bvec should > contains a only single page). > > Cc: Hannes Reinecke > Reported-by: Johannes Thumshirn > Tested-by: Johannes Thumshirn > Reviewed-by: Johannes Thumshirn > Signed-off-by: Johannes Thumshirn > Signed-off-by: Minchan Kim > --- > I don't add stable mark intentionally because I think it's rather risky > without enough testing on 64K page system(ie, partial IO part). > > Thanks for the help, Johannes and Hannes!! > > drivers/block/zram/zram_drv.c | 37 ++--- > 1 file changed, 10 insertions(+), 27 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 01944419b1f3..fefdf260503a 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -137,8 +137,7 @@ static inline bool valid_io_request(struct zram *zram, > > static void update_position(u32 *index, int *offset, struct bio_vec *bvec) > { > - if (*offset + bvec->bv_len >= PAGE_SIZE) > - (*index)++; > + *index += (*offset + bvec->bv_len) / PAGE_SIZE; > *offset = (*offset + bvec->bv_len) % PAGE_SIZE; > } > > @@ -838,34 +837,20 @@ static void __zram_make_request(struct zram *zram, > struct bio *bio) > } > > bio_for_each_segment(bvec, bio, iter) { > - int max_transfer_size = PAGE_SIZE - offset; > - > - if (bvec.bv_len > max_transfer_size) { > - /* > -* zram_bvec_rw() can only make operation on a single > -* zram page. Split the bio vector. > -*/ > - struct bio_vec bv; > - > - bv.bv_page = bvec.bv_page; > - bv.bv_len = max_transfer_size; > - bv.bv_offset = bvec.bv_offset; > + struct bio_vec bv = bvec; > + unsigned int remained = bvec.bv_len; > > + do { > + bv.bv_len = min_t(unsigned int, PAGE_SIZE, remained); > if (zram_bvec_rw(zram, , index, offset, > -op_is_write(bio_op(bio))) < 0) > + op_is_write(bio_op(bio))) < 0) > goto out; > > - bv.bv_len = bvec.bv_len - max_transfer_size; > - bv.bv_offset += max_transfer_size; > - if (zram_bvec_rw(zram, , index + 1, 0, > -op_is_write(bio_op(bio))) < 0) > - goto out; > - } else > - if (zram_bvec_rw(zram, , index, offset, > -op_is_write(bio_op(bio))) < 0) > - goto out; > + bv.bv_offset += bv.bv_len; > + remained -= bv.bv_len; > > - update_position(, , ); > + update_position(, , ); > + } while (remained); > } > > bio_endio(bio); > @@ -882,8 +867,6 @@ static blk_qc_t zram_make_request(struct request_queue > *queue, struct bio *bio) > { > struct zram *zram = queue->queuedata; > > - blk_queue_split(queue, , queue->bio_split); > - > if (!valid_io_request(zram, bio->bi_iter.bi_sector, > bio->bi_iter.bi_size)) { > atomic64_inc(>stats.invalid_io); > -- > 2.7.4 > > -- Kind regards, Minchan Kim
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Wed, Mar 08, 2017 at 08:58:02AM +0100, Johannes Thumshirn wrote: > On 03/08/2017 06:11 AM, Minchan Kim wrote: > > And could you test this patch? It avoids split bio so no need new bio > > allocations and makes zram code simple. > > > > From f778d7564d5cd772f25bb181329362c29548a257 Mon Sep 17 00:00:00 2001 > > From: Minchan Kim> > Date: Wed, 8 Mar 2017 13:35:29 +0900 > > Subject: [PATCH] fix > > > > Not-yet-Signed-off-by: Minchan Kim > > --- > > [...] > > Yup, this works here. > > I did a mkfs.xfs /dev/nvme0n1 > dd if=/dev/urandom of=/test.bin bs=1M count=128 > sha256sum test.bin > mount /dev/nvme0n1 /dir > mv test.bin /dir/ > sha256sum /dir/test.bin > > No panics and sha256sum of the 128MB test file still matches > > Tested-by: Johannes Thumshirn > Reviewed-by: Johannes Thumshirn Thanks a lot, Johannes and Hannes!! > > Now that you removed the one page limit in zram_bvec_rw() you can also > add this hunk to remove the queue splitting: Right. I added what you suggested with detailed description. > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 85f4df8..27b168f6 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -868,8 +868,6 @@ static blk_qc_t zram_make_request(struct > request_queue *queue, struct bio *bio) > { > struct zram *zram = queue->queuedata; > > - blk_queue_split(queue, , queue->bio_split); > - > if (!valid_io_request(zram, bio->bi_iter.bi_sector, > bio->bi_iter.bi_size)) { > atomic64_inc(>stats.invalid_io); > > Byte, > Johannes > Jens, Could you replace the one merged with this? And I don't want to add stable mark in this patch because I feel it need enough testing in 64K page system I don't have. ;( >From bb73e75ab0e21016f60858fd61e7dc6a6813e359 Mon Sep 17 00:00:00 2001 From: Minchan Kim Date: Thu, 9 Mar 2017 14:00:40 +0900 Subject: [PATCH] zram: handle multiple pages attached bio's bvec Johannes Thumshirn reported system goes the panic when using NVMe over Fabrics loopback target with zram. The reason is zram expects each bvec in bio contains a single page but nvme can attach a huge bulk of pages attached to the bio's bvec so that zram's index arithmetic could be wrong so that out-of-bound access makes panic. This patch solves the problem via removing the limit(a bvec should contains a only single page). Cc: Hannes Reinecke Reported-by: Johannes Thumshirn Tested-by: Johannes Thumshirn Reviewed-by: Johannes Thumshirn Signed-off-by: Johannes Thumshirn Signed-off-by: Minchan Kim --- I don't add stable mark intentionally because I think it's rather risky without enough testing on 64K page system(ie, partial IO part). Thanks for the help, Johannes and Hannes!! drivers/block/zram/zram_drv.c | 37 ++--- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 01944419b1f3..fefdf260503a 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -137,8 +137,7 @@ static inline bool valid_io_request(struct zram *zram, static void update_position(u32 *index, int *offset, struct bio_vec *bvec) { - if (*offset + bvec->bv_len >= PAGE_SIZE) - (*index)++; + *index += (*offset + bvec->bv_len) / PAGE_SIZE; *offset = (*offset + bvec->bv_len) % PAGE_SIZE; } @@ -838,34 +837,20 @@ static void __zram_make_request(struct zram *zram, struct bio *bio) } bio_for_each_segment(bvec, bio, iter) { - int max_transfer_size = PAGE_SIZE - offset; - - if (bvec.bv_len > max_transfer_size) { - /* -* zram_bvec_rw() can only make operation on a single -* zram page. Split the bio vector. -*/ - struct bio_vec bv; - - bv.bv_page = bvec.bv_page; - bv.bv_len = max_transfer_size; - bv.bv_offset = bvec.bv_offset; + struct bio_vec bv = bvec; + unsigned int remained = bvec.bv_len; + do { + bv.bv_len = min_t(unsigned int, PAGE_SIZE, remained); if (zram_bvec_rw(zram, , index, offset, -op_is_write(bio_op(bio))) < 0) + op_is_write(bio_op(bio))) < 0) goto out; - bv.bv_len = bvec.bv_len - max_transfer_size; - bv.bv_offset += max_transfer_size; - if (zram_bvec_rw(zram, , index + 1, 0, -
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Wed, Mar 08, 2017 at 08:58:02AM +0100, Johannes Thumshirn wrote: > On 03/08/2017 06:11 AM, Minchan Kim wrote: > > And could you test this patch? It avoids split bio so no need new bio > > allocations and makes zram code simple. > > > > From f778d7564d5cd772f25bb181329362c29548a257 Mon Sep 17 00:00:00 2001 > > From: Minchan Kim > > Date: Wed, 8 Mar 2017 13:35:29 +0900 > > Subject: [PATCH] fix > > > > Not-yet-Signed-off-by: Minchan Kim > > --- > > [...] > > Yup, this works here. > > I did a mkfs.xfs /dev/nvme0n1 > dd if=/dev/urandom of=/test.bin bs=1M count=128 > sha256sum test.bin > mount /dev/nvme0n1 /dir > mv test.bin /dir/ > sha256sum /dir/test.bin > > No panics and sha256sum of the 128MB test file still matches > > Tested-by: Johannes Thumshirn > Reviewed-by: Johannes Thumshirn Thanks a lot, Johannes and Hannes!! > > Now that you removed the one page limit in zram_bvec_rw() you can also > add this hunk to remove the queue splitting: Right. I added what you suggested with detailed description. > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 85f4df8..27b168f6 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -868,8 +868,6 @@ static blk_qc_t zram_make_request(struct > request_queue *queue, struct bio *bio) > { > struct zram *zram = queue->queuedata; > > - blk_queue_split(queue, , queue->bio_split); > - > if (!valid_io_request(zram, bio->bi_iter.bi_sector, > bio->bi_iter.bi_size)) { > atomic64_inc(>stats.invalid_io); > > Byte, > Johannes > Jens, Could you replace the one merged with this? And I don't want to add stable mark in this patch because I feel it need enough testing in 64K page system I don't have. ;( >From bb73e75ab0e21016f60858fd61e7dc6a6813e359 Mon Sep 17 00:00:00 2001 From: Minchan Kim Date: Thu, 9 Mar 2017 14:00:40 +0900 Subject: [PATCH] zram: handle multiple pages attached bio's bvec Johannes Thumshirn reported system goes the panic when using NVMe over Fabrics loopback target with zram. The reason is zram expects each bvec in bio contains a single page but nvme can attach a huge bulk of pages attached to the bio's bvec so that zram's index arithmetic could be wrong so that out-of-bound access makes panic. This patch solves the problem via removing the limit(a bvec should contains a only single page). Cc: Hannes Reinecke Reported-by: Johannes Thumshirn Tested-by: Johannes Thumshirn Reviewed-by: Johannes Thumshirn Signed-off-by: Johannes Thumshirn Signed-off-by: Minchan Kim --- I don't add stable mark intentionally because I think it's rather risky without enough testing on 64K page system(ie, partial IO part). Thanks for the help, Johannes and Hannes!! drivers/block/zram/zram_drv.c | 37 ++--- 1 file changed, 10 insertions(+), 27 deletions(-) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 01944419b1f3..fefdf260503a 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -137,8 +137,7 @@ static inline bool valid_io_request(struct zram *zram, static void update_position(u32 *index, int *offset, struct bio_vec *bvec) { - if (*offset + bvec->bv_len >= PAGE_SIZE) - (*index)++; + *index += (*offset + bvec->bv_len) / PAGE_SIZE; *offset = (*offset + bvec->bv_len) % PAGE_SIZE; } @@ -838,34 +837,20 @@ static void __zram_make_request(struct zram *zram, struct bio *bio) } bio_for_each_segment(bvec, bio, iter) { - int max_transfer_size = PAGE_SIZE - offset; - - if (bvec.bv_len > max_transfer_size) { - /* -* zram_bvec_rw() can only make operation on a single -* zram page. Split the bio vector. -*/ - struct bio_vec bv; - - bv.bv_page = bvec.bv_page; - bv.bv_len = max_transfer_size; - bv.bv_offset = bvec.bv_offset; + struct bio_vec bv = bvec; + unsigned int remained = bvec.bv_len; + do { + bv.bv_len = min_t(unsigned int, PAGE_SIZE, remained); if (zram_bvec_rw(zram, , index, offset, -op_is_write(bio_op(bio))) < 0) + op_is_write(bio_op(bio))) < 0) goto out; - bv.bv_len = bvec.bv_len - max_transfer_size; - bv.bv_offset += max_transfer_size; - if (zram_bvec_rw(zram, , index + 1, 0, -op_is_write(bio_op(bio))) < 0) - goto out; - } else - if (zram_bvec_rw(zram, , index, offset, -
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/08/2017 06:11 AM, Minchan Kim wrote: > And could you test this patch? It avoids split bio so no need new bio > allocations and makes zram code simple. > > From f778d7564d5cd772f25bb181329362c29548a257 Mon Sep 17 00:00:00 2001 > From: Minchan Kim> Date: Wed, 8 Mar 2017 13:35:29 +0900 > Subject: [PATCH] fix > > Not-yet-Signed-off-by: Minchan Kim > --- [...] Yup, this works here. I did a mkfs.xfs /dev/nvme0n1 dd if=/dev/urandom of=/test.bin bs=1M count=128 sha256sum test.bin mount /dev/nvme0n1 /dir mv test.bin /dir/ sha256sum /dir/test.bin No panics and sha256sum of the 128MB test file still matches Tested-by: Johannes Thumshirn Reviewed-by: Johannes Thumshirn Now that you removed the one page limit in zram_bvec_rw() you can also add this hunk to remove the queue splitting: diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 85f4df8..27b168f6 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -868,8 +868,6 @@ static blk_qc_t zram_make_request(struct request_queue *queue, struct bio *bio) { struct zram *zram = queue->queuedata; - blk_queue_split(queue, , queue->bio_split); - if (!valid_io_request(zram, bio->bi_iter.bi_sector, bio->bi_iter.bi_size)) { atomic64_inc(>stats.invalid_io); Byte, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/08/2017 06:11 AM, Minchan Kim wrote: > And could you test this patch? It avoids split bio so no need new bio > allocations and makes zram code simple. > > From f778d7564d5cd772f25bb181329362c29548a257 Mon Sep 17 00:00:00 2001 > From: Minchan Kim > Date: Wed, 8 Mar 2017 13:35:29 +0900 > Subject: [PATCH] fix > > Not-yet-Signed-off-by: Minchan Kim > --- [...] Yup, this works here. I did a mkfs.xfs /dev/nvme0n1 dd if=/dev/urandom of=/test.bin bs=1M count=128 sha256sum test.bin mount /dev/nvme0n1 /dir mv test.bin /dir/ sha256sum /dir/test.bin No panics and sha256sum of the 128MB test file still matches Tested-by: Johannes Thumshirn Reviewed-by: Johannes Thumshirn Now that you removed the one page limit in zram_bvec_rw() you can also add this hunk to remove the queue splitting: diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index 85f4df8..27b168f6 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -868,8 +868,6 @@ static blk_qc_t zram_make_request(struct request_queue *queue, struct bio *bio) { struct zram *zram = queue->queuedata; - blk_queue_split(queue, , queue->bio_split); - if (!valid_io_request(zram, bio->bi_iter.bi_sector, bio->bi_iter.bi_size)) { atomic64_inc(>stats.invalid_io); Byte, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Johannes, On Tue, Mar 07, 2017 at 10:51:45AM +0100, Johannes Thumshirn wrote: > On 03/07/2017 09:55 AM, Minchan Kim wrote: > > On Tue, Mar 07, 2017 at 08:48:06AM +0100, Hannes Reinecke wrote: > >> On 03/07/2017 08:23 AM, Minchan Kim wrote: > >>> Hi Hannes, > >>> > >>> On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reineckewrote: > On 03/07/2017 06:22 AM, Minchan Kim wrote: > > Hello Johannes, > > > > On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: > >> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When > >> using > >> the NVMe over Fabrics loopback target which potentially sends a huge > >> bulk of > >> pages attached to the bio's bvec this results in a kernel panic > >> because of > >> array out of bounds accesses in zram_decompress_page(). > > > > First of all, thanks for the report and fix up! > > Unfortunately, I'm not familiar with that interface of block layer. > > > > It seems this is a material for stable so I want to understand it clear. > > Could you say more specific things to educate me? > > > > What scenario/When/How it is problem? It will help for me to > > understand! > > > >>> > >>> Thanks for the quick response! > >>> > The problem is that zram as it currently stands can only handle bios > where each bvec contains a single page (or, to be precise, a chunk of > data with a length of a page). > >>> > >>> Right. > >>> > > This is not an automatic guarantee from the block layer (who is free to > send us bios with arbitrary-sized bvecs), so we need to set the queue > limits to ensure that. > >>> > >>> What does it mean "bios with arbitrary-sized bvecs"? > >>> What kinds of scenario is it used/useful? > >>> > >> Each bio contains a list of bvecs, each of which points to a specific > >> memory area: > >> > >> struct bio_vec { > >>struct page *bv_page; > >>unsigned intbv_len; > >>unsigned intbv_offset; > >> }; > >> > >> The trick now is that while 'bv_page' does point to a page, the memory > >> area pointed to might in fact be contiguous (if several pages are > >> adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ > >> than a page. > > > > Thanks for detail, Hannes! > > > > If I understand it correctly, it seems to be related to bid_add_page > > with high-order page. Right? > > > > If so, I really wonder why I don't see such problem because several > > places have used it and I expected some of them might do IO with > > contiguous pages intentionally or by chance. Hmm, > > > > IIUC, it's not a nvme specific problme but general problem which > > can trigger normal FSes if they uses contiguos pages? > > > > I'm not a FS expert, but a quick grep shows that non of the file-systems > does the > > for_each_sg() > while(bio_add_page())) > > trick NVMe does. Aah, I see. > > >> > >> Hence the check for 'is_partial_io' in zram_drv.c (which just does a > >> test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for > >> partial I/O (ie if the overall length of the bio_vec is _smaller_ than a > >> page), but also for multipage bvecs (where the length of the bio_vec is > >> _larger_ than a page). > > > > Right. I need to look into that. Thanks for the pointing out! > > > >> > >> So rather than fixing the bio scanning loop in zram it's easier to set > >> the queue limits correctly so that 'is_partial_io' does the correct > >> thing and the overall logic in zram doesn't need to be altered. > > > > > > Isn't that approach require new bio allocation through blk_queue_split? > > Maybe, it wouldn't make severe regression in zram-FS workload but need > > to test. > > Yes, but blk_queue_split() needs information how big the bvecs can be, > hence the patch. > > For details have a look into blk_bio_segment_split() in block/blk-merge.c > > It get's the max_sectors from blk_max_size_offset() which is > q->limits.max_sectors when q->limits.chunk_sectors isn't set and > then loops over the bio's bvecs to check when to split the bio and then > calls bio_split() when appropriate. Yeb so it causes split bio which means new bio allocations which was not needed before. > > > > > Is there any ways to trigger the problem without real nvme device? > > It would really help to test/measure zram. > > It isn't a /real/ device but the fabrics loopback target. If you want a > fast reproducible test-case, have a look at: > > https://github.com/ddiss/rapido/ > the cut_nvme_local.sh script set's up the correct VM for this test. Then > a simple mkfs.xfs /dev/nvme0n1 will oops. Thanks! I will look into that. And could you test this patch? It avoids split bio so no need new bio allocations and makes zram code simple. >From f778d7564d5cd772f25bb181329362c29548a257 Mon Sep 17 00:00:00 2001 From: Minchan Kim Date: Wed, 8 Mar 2017 13:35:29 +0900 Subject: [PATCH]
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Johannes, On Tue, Mar 07, 2017 at 10:51:45AM +0100, Johannes Thumshirn wrote: > On 03/07/2017 09:55 AM, Minchan Kim wrote: > > On Tue, Mar 07, 2017 at 08:48:06AM +0100, Hannes Reinecke wrote: > >> On 03/07/2017 08:23 AM, Minchan Kim wrote: > >>> Hi Hannes, > >>> > >>> On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reinecke wrote: > On 03/07/2017 06:22 AM, Minchan Kim wrote: > > Hello Johannes, > > > > On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: > >> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When > >> using > >> the NVMe over Fabrics loopback target which potentially sends a huge > >> bulk of > >> pages attached to the bio's bvec this results in a kernel panic > >> because of > >> array out of bounds accesses in zram_decompress_page(). > > > > First of all, thanks for the report and fix up! > > Unfortunately, I'm not familiar with that interface of block layer. > > > > It seems this is a material for stable so I want to understand it clear. > > Could you say more specific things to educate me? > > > > What scenario/When/How it is problem? It will help for me to > > understand! > > > >>> > >>> Thanks for the quick response! > >>> > The problem is that zram as it currently stands can only handle bios > where each bvec contains a single page (or, to be precise, a chunk of > data with a length of a page). > >>> > >>> Right. > >>> > > This is not an automatic guarantee from the block layer (who is free to > send us bios with arbitrary-sized bvecs), so we need to set the queue > limits to ensure that. > >>> > >>> What does it mean "bios with arbitrary-sized bvecs"? > >>> What kinds of scenario is it used/useful? > >>> > >> Each bio contains a list of bvecs, each of which points to a specific > >> memory area: > >> > >> struct bio_vec { > >>struct page *bv_page; > >>unsigned intbv_len; > >>unsigned intbv_offset; > >> }; > >> > >> The trick now is that while 'bv_page' does point to a page, the memory > >> area pointed to might in fact be contiguous (if several pages are > >> adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ > >> than a page. > > > > Thanks for detail, Hannes! > > > > If I understand it correctly, it seems to be related to bid_add_page > > with high-order page. Right? > > > > If so, I really wonder why I don't see such problem because several > > places have used it and I expected some of them might do IO with > > contiguous pages intentionally or by chance. Hmm, > > > > IIUC, it's not a nvme specific problme but general problem which > > can trigger normal FSes if they uses contiguos pages? > > > > I'm not a FS expert, but a quick grep shows that non of the file-systems > does the > > for_each_sg() > while(bio_add_page())) > > trick NVMe does. Aah, I see. > > >> > >> Hence the check for 'is_partial_io' in zram_drv.c (which just does a > >> test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for > >> partial I/O (ie if the overall length of the bio_vec is _smaller_ than a > >> page), but also for multipage bvecs (where the length of the bio_vec is > >> _larger_ than a page). > > > > Right. I need to look into that. Thanks for the pointing out! > > > >> > >> So rather than fixing the bio scanning loop in zram it's easier to set > >> the queue limits correctly so that 'is_partial_io' does the correct > >> thing and the overall logic in zram doesn't need to be altered. > > > > > > Isn't that approach require new bio allocation through blk_queue_split? > > Maybe, it wouldn't make severe regression in zram-FS workload but need > > to test. > > Yes, but blk_queue_split() needs information how big the bvecs can be, > hence the patch. > > For details have a look into blk_bio_segment_split() in block/blk-merge.c > > It get's the max_sectors from blk_max_size_offset() which is > q->limits.max_sectors when q->limits.chunk_sectors isn't set and > then loops over the bio's bvecs to check when to split the bio and then > calls bio_split() when appropriate. Yeb so it causes split bio which means new bio allocations which was not needed before. > > > > > Is there any ways to trigger the problem without real nvme device? > > It would really help to test/measure zram. > > It isn't a /real/ device but the fabrics loopback target. If you want a > fast reproducible test-case, have a look at: > > https://github.com/ddiss/rapido/ > the cut_nvme_local.sh script set's up the correct VM for this test. Then > a simple mkfs.xfs /dev/nvme0n1 will oops. Thanks! I will look into that. And could you test this patch? It avoids split bio so no need new bio allocations and makes zram code simple. >From f778d7564d5cd772f25bb181329362c29548a257 Mon Sep 17 00:00:00 2001 From: Minchan Kim Date: Wed, 8 Mar 2017 13:35:29 +0900 Subject: [PATCH] fix Not-yet-Signed-off-by:
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/07/2017 08:23 AM, Minchan Kim wrote: > Hi Hannes, > > On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reineckewrote: >> On 03/07/2017 06:22 AM, Minchan Kim wrote: >>> Hello Johannes, >>> >>> On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using the NVMe over Fabrics loopback target which potentially sends a huge bulk of pages attached to the bio's bvec this results in a kernel panic because of array out of bounds accesses in zram_decompress_page(). >>> >>> First of all, thanks for the report and fix up! >>> Unfortunately, I'm not familiar with that interface of block layer. >>> >>> It seems this is a material for stable so I want to understand it clear. >>> Could you say more specific things to educate me? >>> >>> What scenario/When/How it is problem? It will help for me to understand! >>> > > Thanks for the quick response! > >> The problem is that zram as it currently stands can only handle bios >> where each bvec contains a single page (or, to be precise, a chunk of >> data with a length of a page). > > Right. > >> >> This is not an automatic guarantee from the block layer (who is free to >> send us bios with arbitrary-sized bvecs), so we need to set the queue >> limits to ensure that. > > What does it mean "bios with arbitrary-sized bvecs"? > What kinds of scenario is it used/useful? > Each bio contains a list of bvecs, each of which points to a specific memory area: struct bio_vec { struct page *bv_page; unsigned intbv_len; unsigned intbv_offset; }; The trick now is that while 'bv_page' does point to a page, the memory area pointed to might in fact be contiguous (if several pages are adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ than a page. Hence the check for 'is_partial_io' in zram_drv.c (which just does a test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for partial I/O (ie if the overall length of the bio_vec is _smaller_ than a page), but also for multipage bvecs (where the length of the bio_vec is _larger_ than a page). So rather than fixing the bio scanning loop in zram it's easier to set the queue limits correctly so that 'is_partial_io' does the correct thing and the overall logic in zram doesn't need to be altered. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/07/2017 08:23 AM, Minchan Kim wrote: > Hi Hannes, > > On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reinecke wrote: >> On 03/07/2017 06:22 AM, Minchan Kim wrote: >>> Hello Johannes, >>> >>> On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using the NVMe over Fabrics loopback target which potentially sends a huge bulk of pages attached to the bio's bvec this results in a kernel panic because of array out of bounds accesses in zram_decompress_page(). >>> >>> First of all, thanks for the report and fix up! >>> Unfortunately, I'm not familiar with that interface of block layer. >>> >>> It seems this is a material for stable so I want to understand it clear. >>> Could you say more specific things to educate me? >>> >>> What scenario/When/How it is problem? It will help for me to understand! >>> > > Thanks for the quick response! > >> The problem is that zram as it currently stands can only handle bios >> where each bvec contains a single page (or, to be precise, a chunk of >> data with a length of a page). > > Right. > >> >> This is not an automatic guarantee from the block layer (who is free to >> send us bios with arbitrary-sized bvecs), so we need to set the queue >> limits to ensure that. > > What does it mean "bios with arbitrary-sized bvecs"? > What kinds of scenario is it used/useful? > Each bio contains a list of bvecs, each of which points to a specific memory area: struct bio_vec { struct page *bv_page; unsigned intbv_len; unsigned intbv_offset; }; The trick now is that while 'bv_page' does point to a page, the memory area pointed to might in fact be contiguous (if several pages are adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ than a page. Hence the check for 'is_partial_io' in zram_drv.c (which just does a test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for partial I/O (ie if the overall length of the bio_vec is _smaller_ than a page), but also for multipage bvecs (where the length of the bio_vec is _larger_ than a page). So rather than fixing the bio scanning loop in zram it's easier to set the queue limits correctly so that 'is_partial_io' does the correct thing and the overall logic in zram doesn't need to be altered. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/07/2017 09:55 AM, Minchan Kim wrote: > On Tue, Mar 07, 2017 at 08:48:06AM +0100, Hannes Reinecke wrote: >> On 03/07/2017 08:23 AM, Minchan Kim wrote: >>> Hi Hannes, >>> >>> On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reineckewrote: On 03/07/2017 06:22 AM, Minchan Kim wrote: > Hello Johannes, > > On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: >> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When >> using >> the NVMe over Fabrics loopback target which potentially sends a huge >> bulk of >> pages attached to the bio's bvec this results in a kernel panic because >> of >> array out of bounds accesses in zram_decompress_page(). > > First of all, thanks for the report and fix up! > Unfortunately, I'm not familiar with that interface of block layer. > > It seems this is a material for stable so I want to understand it clear. > Could you say more specific things to educate me? > > What scenario/When/How it is problem? It will help for me to understand! > >>> >>> Thanks for the quick response! >>> The problem is that zram as it currently stands can only handle bios where each bvec contains a single page (or, to be precise, a chunk of data with a length of a page). >>> >>> Right. >>> This is not an automatic guarantee from the block layer (who is free to send us bios with arbitrary-sized bvecs), so we need to set the queue limits to ensure that. >>> >>> What does it mean "bios with arbitrary-sized bvecs"? >>> What kinds of scenario is it used/useful? >>> >> Each bio contains a list of bvecs, each of which points to a specific >> memory area: >> >> struct bio_vec { >> struct page *bv_page; >> unsigned intbv_len; >> unsigned intbv_offset; >> }; >> >> The trick now is that while 'bv_page' does point to a page, the memory >> area pointed to might in fact be contiguous (if several pages are >> adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ >> than a page. > > Thanks for detail, Hannes! > > If I understand it correctly, it seems to be related to bid_add_page > with high-order page. Right? > > If so, I really wonder why I don't see such problem because several > places have used it and I expected some of them might do IO with > contiguous pages intentionally or by chance. Hmm, > > IIUC, it's not a nvme specific problme but general problem which > can trigger normal FSes if they uses contiguos pages? > I'm not a FS expert, but a quick grep shows that non of the file-systems does the for_each_sg() while(bio_add_page())) trick NVMe does. >> >> Hence the check for 'is_partial_io' in zram_drv.c (which just does a >> test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for >> partial I/O (ie if the overall length of the bio_vec is _smaller_ than a >> page), but also for multipage bvecs (where the length of the bio_vec is >> _larger_ than a page). > > Right. I need to look into that. Thanks for the pointing out! > >> >> So rather than fixing the bio scanning loop in zram it's easier to set >> the queue limits correctly so that 'is_partial_io' does the correct >> thing and the overall logic in zram doesn't need to be altered. > > > Isn't that approach require new bio allocation through blk_queue_split? > Maybe, it wouldn't make severe regression in zram-FS workload but need > to test. Yes, but blk_queue_split() needs information how big the bvecs can be, hence the patch. For details have a look into blk_bio_segment_split() in block/blk-merge.c It get's the max_sectors from blk_max_size_offset() which is q->limits.max_sectors when q->limits.chunk_sectors isn't set and then loops over the bio's bvecs to check when to split the bio and then calls bio_split() when appropriate. > > Is there any ways to trigger the problem without real nvme device? > It would really help to test/measure zram. It isn't a /real/ device but the fabrics loopback target. If you want a fast reproducible test-case, have a look at: https://github.com/ddiss/rapido/ the cut_nvme_local.sh script set's up the correct VM for this test. Then a simple mkfs.xfs /dev/nvme0n1 will oops. > > Anyway, to me, it's really subtle at this moment so I doubt it should > be stable material. :( I'm not quite sure, it's at least 4.11 material. See above. Thanks, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/07/2017 09:55 AM, Minchan Kim wrote: > On Tue, Mar 07, 2017 at 08:48:06AM +0100, Hannes Reinecke wrote: >> On 03/07/2017 08:23 AM, Minchan Kim wrote: >>> Hi Hannes, >>> >>> On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reinecke wrote: On 03/07/2017 06:22 AM, Minchan Kim wrote: > Hello Johannes, > > On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: >> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When >> using >> the NVMe over Fabrics loopback target which potentially sends a huge >> bulk of >> pages attached to the bio's bvec this results in a kernel panic because >> of >> array out of bounds accesses in zram_decompress_page(). > > First of all, thanks for the report and fix up! > Unfortunately, I'm not familiar with that interface of block layer. > > It seems this is a material for stable so I want to understand it clear. > Could you say more specific things to educate me? > > What scenario/When/How it is problem? It will help for me to understand! > >>> >>> Thanks for the quick response! >>> The problem is that zram as it currently stands can only handle bios where each bvec contains a single page (or, to be precise, a chunk of data with a length of a page). >>> >>> Right. >>> This is not an automatic guarantee from the block layer (who is free to send us bios with arbitrary-sized bvecs), so we need to set the queue limits to ensure that. >>> >>> What does it mean "bios with arbitrary-sized bvecs"? >>> What kinds of scenario is it used/useful? >>> >> Each bio contains a list of bvecs, each of which points to a specific >> memory area: >> >> struct bio_vec { >> struct page *bv_page; >> unsigned intbv_len; >> unsigned intbv_offset; >> }; >> >> The trick now is that while 'bv_page' does point to a page, the memory >> area pointed to might in fact be contiguous (if several pages are >> adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ >> than a page. > > Thanks for detail, Hannes! > > If I understand it correctly, it seems to be related to bid_add_page > with high-order page. Right? > > If so, I really wonder why I don't see such problem because several > places have used it and I expected some of them might do IO with > contiguous pages intentionally or by chance. Hmm, > > IIUC, it's not a nvme specific problme but general problem which > can trigger normal FSes if they uses contiguos pages? > I'm not a FS expert, but a quick grep shows that non of the file-systems does the for_each_sg() while(bio_add_page())) trick NVMe does. >> >> Hence the check for 'is_partial_io' in zram_drv.c (which just does a >> test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for >> partial I/O (ie if the overall length of the bio_vec is _smaller_ than a >> page), but also for multipage bvecs (where the length of the bio_vec is >> _larger_ than a page). > > Right. I need to look into that. Thanks for the pointing out! > >> >> So rather than fixing the bio scanning loop in zram it's easier to set >> the queue limits correctly so that 'is_partial_io' does the correct >> thing and the overall logic in zram doesn't need to be altered. > > > Isn't that approach require new bio allocation through blk_queue_split? > Maybe, it wouldn't make severe regression in zram-FS workload but need > to test. Yes, but blk_queue_split() needs information how big the bvecs can be, hence the patch. For details have a look into blk_bio_segment_split() in block/blk-merge.c It get's the max_sectors from blk_max_size_offset() which is q->limits.max_sectors when q->limits.chunk_sectors isn't set and then loops over the bio's bvecs to check when to split the bio and then calls bio_split() when appropriate. > > Is there any ways to trigger the problem without real nvme device? > It would really help to test/measure zram. It isn't a /real/ device but the fabrics loopback target. If you want a fast reproducible test-case, have a look at: https://github.com/ddiss/rapido/ the cut_nvme_local.sh script set's up the correct VM for this test. Then a simple mkfs.xfs /dev/nvme0n1 will oops. > > Anyway, to me, it's really subtle at this moment so I doubt it should > be stable material. :( I'm not quite sure, it's at least 4.11 material. See above. Thanks, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Tue, Mar 07, 2017 at 08:48:06AM +0100, Hannes Reinecke wrote: > On 03/07/2017 08:23 AM, Minchan Kim wrote: > > Hi Hannes, > > > > On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reineckewrote: > >> On 03/07/2017 06:22 AM, Minchan Kim wrote: > >>> Hello Johannes, > >>> > >>> On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When > using > the NVMe over Fabrics loopback target which potentially sends a huge > bulk of > pages attached to the bio's bvec this results in a kernel panic because > of > array out of bounds accesses in zram_decompress_page(). > >>> > >>> First of all, thanks for the report and fix up! > >>> Unfortunately, I'm not familiar with that interface of block layer. > >>> > >>> It seems this is a material for stable so I want to understand it clear. > >>> Could you say more specific things to educate me? > >>> > >>> What scenario/When/How it is problem? It will help for me to understand! > >>> > > > > Thanks for the quick response! > > > >> The problem is that zram as it currently stands can only handle bios > >> where each bvec contains a single page (or, to be precise, a chunk of > >> data with a length of a page). > > > > Right. > > > >> > >> This is not an automatic guarantee from the block layer (who is free to > >> send us bios with arbitrary-sized bvecs), so we need to set the queue > >> limits to ensure that. > > > > What does it mean "bios with arbitrary-sized bvecs"? > > What kinds of scenario is it used/useful? > > > Each bio contains a list of bvecs, each of which points to a specific > memory area: > > struct bio_vec { > struct page *bv_page; > unsigned intbv_len; > unsigned intbv_offset; > }; > > The trick now is that while 'bv_page' does point to a page, the memory > area pointed to might in fact be contiguous (if several pages are > adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ > than a page. Thanks for detail, Hannes! If I understand it correctly, it seems to be related to bid_add_page with high-order page. Right? If so, I really wonder why I don't see such problem because several places have used it and I expected some of them might do IO with contiguous pages intentionally or by chance. Hmm, IIUC, it's not a nvme specific problme but general problem which can trigger normal FSes if they uses contiguos pages? > > Hence the check for 'is_partial_io' in zram_drv.c (which just does a > test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for > partial I/O (ie if the overall length of the bio_vec is _smaller_ than a > page), but also for multipage bvecs (where the length of the bio_vec is > _larger_ than a page). Right. I need to look into that. Thanks for the pointing out! > > So rather than fixing the bio scanning loop in zram it's easier to set > the queue limits correctly so that 'is_partial_io' does the correct > thing and the overall logic in zram doesn't need to be altered. Isn't that approach require new bio allocation through blk_queue_split? Maybe, it wouldn't make severe regression in zram-FS workload but need to test. Is there any ways to trigger the problem without real nvme device? It would really help to test/measure zram. Anyway, to me, it's really subtle at this moment so I doubt it should be stable material. :(
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Tue, Mar 07, 2017 at 08:48:06AM +0100, Hannes Reinecke wrote: > On 03/07/2017 08:23 AM, Minchan Kim wrote: > > Hi Hannes, > > > > On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reinecke wrote: > >> On 03/07/2017 06:22 AM, Minchan Kim wrote: > >>> Hello Johannes, > >>> > >>> On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When > using > the NVMe over Fabrics loopback target which potentially sends a huge > bulk of > pages attached to the bio's bvec this results in a kernel panic because > of > array out of bounds accesses in zram_decompress_page(). > >>> > >>> First of all, thanks for the report and fix up! > >>> Unfortunately, I'm not familiar with that interface of block layer. > >>> > >>> It seems this is a material for stable so I want to understand it clear. > >>> Could you say more specific things to educate me? > >>> > >>> What scenario/When/How it is problem? It will help for me to understand! > >>> > > > > Thanks for the quick response! > > > >> The problem is that zram as it currently stands can only handle bios > >> where each bvec contains a single page (or, to be precise, a chunk of > >> data with a length of a page). > > > > Right. > > > >> > >> This is not an automatic guarantee from the block layer (who is free to > >> send us bios with arbitrary-sized bvecs), so we need to set the queue > >> limits to ensure that. > > > > What does it mean "bios with arbitrary-sized bvecs"? > > What kinds of scenario is it used/useful? > > > Each bio contains a list of bvecs, each of which points to a specific > memory area: > > struct bio_vec { > struct page *bv_page; > unsigned intbv_len; > unsigned intbv_offset; > }; > > The trick now is that while 'bv_page' does point to a page, the memory > area pointed to might in fact be contiguous (if several pages are > adjacent). Hence we might be getting a bio_vec where bv_len is _larger_ > than a page. Thanks for detail, Hannes! If I understand it correctly, it seems to be related to bid_add_page with high-order page. Right? If so, I really wonder why I don't see such problem because several places have used it and I expected some of them might do IO with contiguous pages intentionally or by chance. Hmm, IIUC, it's not a nvme specific problme but general problem which can trigger normal FSes if they uses contiguos pages? > > Hence the check for 'is_partial_io' in zram_drv.c (which just does a > test 'if bv_len != PAGE_SIZE) is in fact wrong, as it would trigger for > partial I/O (ie if the overall length of the bio_vec is _smaller_ than a > page), but also for multipage bvecs (where the length of the bio_vec is > _larger_ than a page). Right. I need to look into that. Thanks for the pointing out! > > So rather than fixing the bio scanning loop in zram it's easier to set > the queue limits correctly so that 'is_partial_io' does the correct > thing and the overall logic in zram doesn't need to be altered. Isn't that approach require new bio allocation through blk_queue_split? Maybe, it wouldn't make severe regression in zram-FS workload but need to test. Is there any ways to trigger the problem without real nvme device? It would really help to test/measure zram. Anyway, to me, it's really subtle at this moment so I doubt it should be stable material. :(
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/07/2017 06:22 AM, Minchan Kim wrote: > Hello Johannes, > > On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: >> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using >> the NVMe over Fabrics loopback target which potentially sends a huge bulk of >> pages attached to the bio's bvec this results in a kernel panic because of >> array out of bounds accesses in zram_decompress_page(). > > First of all, thanks for the report and fix up! > Unfortunately, I'm not familiar with that interface of block layer. > > It seems this is a material for stable so I want to understand it clear. > Could you say more specific things to educate me? > > What scenario/When/How it is problem? It will help for me to understand! > The problem is that zram as it currently stands can only handle bios where each bvec contains a single page (or, to be precise, a chunk of data with a length of a page). This is not an automatic guarantee from the block layer (who is free to send us bios with arbitrary-sized bvecs), so we need to set the queue limits to ensure that. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/07/2017 06:22 AM, Minchan Kim wrote: > Hello Johannes, > > On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: >> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using >> the NVMe over Fabrics loopback target which potentially sends a huge bulk of >> pages attached to the bio's bvec this results in a kernel panic because of >> array out of bounds accesses in zram_decompress_page(). > > First of all, thanks for the report and fix up! > Unfortunately, I'm not familiar with that interface of block layer. > > It seems this is a material for stable so I want to understand it clear. > Could you say more specific things to educate me? > > What scenario/When/How it is problem? It will help for me to understand! > The problem is that zram as it currently stands can only handle bios where each bvec contains a single page (or, to be precise, a chunk of data with a length of a page). This is not an automatic guarantee from the block layer (who is free to send us bios with arbitrary-sized bvecs), so we need to set the queue limits to ensure that. Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Hannes, On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reineckewrote: > On 03/07/2017 06:22 AM, Minchan Kim wrote: >> Hello Johannes, >> >> On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: >>> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using >>> the NVMe over Fabrics loopback target which potentially sends a huge bulk of >>> pages attached to the bio's bvec this results in a kernel panic because of >>> array out of bounds accesses in zram_decompress_page(). >> >> First of all, thanks for the report and fix up! >> Unfortunately, I'm not familiar with that interface of block layer. >> >> It seems this is a material for stable so I want to understand it clear. >> Could you say more specific things to educate me? >> >> What scenario/When/How it is problem? It will help for me to understand! >> Thanks for the quick response! > The problem is that zram as it currently stands can only handle bios > where each bvec contains a single page (or, to be precise, a chunk of > data with a length of a page). Right. > > This is not an automatic guarantee from the block layer (who is free to > send us bios with arbitrary-sized bvecs), so we need to set the queue > limits to ensure that. What does it mean "bios with arbitrary-sized bvecs"? What kinds of scenario is it used/useful? And how can we solve it by setting queue limit? Sorry for the many questions due to limited knowledge. Thanks.
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hi Hannes, On Tue, Mar 7, 2017 at 4:00 PM, Hannes Reinecke wrote: > On 03/07/2017 06:22 AM, Minchan Kim wrote: >> Hello Johannes, >> >> On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: >>> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using >>> the NVMe over Fabrics loopback target which potentially sends a huge bulk of >>> pages attached to the bio's bvec this results in a kernel panic because of >>> array out of bounds accesses in zram_decompress_page(). >> >> First of all, thanks for the report and fix up! >> Unfortunately, I'm not familiar with that interface of block layer. >> >> It seems this is a material for stable so I want to understand it clear. >> Could you say more specific things to educate me? >> >> What scenario/When/How it is problem? It will help for me to understand! >> Thanks for the quick response! > The problem is that zram as it currently stands can only handle bios > where each bvec contains a single page (or, to be precise, a chunk of > data with a length of a page). Right. > > This is not an automatic guarantee from the block layer (who is free to > send us bios with arbitrary-sized bvecs), so we need to set the queue > limits to ensure that. What does it mean "bios with arbitrary-sized bvecs"? What kinds of scenario is it used/useful? And how can we solve it by setting queue limit? Sorry for the many questions due to limited knowledge. Thanks.
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hello Johannes, On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). First of all, thanks for the report and fix up! Unfortunately, I'm not familiar with that interface of block layer. It seems this is a material for stable so I want to understand it clear. Could you say more specific things to educate me? What scenario/When/How it is problem? It will help for me to understand! Thanks. > > Signed-off-by: Johannes Thumshirn> --- > drivers/block/zram/zram_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index e27d89a..dceb5ed 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -1189,6 +1189,8 @@ static int zram_add(void) > blk_queue_io_min(zram->disk->queue, PAGE_SIZE); > blk_queue_io_opt(zram->disk->queue, PAGE_SIZE); > zram->disk->queue->limits.discard_granularity = PAGE_SIZE; > + zram->disk->queue->limits.max_sectors = SECTORS_PER_PAGE; > + zram->disk->queue->limits.chunk_sectors = 0; > blk_queue_max_discard_sectors(zram->disk->queue, UINT_MAX); > /* >* zram_bio_discard() will clear all logical blocks if logical block > -- > 1.8.5.6 >
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
Hello Johannes, On Mon, Mar 06, 2017 at 11:23:35AM +0100, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). First of all, thanks for the report and fix up! Unfortunately, I'm not familiar with that interface of block layer. It seems this is a material for stable so I want to understand it clear. Could you say more specific things to educate me? What scenario/When/How it is problem? It will help for me to understand! Thanks. > > Signed-off-by: Johannes Thumshirn > --- > drivers/block/zram/zram_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index e27d89a..dceb5ed 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -1189,6 +1189,8 @@ static int zram_add(void) > blk_queue_io_min(zram->disk->queue, PAGE_SIZE); > blk_queue_io_opt(zram->disk->queue, PAGE_SIZE); > zram->disk->queue->limits.discard_granularity = PAGE_SIZE; > + zram->disk->queue->limits.max_sectors = SECTORS_PER_PAGE; > + zram->disk->queue->limits.chunk_sectors = 0; > blk_queue_max_discard_sectors(zram->disk->queue, UINT_MAX); > /* >* zram_bio_discard() will clear all logical blocks if logical block > -- > 1.8.5.6 >
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/06/2017 01:18 PM, Andrew Morton wrote: > On Mon, 6 Mar 2017 08:21:11 -0700 Jens Axboewrote: > >> On 03/06/2017 03:23 AM, Johannes Thumshirn wrote: >>> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using >>> the NVMe over Fabrics loopback target which potentially sends a huge bulk of >>> pages attached to the bio's bvec this results in a kernel panic because of >>> array out of bounds accesses in zram_decompress_page(). >> >> Applied, thanks. > > With an added cc:stable, hopefully? I didn't. But I can email it to stable@ when it lands in Linus's tree. -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/06/2017 01:18 PM, Andrew Morton wrote: > On Mon, 6 Mar 2017 08:21:11 -0700 Jens Axboe wrote: > >> On 03/06/2017 03:23 AM, Johannes Thumshirn wrote: >>> zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using >>> the NVMe over Fabrics loopback target which potentially sends a huge bulk of >>> pages attached to the bio's bvec this results in a kernel panic because of >>> array out of bounds accesses in zram_decompress_page(). >> >> Applied, thanks. > > With an added cc:stable, hopefully? I didn't. But I can email it to stable@ when it lands in Linus's tree. -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Mon, 6 Mar 2017 08:21:11 -0700 Jens Axboewrote: > On 03/06/2017 03:23 AM, Johannes Thumshirn wrote: > > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > > pages attached to the bio's bvec this results in a kernel panic because of > > array out of bounds accesses in zram_decompress_page(). > > Applied, thanks. With an added cc:stable, hopefully?
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On Mon, 6 Mar 2017 08:21:11 -0700 Jens Axboe wrote: > On 03/06/2017 03:23 AM, Johannes Thumshirn wrote: > > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > > pages attached to the bio's bvec this results in a kernel panic because of > > array out of bounds accesses in zram_decompress_page(). > > Applied, thanks. With an added cc:stable, hopefully?
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/06/2017 03:23 AM, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). Applied, thanks. -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/06/2017 03:23 AM, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). Applied, thanks. -- Jens Axboe
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On (03/06/17 11:23), Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). > > Signed-off-by: Johannes ThumshirnCc Andrew Link: lkml.kernel.org/r/20170306102335.9180-1-jthumsh...@suse.de Reviewed-by: Sergey Senozhatsky thanks! -ss
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On (03/06/17 11:23), Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). > > Signed-off-by: Johannes Thumshirn Cc Andrew Link: lkml.kernel.org/r/20170306102335.9180-1-jthumsh...@suse.de Reviewed-by: Sergey Senozhatsky thanks! -ss
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/06/2017 11:23 AM, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). > > Signed-off-by: Johannes Thumshirn> --- > drivers/block/zram/zram_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCH] zram: set physical queue limits to avoid array out of bounds accesses
On 03/06/2017 11:23 AM, Johannes Thumshirn wrote: > zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using > the NVMe over Fabrics loopback target which potentially sends a huge bulk of > pages attached to the bio's bvec this results in a kernel panic because of > array out of bounds accesses in zram_decompress_page(). > > Signed-off-by: Johannes Thumshirn > --- > drivers/block/zram/zram_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckeTeamlead Storage & Networking h...@suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
[PATCH] zram: set physical queue limits to avoid array out of bounds accesses
zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using the NVMe over Fabrics loopback target which potentially sends a huge bulk of pages attached to the bio's bvec this results in a kernel panic because of array out of bounds accesses in zram_decompress_page(). Signed-off-by: Johannes Thumshirn--- drivers/block/zram/zram_drv.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index e27d89a..dceb5ed 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1189,6 +1189,8 @@ static int zram_add(void) blk_queue_io_min(zram->disk->queue, PAGE_SIZE); blk_queue_io_opt(zram->disk->queue, PAGE_SIZE); zram->disk->queue->limits.discard_granularity = PAGE_SIZE; + zram->disk->queue->limits.max_sectors = SECTORS_PER_PAGE; + zram->disk->queue->limits.chunk_sectors = 0; blk_queue_max_discard_sectors(zram->disk->queue, UINT_MAX); /* * zram_bio_discard() will clear all logical blocks if logical block -- 1.8.5.6
[PATCH] zram: set physical queue limits to avoid array out of bounds accesses
zram can handle at most SECTORS_PER_PAGE sectors in a bio's bvec. When using the NVMe over Fabrics loopback target which potentially sends a huge bulk of pages attached to the bio's bvec this results in a kernel panic because of array out of bounds accesses in zram_decompress_page(). Signed-off-by: Johannes Thumshirn --- drivers/block/zram/zram_drv.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index e27d89a..dceb5ed 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -1189,6 +1189,8 @@ static int zram_add(void) blk_queue_io_min(zram->disk->queue, PAGE_SIZE); blk_queue_io_opt(zram->disk->queue, PAGE_SIZE); zram->disk->queue->limits.discard_granularity = PAGE_SIZE; + zram->disk->queue->limits.max_sectors = SECTORS_PER_PAGE; + zram->disk->queue->limits.chunk_sectors = 0; blk_queue_max_discard_sectors(zram->disk->queue, UINT_MAX); /* * zram_bio_discard() will clear all logical blocks if logical block -- 1.8.5.6