linux-kernel - Re: [PATCH v9] xen/grant-table: Avoid m2p

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53063D2B.1040502@citrix.com>
Date:	Thu, 20 Feb 2014 17:36:43 +0000
From:	Zoltan Kiss <zoltan.kiss@...rix.com>
To:	Stefano Stabellini <stefano.stabellini@...citrix.com>
CC:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	David Vrabel <david.vrabel@...rix.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, <x86@...nel.org>,
	<xen-devel@...ts.xenproject.org>, <linux-kernel@...r.kernel.org>,
	Roger Pau Monné <roger.pau@...rix.com>,
	Jan Beulich <jbeulich@...e.com>,
	Ian Campbell <Ian.Campbell@...rix.com>
Subject: Re: [PATCH v9] xen/grant-table: Avoid m2p_override during mapping

On 20/02/14 17:26, Stefano Stabellini wrote:
> On Mon, 17 Feb 2014, Zoltan Kiss wrote:
>> On 16/02/14 18:36, Stefano Stabellini wrote:
>>> On Wed, 12 Feb 2014, Zoltan Kiss wrote:
>>>> diff --git a/arch/arm/include/asm/xen/page.h
>>>> b/arch/arm/include/asm/xen/page.h
>>>> index e0965ab..4eaeb3f 100644
>>>> --- a/arch/arm/include/asm/xen/page.h
>>>> +++ b/arch/arm/include/asm/xen/page.h
>>>> @@ -97,16 +97,15 @@ static inline pte_t *lookup_address(unsigned long
>>>> address, unsigned int *level)
>>>>    	return NULL;
>>>>    }
>>>>
>>>> -static inline int m2p_add_override(unsigned long mfn, struct page *page,
>>>> -		struct gnttab_map_grant_ref *kmap_op)
>>>> -{
>>>> -	return 0;
>>>> -}
>>>> -
>>>> -static inline int m2p_remove_override(struct page *page, bool clear_pte)
>>>> -{
>>>> -	return 0;
>>>> -}
>>>> +extern int set_foreign_p2m_mapping(struct gnttab_map_grant_ref *map_ops,
>>>> +				   struct gnttab_map_grant_ref *kmap_ops,
>>>> +				   struct page **pages, unsigned int count,
>>>> +				   bool m2p_override);
>>>> +
>>>> +extern int clear_foreign_p2m_mapping(struct gnttab_unmap_grant_ref
>>>> *unmap_ops,
>>>> +				     struct gnttab_map_grant_ref *kmap_ops,
>>>> +				     struct page **pages, unsigned int count,
>>>> +				     bool m2p_override);
>>>
>>> Much much better.
>>> The only comment I have is about this m2p_override boolean parameter.
>>> m2p_override is now meaningless in this context, what we really want to
>>> let the arch specific implementation know is whether the mapping is a
>>> kernel only mapping or a userspace mapping.
>>> Testing for kmap_ops != NULL might even be enough, but it would not
>>> improve the interface.
>> gntdev is the only user of this, the kmap_ops parameter there is:
>> use_ptemod ? map->kmap_ops + offset : NULL
>> where:
>> use_ptemod = !xen_feature(XENFEAT_auto_translated_physmap);
>> So I think we can't rely on kmap_ops to decide whether we should m2p_override
>> or not.
>>
>>> Is it possible to realize if the mapping is a userspace mapping by
>>> checking for GNTMAP_application_map in map_ops?
>>> Otherwise I would keep the boolean and rename it to user_mapping.
>> Sounds better, but as far as I see gntdev set that flag in find_grant_ptes,
>> which is called only
>>
>> if (use_ptemod) {
>> 	err = apply_to_page_range(vma->vm_mm, vma->vm_start,
>> 				  vma->vm_end - vma->vm_start,
>> 				  find_grant_ptes, map);
>>
>> So if xen_feature(XENFEAT_auto_translated_physmap), we don't have kmap_ops,
>> and GNTMAP_application_map is not set as well, but I guess we still need
>> m2p_override. Or not? I'm a bit confused, maybe because of Monday ...
>
> If xen_feature(XENFEAT_auto_translated_physmap) we shouldn't need the
> m2p_override.
>

So it's safe to assume that we need m2p_override only if kmap_ops != 
NULL, and we can avoid the extra bool parameter, is that correct? At 
least with the current users of grant mapping it seems to be true.
In which case we don't need the wrappers for gnttab_[un]map_refs as well.
Actually the most of m2p_add/remove_override takes effect only if there 
is a kmap_op parameter, but what about the rest of the code there?

Zoli
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/