[for-4.16] freebsd/privcmd: fix MMAP_RESOURCE ioctl definition

Commit Message

Roger Pau Monné Nov. 15, 2021, 11:08 a.m. UTC

Current ioctl definition was wrong in both FreeBSD and Xen sources, as
the MMAP_RESOURCE ioctl needs to copy back the size of the resource
when passed a zero address and size. FreeBSD encodes in the definition
of the ioctl number whether parameters should be copied in (W) and/or
copied out (R). The current definition for MMAP_RESOURCE is lacking
the copy out part (R), and thus the call to query the size of a
resource would always return 0.

This change will break the current ioctl interface, the tools can
however fall back to using the foreign memory interface in order to
map resources from guests.

This was a shortcoming from when the hypercall and ioctl gained the
ability to query the size of the resources, as originally the
MMAP_RESOURCE ioctl didn't need to copy out any data.

Signed-off-by: Roger Pau Monné <[e-mail]>
---
Cc: Ian Jackson <[e-mail]>

The change only affects FreeBSD, and it's only a change in a
definition of an ioctl, so it's unlikely to break existing code logic.
Without this change Xen tools won't be able to use the MMAP_RESOURCE
ioctl.
---
 tools/include/xen-sys/FreeBSD/privcmd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Cooper Nov. 15, 2021, 12:03 p.m. UTC | #1

On 15/11/2021 11:08, Roger Pau Monne wrote:
> Current ioctl definition was wrong in both FreeBSD and Xen sources, as
> the MMAP_RESOURCE ioctl needs to copy back the size of the resource
> when passed a zero address and size. FreeBSD encodes in the definition
> of the ioctl number whether parameters should be copied in (W) and/or
> copied out (R). The current definition for MMAP_RESOURCE is lacking
> the copy out part (R), and thus the call to query the size of a
> resource would always return 0.
>
> This change will break the current ioctl interface, the tools can
> however fall back to using the foreign memory interface in order to
> map resources from guests.
>
> This was a shortcoming from when the hypercall and ioctl gained the
> ability to query the size of the resources, as originally the
> MMAP_RESOURCE ioctl didn't need to copy out any data.
>
> Signed-off-by: Roger Pau Monné <[e-mail]>
> ---
> Cc: Ian Jackson <[e-mail]>
>
> The change only affects FreeBSD, and it's only a change in a
> definition of an ioctl, so it's unlikely to break existing code logic.
> Without this change Xen tools won't be able to use the MMAP_RESOURCE
> ioctl.

I guess you found this while trying to fix test-resource, in which case
a further argument for the change is "the unit tests now pass on FreeBSD" ?

~Andrew

Roger Pau Monné Nov. 15, 2021, 12:19 p.m. UTC | #2

On Mon, Nov 15, 2021 at 12:03:26PM +0000, Andrew Cooper wrote:
> On 15/11/2021 11:08, Roger Pau Monne wrote:
> > Current ioctl definition was wrong in both FreeBSD and Xen sources, as
> > the MMAP_RESOURCE ioctl needs to copy back the size of the resource
> > when passed a zero address and size. FreeBSD encodes in the definition
> > of the ioctl number whether parameters should be copied in (W) and/or
> > copied out (R). The current definition for MMAP_RESOURCE is lacking
> > the copy out part (R), and thus the call to query the size of a
> > resource would always return 0.
> >
> > This change will break the current ioctl interface, the tools can
> > however fall back to using the foreign memory interface in order to
> > map resources from guests.
> >
> > This was a shortcoming from when the hypercall and ioctl gained the
> > ability to query the size of the resources, as originally the
> > MMAP_RESOURCE ioctl didn't need to copy out any data.
> >
> > Signed-off-by: Roger Pau Monné <[e-mail]>
> > ---
> > Cc: Ian Jackson <[e-mail]>
> >
> > The change only affects FreeBSD, and it's only a change in a
> > definition of an ioctl, so it's unlikely to break existing code logic.
> > Without this change Xen tools won't be able to use the MMAP_RESOURCE
> > ioctl.
> 
> I guess you found this while trying to fix test-resource, in which case
> a further argument for the change is "the unit tests now pass on FreeBSD" ?

Indeed. It seems like this is the only instance of a resource size
query that we have implemented so far.

Thanks, Roger.

Andrew Cooper Nov. 15, 2021, 12:25 p.m. UTC | #3

On 15/11/2021 12:19, Roger Pau Monné wrote:
> On Mon, Nov 15, 2021 at 12:03:26PM +0000, Andrew Cooper wrote:
>> On 15/11/2021 11:08, Roger Pau Monne wrote:
>>> Current ioctl definition was wrong in both FreeBSD and Xen sources, as
>>> the MMAP_RESOURCE ioctl needs to copy back the size of the resource
>>> when passed a zero address and size. FreeBSD encodes in the definition
>>> of the ioctl number whether parameters should be copied in (W) and/or
>>> copied out (R). The current definition for MMAP_RESOURCE is lacking
>>> the copy out part (R), and thus the call to query the size of a
>>> resource would always return 0.
>>>
>>> This change will break the current ioctl interface, the tools can
>>> however fall back to using the foreign memory interface in order to
>>> map resources from guests.
>>>
>>> This was a shortcoming from when the hypercall and ioctl gained the
>>> ability to query the size of the resources, as originally the
>>> MMAP_RESOURCE ioctl didn't need to copy out any data.
>>>
>>> Signed-off-by: Roger Pau Monné <[e-mail]>
>>> ---
>>> Cc: Ian Jackson <[e-mail]>
>>>
>>> The change only affects FreeBSD, and it's only a change in a
>>> definition of an ioctl, so it's unlikely to break existing code logic.
>>> Without this change Xen tools won't be able to use the MMAP_RESOURCE
>>> ioctl.
>> I guess you found this while trying to fix test-resource, in which case
>> a further argument for the change is "the unit tests now pass on FreeBSD" ?
> Indeed. It seems like this is the only instance of a resource size
> query that we have implemented so far.

Well - it's used on the domain create path ever since being introduced
to avoid the "shoot a hole in a superpage in order to map the grant
table" problem, but it does have a fallback which is probably why it
went unnoticed.

~Andrew

Ian Jackson Nov. 16, 2021, 3:12 p.m. UTC | #4

Roger Pau Monne writes ("[PATCH for-4.16] freebsd/privcmd: fix MMAP_RESOURCE ioctl definition"):
> Current ioctl definition was wrong in both FreeBSD and Xen sources, as
> the MMAP_RESOURCE ioctl needs to copy back the size of the resource
> when passed a zero address and size. FreeBSD encodes in the definition
> of the ioctl number whether parameters should be copied in (W) and/or
> copied out (R). The current definition for MMAP_RESOURCE is lacking
> the copy out part (R), and thus the call to query the size of a
> resource would always return 0.
> 
> This change will break the current ioctl interface, the tools can
> however fall back to using the foreign memory interface in order to
> map resources from guests.
> 
> This was a shortcoming from when the hypercall and ioctl gained the
> ability to query the size of the resources, as originally the
> MMAP_RESOURCE ioctl didn't need to copy out any data.
> 
> Signed-off-by: Roger Pau Monné <[e-mail]>
> ---
> Cc: Ian Jackson <[e-mail]>
> 
> The change only affects FreeBSD, and it's only a change in a
> definition of an ioctl, so it's unlikely to break existing code logic.
> Without this change Xen tools won't be able to use the MMAP_RESOURCE
> ioctl.

Release-Acked-by: Ian Jackson <[e-mail]>

Patch

diff --git a/tools/include/xen-sys/FreeBSD/privcmd.h b/tools/include/xen-sys/FreeBSD/privcmd.h
index 649ad443c7..70cee3db68 100644
--- a/tools/include/xen-sys/FreeBSD/privcmd.h
+++ b/tools/include/xen-sys/FreeBSD/privcmd.h
@@ -84,7 +84,7 @@  typedef struct ioctl_privcmd_dmop privcmd_dm_op_t;
 #define IOCTL_PRIVCMD_MMAPBATCH					\
 	_IOWR('E', 1, struct ioctl_privcmd_mmapbatch)
 #define IOCTL_PRIVCMD_MMAP_RESOURCE				\
-	_IOW('E', 2, struct ioctl_privcmd_mmapresource)
+	_IOWR('E', 2, struct ioctl_privcmd_mmapresource)
 #define IOCTL_PRIVCMD_DM_OP					\
 	_IOW('E', 3, struct ioctl_privcmd_dmop)
 #define IOCTL_PRIVCMD_RESTRICT					\