| Message ID | 20220716145658.4175730-1-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V7,1/2] xen/arm: Harden the P2M code in p2m_remove_mapping() | expand |
Hi Oleksandr, In the future, please provide a cover letter (even if it is short) when you bundle multiple patches. This is useful to make generic comments and help threading in e-mail client (each patch would be a subthread of 0 rather than 1). On 16/07/2022 15:56, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Borrow the x86's check from p2m_remove_page() which was added > by the following commit: c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e > "x86/p2m: don't assert that the passed in MFN matches for a remove" > and adjust it to the Arm code base. > > Basically, this check will be strictly needed for the xenheap pages > after applying a subsequent commit which will introduce xenheap based > M2P approach on Arm. But, it will be a good opportunity to harden > the P2M code for *every* RAM pages since it is possible to remove > any GFN - MFN mapping currently on Arm (even with the wrong helpers). > > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > You can find the corresponding discussion at: > https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xen.org/ > https://lore.kernel.org/xen-devel/1652294845-13980-2-git-send-email-olekstysh@gmail.com/ > > Changes V6 -> V7: > - make this commit to be the first > - update commit description and add a comment in code > --- > xen/arch/arm/p2m.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index d00c2e462a..2a0d383df4 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1308,11 +1308,39 @@ static inline int p2m_remove_mapping(struct domain *d, > mfn_t mfn) > { > struct p2m_domain *p2m = p2m_get_hostp2m(d); > + unsigned long i; > int rc; > > p2m_write_lock(p2m); > + /* > + * Before removing the GFN - MFN mapping for any RAM pages make sure > + * that there is no difference between what is already mapped and what > + * is requested to be unmapped. > + * If they don't match bail out early. For instance, this could happen > + * if two CPUs are requesting to unmap the same P2M concurrently. Missing word: P2M *entry* Other than that: Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 16.07.22 18:06, Julien Grall wrote: > Hi Oleksandr, Hello Julien > > In the future, please provide a cover letter (even if it is short) > when you bundle multiple patches. This is useful to make generic > comments and help threading in e-mail client (each patch would be a > subthread of 0 rather than 1). Yes, will do. > > On 16/07/2022 15:56, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> Borrow the x86's check from p2m_remove_page() which was added >> by the following commit: c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e >> "x86/p2m: don't assert that the passed in MFN matches for a remove" >> and adjust it to the Arm code base. >> >> Basically, this check will be strictly needed for the xenheap pages >> after applying a subsequent commit which will introduce xenheap based >> M2P approach on Arm. But, it will be a good opportunity to harden >> the P2M code for *every* RAM pages since it is possible to remove >> any GFN - MFN mapping currently on Arm (even with the wrong helpers). >> >> Suggested-by: Julien Grall <jgrall@amazon.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> You can find the corresponding discussion at: >> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xen.org/__;!!GF_29dbcQIUBPA!3a2u-XL4NvAzSMfz72LARrdWVFvq2In5ZpUdxP2cSt7bM8PgV7P_ZclZG2R-rE9PcosUHyqsKRNfVG2TiM9Tlg$ >> [lore[.]kernel[.]org] >> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/1652294845-13980-2-git-send-email-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!3a2u-XL4NvAzSMfz72LARrdWVFvq2In5ZpUdxP2cSt7bM8PgV7P_ZclZG2R-rE9PcosUHyqsKRNfVG0kg7IZSA$ >> [lore[.]kernel[.]org] >> >> Changes V6 -> V7: >> - make this commit to be the first >> - update commit description and add a comment in code >> --- >> xen/arch/arm/p2m.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index d00c2e462a..2a0d383df4 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -1308,11 +1308,39 @@ static inline int p2m_remove_mapping(struct >> domain *d, >> mfn_t mfn) >> { >> struct p2m_domain *p2m = p2m_get_hostp2m(d); >> + unsigned long i; >> int rc; >> p2m_write_lock(p2m); >> + /* >> + * Before removing the GFN - MFN mapping for any RAM pages make >> sure >> + * that there is no difference between what is already mapped >> and what >> + * is requested to be unmapped. >> + * If they don't match bail out early. For instance, this could >> happen >> + * if two CPUs are requesting to unmap the same P2M concurrently. > > Missing word: P2M *entry* Yes. May I please ask, could this be done on the commit if this appears to be the last version? > > Other than that: > > Reviewed-by: Julien Grall <jgrall@amazon.com> Thank you! > > > Cheers, >
Hi Oleksandr, On 16/07/2022 16:29, Oleksandr Tyshchenko wrote: >> On 16/07/2022 15:56, Oleksandr Tyshchenko wrote: >>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> >>> Borrow the x86's check from p2m_remove_page() which was added >>> by the following commit: c65ea16dbcafbe4fe21693b18f8c2a3c5d14600e >>> "x86/p2m: don't assert that the passed in MFN matches for a remove" >>> and adjust it to the Arm code base. >>> >>> Basically, this check will be strictly needed for the xenheap pages >>> after applying a subsequent commit which will introduce xenheap based >>> M2P approach on Arm. But, it will be a good opportunity to harden >>> the P2M code for *every* RAM pages since it is possible to remove >>> any GFN - MFN mapping currently on Arm (even with the wrong helpers). >>> >>> Suggested-by: Julien Grall <jgrall@amazon.com> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> --- >>> You can find the corresponding discussion at: >>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/82d8bfe0-cb46-d303-6a60-2324dd76a1f7@xen.org/__;!!GF_29dbcQIUBPA!3a2u-XL4NvAzSMfz72LARrdWVFvq2In5ZpUdxP2cSt7bM8PgV7P_ZclZG2R-rE9PcosUHyqsKRNfVG2TiM9Tlg$ >>> [lore[.]kernel[.]org] >>> https://urldefense.com/v3/__https://lore.kernel.org/xen-devel/1652294845-13980-2-git-send-email-olekstysh@gmail.com/__;!!GF_29dbcQIUBPA!3a2u-XL4NvAzSMfz72LARrdWVFvq2In5ZpUdxP2cSt7bM8PgV7P_ZclZG2R-rE9PcosUHyqsKRNfVG0kg7IZSA$ >>> [lore[.]kernel[.]org] >>> >>> Changes V6 -> V7: >>> - make this commit to be the first >>> - update commit description and add a comment in code >>> --- >>> xen/arch/arm/p2m.c | 28 ++++++++++++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >>> index d00c2e462a..2a0d383df4 100644 >>> --- a/xen/arch/arm/p2m.c >>> +++ b/xen/arch/arm/p2m.c >>> @@ -1308,11 +1308,39 @@ static inline int p2m_remove_mapping(struct >>> domain *d, >>> mfn_t mfn) >>> { >>> struct p2m_domain *p2m = p2m_get_hostp2m(d); >>> + unsigned long i; >>> int rc; >>> p2m_write_lock(p2m); >>> + /* >>> + * Before removing the GFN - MFN mapping for any RAM pages make >>> sure >>> + * that there is no difference between what is already mapped >>> and what >>> + * is requested to be unmapped. >>> + * If they don't match bail out early. For instance, this could >>> happen >>> + * if two CPUs are requesting to unmap the same P2M concurrently. >> >> Missing word: P2M *entry* > > Yes. May I please ask, could this be done on the commit if this appears > to be the last version? I have committed the series with the same typo. Cheers,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index d00c2e462a..2a0d383df4 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1308,11 +1308,39 @@ static inline int p2m_remove_mapping(struct domain *d, mfn_t mfn) { struct p2m_domain *p2m = p2m_get_hostp2m(d); + unsigned long i; int rc; p2m_write_lock(p2m); + /* + * Before removing the GFN - MFN mapping for any RAM pages make sure + * that there is no difference between what is already mapped and what + * is requested to be unmapped. + * If they don't match bail out early. For instance, this could happen + * if two CPUs are requesting to unmap the same P2M concurrently. + */ + for ( i = 0; i < nr; ) + { + unsigned int cur_order; + p2m_type_t t; + mfn_t mfn_return = p2m_get_entry(p2m, gfn_add(start_gfn, i), &t, NULL, + &cur_order, NULL); + + if ( p2m_is_any_ram(t) && + (!mfn_valid(mfn) || !mfn_eq(mfn_add(mfn, i), mfn_return)) ) + { + rc = -EILSEQ; + goto out; + } + + i += (1UL << cur_order) - + ((gfn_x(start_gfn) + i) & ((1UL << cur_order) - 1)); + } + rc = p2m_set_entry(p2m, start_gfn, nr, INVALID_MFN, p2m_invalid, p2m_access_rwx); + +out: p2m_write_unlock(p2m); return rc;