Re: [v2] vhost: add vsock compat ioctl

Re: [v2] vhost: add vsock compat ioctl

[David Miller]

Although the top level ioctls are probably size and layout compatible,
I do not think that the deeper ioctls can be called by compat binaries
without some translations in order for them to work.

Re: [v2] vhost: add vsock compat ioctl

[Sonny Rao]

On Fri, Mar 16, 2018 at 12:30 PM, David Miller <davem@davemloft.net> wrote:
>
> Although the top level ioctls are probably size and layout compatible,
> I do not think that the deeper ioctls can be called by compat binaries
> without some translations in order for them to work.

Ok, thanks -- I have only tested VHOST_VSOCK_SET_GUEST_CID and
VHOST_VSOCK_SET_RUNNING but by deeper ioctls I think you might mean
the ioctls under vhost_dev_ioctl() and vhost_vring_ioctl()?

Re: [v2] vhost: add vsock compat ioctl

[David Miller]

From: Sonny Rao <sonnyrao@chromium.org>
Date: Fri, 16 Mar 2018 17:54:12 -0700

> On Fri, Mar 16, 2018 at 12:30 PM, David Miller <davem@davemloft.net> wrote:
>>
>> Although the top level ioctls are probably size and layout compatible,
>> I do not think that the deeper ioctls can be called by compat binaries
>> without some translations in order for them to work.
> 
> Ok, thanks -- I have only tested VHOST_VSOCK_SET_GUEST_CID and
> VHOST_VSOCK_SET_RUNNING but by deeper ioctls I think you might mean
> the ioctls under vhost_dev_ioctl() and vhost_vring_ioctl()?

Yes.

Re: [v2] vhost: add vsock compat ioctl

[Stefan Hajnoczi]

On Fri, Mar 16, 2018 at 7:30 PM, David Miller <davem@davemloft.net> wrote:
> Although the top level ioctls are probably size and layout compatible,
> I do not think that the deeper ioctls can be called by compat binaries
> without some translations in order for them to work.

I audited the vhost ioctl code when reviewing this patch and was
unable to find anything that would break for a 32-bit userspace
process.

drivers/vhost/net.c does the same thing already, which doesn't prove
it's correct but makes me more confident I didn't miss something while
auditing the vhost ioctl code.

Did you have a specific ioctl in mind?

Stefan

Re: [v2] vhost: add vsock compat ioctl

[Sonny Rao]

On Thu, Mar 22, 2018 at 2:25 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Mar 16, 2018 at 7:30 PM, David Miller <davem@davemloft.net> wrote:
>> Although the top level ioctls are probably size and layout compatible,
>> I do not think that the deeper ioctls can be called by compat binaries
>> without some translations in order for them to work.
>
> I audited the vhost ioctl code when reviewing this patch and was
> unable to find anything that would break for a 32-bit userspace
> process.
>
> drivers/vhost/net.c does the same thing already, which doesn't prove
> it's correct but makes me more confident I didn't miss something while
> auditing the vhost ioctl code.
>
> Did you have a specific ioctl in mind?

I think he means that we need to use the compat_ptr macro on any other
pointers we get from userspace in those other ioctls.  For most
architectures this macro doesn't seem to do much but it does on some
-- I think s390 modifies the pointer.

>
> Stefan

Re: [v2] vhost: add vsock compat ioctl

[David Miller]

From: Stefan Hajnoczi <stefanha@gmail.com>
Date: Thu, 22 Mar 2018 09:25:35 +0000

> On Fri, Mar 16, 2018 at 7:30 PM, David Miller <davem@davemloft.net> wrote:
>> Although the top level ioctls are probably size and layout compatible,
>> I do not think that the deeper ioctls can be called by compat binaries
>> without some translations in order for them to work.
> 
> I audited the vhost ioctl code when reviewing this patch and was
> unable to find anything that would break for a 32-bit userspace
> process.
> 
> drivers/vhost/net.c does the same thing already, which doesn't prove
> it's correct but makes me more confident I didn't miss something while
> auditing the vhost ioctl code.
> 
> Did you have a specific ioctl in mind?

I walked over the vhost ioctl datastructures and they all appear like
they should be compatible.

So I guess it won't be a problem after all.