| Message ID | f9cd7b84-6f51-d797-cd2a-b9c9bc62b0f6@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Argo: don't obtain excess page references | expand |
On 11.10.2022 11:28, Jan Beulich wrote: > find_ring_mfn() already holds a page reference when trying to obtain a > writable type reference. We shouldn't make assumptions on the general > reference count limit being effectively "infinity". Obtain merely a type > ref, re-using the general ref by only dropping the previously acquired > one in the case of an error. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Ping? > --- > I further question the log-dirty check there: The present P2M type of a > page doesn't really matter for writing to the page (plus it's stale by > the time it is looked at). Instead I think every write to such a page > needs to be accompanied by a call to paging_mark_dirty(). > > --- a/xen/common/argo.c > +++ b/xen/common/argo.c > @@ -1429,10 +1429,11 @@ find_ring_mfn(struct domain *d, gfn_t gf > ret = -EAGAIN; > #endif > else if ( (p2mt != p2m_ram_rw) || > - !get_page_and_type(page, d, PGT_writable_page) ) > + !get_page_type(page, PGT_writable_page) ) > ret = -EINVAL; > > - put_page(page); > + if ( unlikely(ret) ) > + put_page(page); > > return ret; > } >
On Mon, Nov 21, 2022 at 4:41 AM Jan Beulich <jbeulich@suse.com> wrote: > On 11.10.2022 11:28, Jan Beulich wrote: > > find_ring_mfn() already holds a page reference when trying to obtain a > > writable type reference. We shouldn't make assumptions on the general > > reference count limit being effectively "infinity". Obtain merely a type > > ref, re-using the general ref by only dropping the previously acquired > > one in the case of an error. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Ping? > Message received -- I will look at this. Thanks. Christopher > > > --- > > I further question the log-dirty check there: The present P2M type of a > > page doesn't really matter for writing to the page (plus it's stale by > > the time it is looked at). Instead I think every write to such a page > > needs to be accompanied by a call to paging_mark_dirty(). > > > > --- a/xen/common/argo.c > > +++ b/xen/common/argo.c > > @@ -1429,10 +1429,11 @@ find_ring_mfn(struct domain *d, gfn_t gf > > ret = -EAGAIN; > > #endif > > else if ( (p2mt != p2m_ram_rw) || > > - !get_page_and_type(page, d, PGT_writable_page) ) > > + !get_page_type(page, PGT_writable_page) ) > > ret = -EINVAL; > > > > - put_page(page); > > + if ( unlikely(ret) ) > > + put_page(page); > > > > return ret; > > } > > > >
On Mon, Nov 21, 2022 at 4:41 AM Jan Beulich <jbeulich@suse.com> wrote: > On 11.10.2022 11:28, Jan Beulich wrote: > > find_ring_mfn() already holds a page reference when trying to obtain a > > writable type reference. We shouldn't make assumptions on the general > > reference count limit being effectively "infinity". Obtain merely a type > > ref, re-using the general ref by only dropping the previously acquired > > one in the case of an error. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Ping? > Hi Jan, Sorry it has taken me so long to review this patch and thank-you for posting it. The points raised are helpful. Wrt to the patch - I can't ack because: the general ref that is already held is from the page owner, and it may actually be foreign; so the second ref acquire is currently ensuring that it is a match for the owner of the ring. That needs addressing. Am supportive of points raised: - review + limit ref counts taken - better to not need two general page refs - a type ref rather than general may be sufficient to hold for the ring lifetime? - paging_mark_dirty at writes - p2m log dirty would be better to be allowed than EAGAIN - allowing mapping of foreign pages may have uses though likely also challenging I should let you know that my time available is extremely limited at the moment, sorry. Christopher > > > --- > > I further question the log-dirty check there: The present P2M type of a > > page doesn't really matter for writing to the page (plus it's stale by > > the time it is looked at). Instead I think every write to such a page > > needs to be accompanied by a call to paging_mark_dirty(). > > > > --- a/xen/common/argo.c > > +++ b/xen/common/argo.c > > @@ -1429,10 +1429,11 @@ find_ring_mfn(struct domain *d, gfn_t gf > > ret = -EAGAIN; > > #endif > > else if ( (p2mt != p2m_ram_rw) || > > - !get_page_and_type(page, d, PGT_writable_page) ) > > + !get_page_type(page, PGT_writable_page) ) > > ret = -EINVAL; > > > > - put_page(page); > > + if ( unlikely(ret) ) > > + put_page(page); > > > > return ret; > > } > > > >
On 30.01.2023 05:35, Christopher Clark wrote: > On Mon, Nov 21, 2022 at 4:41 AM Jan Beulich <jbeulich@suse.com> wrote: > >> On 11.10.2022 11:28, Jan Beulich wrote: >>> find_ring_mfn() already holds a page reference when trying to obtain a >>> writable type reference. We shouldn't make assumptions on the general >>> reference count limit being effectively "infinity". Obtain merely a type >>> ref, re-using the general ref by only dropping the previously acquired >>> one in the case of an error. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> Ping? > > Sorry it has taken me so long to review this patch and thank-you for > posting it. The points raised are helpful. > > Wrt to the patch - I can't ack because: > the general ref that is already held is from the page owner, and it may > actually be foreign; so the second ref acquire is currently ensuring that > it is a match for the owner of the ring. That needs addressing. I'm afraid I may not understand your reply: Are you saying there's something wrong with the change? Or are you saying there's something wrong that merely becomes apparent due to the change? Or yet something else? > Am supportive of points raised: > - review + limit ref counts taken > - better to not need two general page refs > - a type ref rather than general may be sufficient to hold for the ring > lifetime? > - paging_mark_dirty at writes > - p2m log dirty would be better to be allowed than EAGAIN I can certainly extend the patch to a series, but that'll make sense only if ... > - allowing mapping of foreign pages may have uses though likely also > challenging > > I should let you know that my time available is extremely limited at the > moment, sorry. ... it would then also be looked at within a reasonable amount of time. I'm already sitting on way too many unreviewed patches. Jan
On 30.01.2023 09:03, Jan Beulich wrote: > On 30.01.2023 05:35, Christopher Clark wrote: >> On Mon, Nov 21, 2022 at 4:41 AM Jan Beulich <jbeulich@suse.com> wrote: >> >>> On 11.10.2022 11:28, Jan Beulich wrote: >>>> find_ring_mfn() already holds a page reference when trying to obtain a >>>> writable type reference. We shouldn't make assumptions on the general >>>> reference count limit being effectively "infinity". Obtain merely a type >>>> ref, re-using the general ref by only dropping the previously acquired >>>> one in the case of an error. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> Ping? >> >> Sorry it has taken me so long to review this patch and thank-you for >> posting it. The points raised are helpful. >> >> Wrt to the patch - I can't ack because: >> the general ref that is already held is from the page owner, and it may >> actually be foreign; so the second ref acquire is currently ensuring that >> it is a match for the owner of the ring. That needs addressing. > > I'm afraid I may not understand your reply: Are you saying there's something > wrong with the change? Or are you saying there's something wrong that merely > becomes apparent due to the change? Or yet something else? And to extend on the questions: What notion of "foreign" are you referring to here? The earlier check_get_page_from_gfn() is passed the very same domain. And anyway, get_page() wouldn't allow to acquire a ref with a domain other than the page owner. Plus the sole caller of find_ring_mfns() passes currd. Jan
--- a/xen/common/argo.c +++ b/xen/common/argo.c @@ -1429,10 +1429,11 @@ find_ring_mfn(struct domain *d, gfn_t gf ret = -EAGAIN; #endif else if ( (p2mt != p2m_ram_rw) || - !get_page_and_type(page, d, PGT_writable_page) ) + !get_page_type(page, PGT_writable_page) ) ret = -EINVAL; - put_page(page); + if ( unlikely(ret) ) + put_page(page); return ret; }
find_ring_mfn() already holds a page reference when trying to obtain a writable type reference. We shouldn't make assumptions on the general reference count limit being effectively "infinity". Obtain merely a type ref, re-using the general ref by only dropping the previously acquired one in the case of an error. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I further question the log-dirty check there: The present P2M type of a page doesn't really matter for writing to the page (plus it's stale by the time it is looked at). Instead I think every write to such a page needs to be accompanied by a call to paging_mark_dirty().