Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

virtcontainers: endianess issue #947

Closed
alicefr opened this issue Nov 28, 2018 · 4 comments
Closed

virtcontainers: endianess issue #947

alicefr opened this issue Nov 28, 2018 · 4 comments

Comments

@alicefr
Copy link

alicefr commented Nov 28, 2018

In the utils_linux.go there is an endianess bug. I'm testing the kata-runtime on s390x (big endian) with vsock and the function ioctlFunc always returned EINVAL(errno=22). I tried to reproduce the issue with these small golang programs:
test32.go:

package main

import (
        "fmt"
        "os"
        "syscall"
        "unsafe"

        "golang.org/x/sys/unix"
)

// from <linux/vhost.h>
// VHOST_VSOCK_SET_GUEST_CID = _IOW(VHOST_VIRTIO, 0x60, __u64)
const ioctlVhostVsockSetGuestCid = 0x4008AF60

type argsType uint32

func ioctl(fd uintptr, request int, arg1 argsType) error {
        if _, _, errno := unix.Syscall(
                unix.SYS_IOCTL,
                fd,
                uintptr(request),
                uintptr(unsafe.Pointer(&arg1)),
        ); errno != 0 {
                return os.NewSyscallError("ioctl", fmt.Errorf("%d", int(errno)))
        }

        return nil
}

func main() {
        var cid argsType = 0x4

        vsockFd, err := os.OpenFile("/dev/vhost-vsock", syscall.O_RDWR, 0666)
        if err != nil {
                fmt.Println("ERROR:")
        }

        if err := ioctl(vsockFd.Fd(), ioctlVhostVsockSetGuestCid, cid); err == nil {
                fmt.Printf("SUCCESS\n")
        } else {
                fmt.Printf("err:%s vsockFd=%d cid=%x \n", err, vsockFd.Fd(), cid)
        }

        vsockFd.Close()
}

and test64.go

package main

import (
        "fmt"
        "os"
        "syscall"
        "unsafe"

        "golang.org/x/sys/unix"
)

// from <linux/vhost.h>
// VHOST_VSOCK_SET_GUEST_CID = _IOW(VHOST_VIRTIO, 0x60, __u64)
const ioctlVhostVsockSetGuestCid = 0x4008AF60

type argsType uint64

func ioctl(fd uintptr, request int, arg1 argsType) error {
        if _, _, errno := unix.Syscall(
                unix.SYS_IOCTL,
                fd,
                uintptr(request),
                uintptr(unsafe.Pointer(&arg1)),
        ); errno != 0 {
                return os.NewSyscallError("ioctl", fmt.Errorf("%d", int(errno)))
        }

        return nil
}

func main() {
        var cid argsType = 0x4

        vsockFd, err := os.OpenFile("/dev/vhost-vsock", syscall.O_RDWR, 0666)
        if err != nil {
                fmt.Println("ERROR:")
        }

        if err := ioctl(vsockFd.Fd(), ioctlVhostVsockSetGuestCid, cid); err == nil {
                fmt.Printf("SUCCESS\n")
        } else {
                fmt.Printf("err:%s vsockFd=%d cid=%x \n", err, vsockFd.Fd(), cid)
        }

        vsockFd.Close()
}

As you probably noticed the single difference between the 2 files is the:

type argsType

On s390x:

$ sudo go run test32.go 
err:ioctl: 22 vsockFd=3 cid=4
$ sudo go run test64.go 
SUCCESS

on x86_64

$ sudo go run test32.go
SUCCESS
$ sudo go run test64.go
SUCCESS

In the kernel the cid for vsock is u64 (see https://github.com/torvalds/linux/blob/ef78e5ec9214376c5cb989f5da70b02d0c117b66/drivers/vhost/vsock.c#L664). Up to now only the first 32 bits are used as you can see from the comment. However, if the args is a unit32, that provokes an endianess issue.

There are 2 possibilites:
1- Change the args type in ioctlFunc from uint32 to uint64
2- Change all the CID vsock from unit32 to uint64. That requires also to change the type in govmm

@sboeuf
Copy link

sboeuf commented Nov 28, 2018

@alicefr thanks for this detailed explanation!

There are 2 possibilites:
1- Change the args type in ioctlFunc from uint32 to uint64
2- Change all the CID vsock from unit32 to uint64. That requires also to change the type in govmm

I would say that we want to fix the issue properly, and that means we should fix both 1 and 2 IMO. Because the kernel uses u64 for vsock's CID, I think we should do the same in our code.

So you could start with 1, which would fix the issue, and later implement 2 in order to have a fully compliant code.

@jodh-intel
Copy link
Contributor

/cc @markdryan.

@alicefr
Copy link
Author

alicefr commented Nov 29, 2018

@sboeuf I agree, it could also avoid future bugs. I don't know if the ioctl in utils_linux in the only place where the issue appears

alicefr pushed a commit to alicefr/runtime that referenced this issue Nov 29, 2018
The PR changes the parameter args to uint32 into uint64 for ioctl function.
That leads to an endianess bug.

Fixes: kata-containers#947

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
alicefr pushed a commit to alicefr/runtime that referenced this issue Nov 29, 2018
The PR changes the parameter args from uint32 to uint64 for ioctl function.
That leads to an endianess bug.

Fixes: kata-containers#947

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
alicefr pushed a commit to alicefr/govmm that referenced this issue Nov 29, 2018
The correct type used by qemu and in kernel is uint64 and this leads to
an endianess problem with ioctl system call. See the issue
kata-containers/runtime#947

Fixes: kata-containers#70

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
alicefr pushed a commit to alicefr/govmm that referenced this issue Nov 29, 2018
The correct type used by qemu and in kernel is uint64 and this leads to
an endianess problem with ioctl system call. See the issue
kata-containers/runtime#947

Fixes: kata-containers#70

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
@alicefr
Copy link
Author

alicefr commented Nov 29, 2018

To solve the second point the pr kata-containers/govmm#71 needs to be merged

alicefr pushed a commit to alicefr/runtime that referenced this issue Nov 29, 2018
The PR changes the parameter args from uint32 to uint64 for ioctl function.
That leads to an endianess bug.

Fixes: kata-containers#947

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
alicefr pushed a commit to alicefr/runtime that referenced this issue Nov 30, 2018
The CID of VSock needs to be change to uint64. Otherwise that leads to
an endianess issue. For more details see
kata-containers#947

Fixes: kata-containers#958

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
egernst pushed a commit to egernst/runtime that referenced this issue Dec 2, 2018
The PR changes the parameter args from uint32 to uint64 for ioctl function.
That leads to an endianess bug.

Fixes: kata-containers#947

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
alicefr pushed a commit to alicefr/runtime that referenced this issue Dec 4, 2018
The CID of VSock needs to be change to uint64. Otherwise that leads to
an endianess issue. For more details see
kata-containers#947

Fixes: kata-containers#958

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
alicefr pushed a commit to alicefr/runtime that referenced this issue Dec 4, 2018
The CID of VSock needs to be change to uint64. Otherwise that leads to
an endianess issue. For more details see
kata-containers#947

Fixes: kata-containers#958

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
alicefr pushed a commit to alicefr/runtime that referenced this issue Dec 6, 2018
The CID of VSock needs to be change to uint64. Otherwise that leads to
an endianess issue. For more details see
kata-containers#947

Remove the uint64 introduced by kata-containers#984

Fixes: kata-containers#958

Signed-off-by: Alice Frosi <afrosi@de.ibm.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants