| Message ID | 20200728113712.22966-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Multiple fixes to XENMEM_acquire_resource | expand |
On 28.07.2020 13:37, Andrew Cooper wrote: > Copy the nr_frames from the correct structure, so the caller doesn't > unconditionally receive 0. Well, no - it does get copied from the correct structure. It's just that the field doesn't get set properly up front. Otherwise you'll (a) build in an unchecked assumption that the native and compat fields match in type and (b) set a bad example for people looking here and then cloning this code in perhaps a case where (a) is not even true. If you agree, the alternative change of setting cmp.mar.nr_frames from nat.mar->nr_frames before the call is Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
> -----Original Message----- > From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: 28 July 2020 12:37 > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <George.Dunlap@eu.citrix.com>; Ian > Jackson <ian.jackson@citrix.com>; Jan Beulich <JBeulich@suse.com>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Julien > Grall <julien@xen.org>; Paul Durrant <paul@xen.org>; Michał Leszczyński <michal.leszczynski@cert.pl>; > Hubert Jasudowicz <hubert.jasudowicz@cert.pl> > Subject: [PATCH 3/5] xen/memory: Fix compat XENMEM_acquire_resource for size requests > > Copy the nr_frames from the correct structure, so the caller doesn't > unconditionally receive 0. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Paul Durrant <paul@xen.org> > --- > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Ian Jackson <ian.jackson@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@xen.org> > CC: Paul Durrant <paul@xen.org> > CC: Michał Leszczyński <michal.leszczynski@cert.pl> > CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> > --- > xen/common/compat/memory.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c > index 3851f756c7..ed92e05b08 100644 > --- a/xen/common/compat/memory.c > +++ b/xen/common/compat/memory.c > @@ -599,7 +599,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) > if ( __copy_field_to_guest( > guest_handle_cast(compat, > compat_mem_acquire_resource_t), > - &cmp.mar, nr_frames) ) > + nat.mar, nr_frames) ) > return -EFAULT; > } > else > -- > 2.11.0
On 29/07/2020 21:09, Jan Beulich wrote: > On 28.07.2020 13:37, Andrew Cooper wrote: >> Copy the nr_frames from the correct structure, so the caller doesn't >> unconditionally receive 0. > > Well, no - it does get copied from the correct structure. It's just > that the field doesn't get set properly up front. You appear to be objecting to my use of the term "correct". There are two structures. One contains the correct value, and one contains the wrong value, which happens to always be 0. I stand by sentence as currently written. > Otherwise you'll > (a) build in an unchecked assumption that the native and compat > fields match in type Did you actually check? Because I did before embarking on this course of action. In file included from /local/xen.git/xen/include/xen/guest_access.h:10:0, from compat/memory.c:5: /local/xen.git/xen/include/asm/guest_access.h:152:28: error: comparison of distinct pointer types lacks a cast [-Werror] (void)(&(hnd).p->field == _s); \ ^ compat/memory.c:628:22: note: in expansion of macro ‘__copy_field_to_guest’ if ( __copy_field_to_guest( ^~~~~~~~~~~~~~~~~~~~~ This is what the compiler thinks of the code, when nr_frames is changed from uint32_t to unsigned long. > and (b) set a bad example for people looking > here This entire function is a massive set of bad examples; the worst IMO being the fact that there isn't a single useful comment anywhere in it concerning how the higher level loop structure works. I'm constantly annoyed that I need to reverse engineer it from scratch every time I look at it, despite having a better-than-most understanding of what it is trying to achieve, and how it is supposed to work. I realise this is noones fault in particular, but it is not fair/reasonable to claim that this change is the thing setting a bad example in this file. > and then cloning this code in perhaps a case where (a) is not > even true. If you agree, the alternative change of setting > cmp.mar.nr_frames from nat.mar->nr_frames before the call is Is there more to this sentence? While this example could be implemented (at even higher overhead) by copying nat back to cmp before passing it back to the guest, the same is not true for the changes required to fix batching (which is another series the same size as this). ~Andrew
On 30.07.2020 21:12, Andrew Cooper wrote: > On 29/07/2020 21:09, Jan Beulich wrote: >> On 28.07.2020 13:37, Andrew Cooper wrote: >>> Copy the nr_frames from the correct structure, so the caller doesn't >>> unconditionally receive 0. >> >> Well, no - it does get copied from the correct structure. It's just >> that the field doesn't get set properly up front. > > You appear to be objecting to my use of the term "correct". > > There are two structures. One contains the correct value, and one > contains the wrong value, which happens to always be 0. > > I stand by sentence as currently written. At the risk of splitting hair, what you copy from is a field holding the correct value, but not the correct field. This only works correctly because of the way __copy_field_{from,to}_guest() happen to be implemented; there are possible alternative implementations where this would break, despite ... >> Otherwise you'll >> (a) build in an unchecked assumption that the native and compat >> fields match in type > > Did you actually check? Because I did before embarking on this course > of action. > > In file included from /local/xen.git/xen/include/xen/guest_access.h:10:0, > from compat/memory.c:5: > /local/xen.git/xen/include/asm/guest_access.h:152:28: error: comparison > of distinct pointer types lacks a cast [-Werror] > (void)(&(hnd).p->field == _s); \ > ^ > compat/memory.c:628:22: note: in expansion of macro ‘__copy_field_to_guest’ > if ( __copy_field_to_guest( > ^~~~~~~~~~~~~~~~~~~~~ > > This is what the compiler thinks of the code, when nr_frames is changed > from uint32_t to unsigned long. ... this type safety check (which, I admit, I didn't consider when writing my reply). I continue to think that handle and struct should match up not just for {,__}copy_{from,to}_guest() but also for {,__}copy_field_{from,to}_guest(). >> and (b) set a bad example for people looking >> here > > This entire function is a massive set of bad examples; the worst IMO > being the fact that there isn't a single useful comment anywhere in it > concerning how the higher level loop structure works. > > I'm constantly annoyed that I need to reverse engineer it from scratch > every time I look at it, despite having a better-than-most understanding > of what it is trying to achieve, and how it is supposed to work. > > I realise this is noones fault in particular, but it is not > fair/reasonable to claim that this change is the thing setting a bad > example in this file. I'd be happy to see "bad examples" be corrected. As stated at various occasions, at the time I first implemented the compat layer this seemed like the most reasonable approach to me. If you see room for improvement, then I'm all for it. >> and then cloning this code in perhaps a case where (a) is not >> even true. If you agree, the alternative change of setting >> cmp.mar.nr_frames from nat.mar->nr_frames before the call is > > Is there more to this sentence? I guess I can't figure what you mean here. > While this example could be implemented (at even higher overhead) by > copying nat back to cmp before passing it back to the guest, the same is > not true for the changes required to fix batching (which is another > series the same size as this). I'll see when you post this, but I think we will want the principle outlined above to continue to hold. Jan
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c index 3851f756c7..ed92e05b08 100644 --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -599,7 +599,7 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) if ( __copy_field_to_guest( guest_handle_cast(compat, compat_mem_acquire_resource_t), - &cmp.mar, nr_frames) ) + nat.mar, nr_frames) ) return -EFAULT; } else
Copy the nr_frames from the correct structure, so the caller doesn't unconditionally receive 0. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> CC: Paul Durrant <paul@xen.org> CC: Michał Leszczyński <michal.leszczynski@cert.pl> CC: Hubert Jasudowicz <hubert.jasudowicz@cert.pl> --- xen/common/compat/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)