Hi folks,
Please hold on, this patch fails on a large size read test case this morning. I will post a new version after the issue fixed.
Thanks.
Coly Li
On 5/18/21 12:22 AM, colyli@suse.de wrote:
From: Coly Li colyli@suse.de
In the cache missing code path of cached device, if a proper location from the internal B+ tree is matched for a cache miss range, function cached_dev_cache_miss() will be called in cache_lookup_fn() in the following code block, [code block 1] 526 unsigned int sectors = KEY_INODE(k) == s->iop.inode 527 ? min_t(uint64_t, INT_MAX, 528 KEY_START(k) - bio->bi_iter.bi_sector) 529 : INT_MAX; 530 int ret = s->d->cache_miss(b, s, bio, sectors);
Here s->d->cache_miss() is the call backfunction pointer initialized as cached_dev_cache_miss(), the last parameter 'sectors' is an important hint to calculate the size of read request to backing device of the missing cache data.
Current calculation in above code block may generate oversized value of 'sectors', which consequently may trigger 2 different potential kernel panics by BUG() or BUG_ON() as listed below,
- BUG_ON() inside bch_btree_insert_key(),
[code block 2] 886 BUG_ON(b->ops->is_extents && !KEY_SIZE(k)); 2) BUG() inside biovec_slab(), [code block 3] 51 default: 52 BUG(); 53 return NULL;
All the above panics are original from cached_dev_cache_miss() by the oversized parameter 'sectors'.
Inside cached_dev_cache_miss(), parameter 'sectors' is used to calculate the size of data read from backing device for the cache missing. This size is stored in s->insert_bio_sectors by the following lines of code, [code block 4] 909 s->insert_bio_sectors = min(sectors, bio_sectors(bio) + reada);
Then the actual key inserting to the internal B+ tree is generated and stored in s->iop.replace_key by the following lines of code, [code block 5] 911 s->iop.replace_key = KEY(s->iop.inode, 912 bio->bi_iter.bi_sector + s->insert_bio_sectors, 913 s->insert_bio_sectors); The oversized parameter 'sectors' may trigger panic 1) by BUG_ON() from the above code block.
And the bio sending to backing device for the missing data is allocated with hint from s->insert_bio_sectors by the following lines of code, [code block 6] 926 cache_bio = bio_alloc_bioset(GFP_NOWAIT, 927 DIV_ROUND_UP(s->insert_bio_sectors, PAGE_SECTORS), 928 &dc->disk.bio_split); The oversized parameter 'sectors' may trigger panic 2) by BUG() from the agove code block.
Now let me explain how the panics happen with the oversized 'sectors'. In code block 5, replace_key is generated by macro KEY(). From the definition of macro KEY(), [code block 7] 71 #define KEY(inode, offset, size) \ 72 ((struct bkey) { \ 73 .high = (1ULL << 63) | ((__u64) (size) << 20) | (inode), \ 74 .low = (offset) \ 75 })
Here 'size' is 16bits width embedded in 64bits member 'high' of struct bkey. But in code block 1, if "KEY_START(k) - bio->bi_iter.bi_sector" is very probably to be larger than (1<<16) - 1, which makes the bkey size calculation in code block 5 is overflowed. In one bug report the value of parameter 'sectors' is 131072 (= 1 << 17), the overflowed 'sectors' results the overflowed s->insert_bio_sectors in code block 4, then makes size field of s->iop.replace_key to be 0 in code block 5. Then the 0- sized s->iop.replace_key is inserted into the internal B+ tree as cache missing check key (a special key to detect and avoid a racing between normal write request and cache missing read request) as, [code block 8] 915 ret = bch_btree_insert_check_key(b, &s->op, &s->iop.replace_key);
Then the 0-sized s->iop.replace_key as 3rd parameter triggers the bkey size check BUG_ON() in code block 2, and causes the kernel panic 1).
Another kernel panic is from code block 6, is from the oversized value s->insert_bio_sectors resulted by the oversized 'sectors'. From a bug report the result of "DIV_ROUND_UP(s->insert_bio_sectors, PAGE_SECTORS)" from code block 6 can be 344, 282, 946, 342 and many other values which larther than BIO_MAX_VECS (a.k.a 256). When calling bio_alloc_bioset() with such larger-than-256 value as the 2nd parameter, this value will eventually be sent to biovec_slab() as parameter 'nr_vecs' in following code path, bio_alloc_bioset() ==> bvec_alloc() ==> biovec_slab()
Because parameter 'nr_vecs' is larger-than-256 value, the panic by BUG() in code block 3 is triggered inside biovec_slab().
From the above analysis, it is obvious that in order to avoid the kernel panics in code block 2 and code block 3, the calculated 'sectors' in code block 1 should not generate overflowed value in code block 5 and code block 6.
To avoid overflow in code block 5, the maximum 'sectors' value should be equal or less than (1 << KEY_SIZE_BITS) - 1. And to avoid overflow in code block 6, the maximum 'sectors' value should be euqal or less than BIO_MAX_VECS * PAGE_SECTORS. Considering the kernel page size can be variable, a reasonable maximum limitation of 'sectors' in code block 1 should be smaller on from "(1 << KEY_SIZE_BITS) - 1" and "BIO_MAX_VECS * PAGE_SECTORS".
In this patch, a local variable inside cache_lookup_fn() is added as, max_cache_miss_size = min_t(uint32_t, (1 << KEY_SIZE_BITS) - 1, BIO_MAX_VECS * PAGE_SECTORS); Then code block 1 uses max_cache_miss_size to limit the maximum value of 'sectors' calculation as, 533 unsigned int sectors = KEY_INODE(k) == s->iop.inode 534 ? min_t(uint64_t, max_cache_miss_size, 535 KEY_START(k) - bio->bi_iter.bi_sector) 536 : max_cache_miss_size;
Now the calculated 'sectors' value sent into cached_dev_cache_miss() won't trigger neither of the above kernel panics.
Current problmatic code can be partially found since Linux v5.13-rc1, therefore all maintained stable kernels should try to apply this fix.
Reported-by: Diego Ercolani diego.ercolani@gmail.com Reported-by: Jan Szubiak jan.szubiak@linuxpolska.pl Reported-by: Marco Rebhan me@dblsaiko.net Reported-by: Matthias Ferdinand bcache@mfedv.net Reported-by: Thorsten Knabe linux@thorsten-knabe.de Reported-by: Victor Westerhuis victor@westerhu.is Reported-by: Vojtech Pavlik vojtech@suse.cz Signed-off-by: Coly Li colyli@suse.de Cc: stable@vger.kernel.org Cc: Takashi Iwai tiwai@suse.com Cc: Kent Overstreet kent.overstreet@gmail.com
drivers/md/bcache/request.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c index 29c231758293..90fa9ac47661 100644 --- a/drivers/md/bcache/request.c +++ b/drivers/md/bcache/request.c @@ -515,18 +515,25 @@ static int cache_lookup_fn(struct btree_op *op, struct btree *b, struct bkey *k) struct search *s = container_of(op, struct search, op); struct bio *n, *bio = &s->bio.bio; struct bkey *bio_key;
- unsigned int ptr;
- unsigned int ptr, max_cache_miss_size;
if (bkey_cmp(k, &KEY(s->iop.inode, bio->bi_iter.bi_sector, 0)) <= 0) return MAP_CONTINUE;
- /*
* Make sure the cache missing size won't exceed the restrictions of* max bkey size and max bio's bi_max_vecs.*/- max_cache_miss_size = min_t(uint32_t,
(1 << KEY_SIZE_BITS) - 1, BIO_MAX_VECS * PAGE_SECTORS);- if (KEY_INODE(k) != s->iop.inode || KEY_START(k) > bio->bi_iter.bi_sector) { unsigned int bio_sectors = bio_sectors(bio); unsigned int sectors = KEY_INODE(k) == s->iop.inode
? min_t(uint64_t, INT_MAX,
? min_t(uint64_t, max_cache_miss_size, KEY_START(k) - bio->bi_iter.bi_sector)
: INT_MAX;
: max_cache_miss_size;if (ret != MAP_CONTINUE) @@ -547,7 +554,7 @@ static int cache_lookup_fn(struct btree_op *op, struct btree *b, struct bkey *k) if (KEY_DIRTY(k)) s->read_dirty_data = true;
- n = bio_next_split(bio, min_t(uint64_t, INT_MAX,
- n = bio_next_split(bio, min_t(uint64_t, max_cache_miss_size, KEY_OFFSET(k) - bio->bi_iter.bi_sector), GFP_NOIO, &s->d->bio_split);