On 2018 Mai 17, Borislav Petkov wrote: > On Tue, May 15, 2018 at 11:39:54AM +0200, Johannes Hirte wrote: > > The out-of-bound access happens in get_block_address: > > > > if (bankp && bankp->blocks) { > > struct threshold_block *blockp blockp = &bankp->blocks[block]; > > > > with block=1. This doesn't exists. I don't even find any array here. > > There is a linked list, created in allocate_threshold_blocks. On my > > system I get 17 lists with one element each. > > Yes, what a mess this is. ;-\ > > There's no such thing as ->blocks[block] array. We assign simply the > threshold_block to it in allocate_threshold_blocks: > > per_cpu(threshold_banks, cpu)[bank]->blocks = b; > > And I can't say the design of this thing is really friendly but it is > still no excuse that I missed that during review. Grrr. > > So, Yazen, what really needs to happen here is to iterate the > bank->blocks->miscj list to find the block you're looking for and return > its address, the opposite to this here: > > if (per_cpu(threshold_banks, cpu)[bank]->blocks) { > list_add(&b->miscj, > &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj); > } else { > per_cpu(threshold_banks, cpu)[bank]->blocks = b; > } > > and don't forget to look at ->blocks itself. > > And then you need to make sure that searching for block addresses still > works when resuming from suspend so that you can avoid the RDMSR IPIs. >
Maybe I'm missing something, but those RDMSR IPSs don't happen on pre-SMCA systems, right? So the caching should be avoided here, cause the whole lookup looks more expensive to me than the simple switch-block in get_block_address. -- Regards, Johannes