Re: x86/mm/pageattr: Code without effect?

From: Stefan Bader
Date: Mon Apr 08 2013 - 10:51:32 EST


On 08.04.2013 16:15, Borislav Petkov wrote:
> On Mon, Apr 08, 2013 at 03:10:00PM +0200, Stefan Bader wrote:
>> * that we limited the number of possible pages already to
>> * the number of pages in the large page.
>> */
>> if (address == (address & pmask) && cpa->numpages == (psize >>
>> PAGE_SHIFT)) {
>> /*
>> * The address is aligned and the number of pages
>> * covers the full page.
>> */
>> new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
>> ^
>>
>> This one. The first patch changed
>>
>> - new_pte = pfn_pte(pte_pfn(old_pte), canon_pgprot(new_prot));
>> + new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
>>
>> The fixup patch drops new_prot completely from being initialized and only works
>> on req_prot. Probably it would be best to also drop the definition of new_prot.
>> I think it then completely unused.
>
> Actually, we do need and initialize new_prot at line 495:
>
> pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
> cpa->pfn = pfn;
>
> new_prot = static_protections(req_prot, address, pfn); <---

You are right. Seems I missed that and a couble of other places. I can see them
now... Hm, Monday morning or just morning issue... So, yes, its new_prot is
initialized and is still needed, otherwise the loop over the whole range would
be subtly different.
Sorry for the noise.

-Stefan

>
> and we need it for the subsequent loop where we go over the 512 PTEs to
> decide whether to split or not.
>
> So it is needed after all, AFAICT.
>


Attachment: signature.asc
Description: OpenPGP digital signature