On mips64le, syscall.Getdents calls the old getdents syscall

[lechner]

From Debian packaging. On mips64le, golang developers provided the older getdents syscall instead of the newer getdents64. Gocryptfs (and also Go-Fuse) depends on the newer syscall and produces nonsensical output on mips64le.

The Dirent structure that ships with golang was not meant be be used with getdents but with glibc's readdir (at least on Linux, please see NOTES in manpage). By coincidence or by design, it works with the newer syscall and is often used that way, but it does not work as shipped in golang on mips64le. An important difference is the implementation of the Type member, which was declared explicitly for the newer call. In the older linux_dirent structure it is located at Reclen - 1. One could perhaps make a case that syscall.Getdirent should be renamed on mips64le, or that both calls should be provided.

While we could probably fix gocryptfs via build constraints, I think the issue should be addressed upstream. What do you think, please? Thank you!

[rfjakob]

Bear with me for a second, I have to dig a little to understand what is going on...

On mips64le, as you noted, getdents is used instead of getdents64: https://github.com/golang/go/blob/bcc86d5f42fddc2eca1bb066668388aa6807c38c/src/syscall/syscall_linux_mips64x.go#L13

	_SYS_getdents  = SYS_GETDENTS

And getdents returns linux_dirent, which defined in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/readdir.c?h=v4.15#n147 :

struct linux_dirent {
	unsigned long	d_ino;
	unsigned long	d_off;
	unsigned short	d_reclen;
	char		d_name[1];
};

In Go mips64le, syscall.Dirent is defined here https://github.com/golang/go/blob/bcc86d5f42fddc2eca1bb066668388aa6807c38c/src/syscall/ztypes_linux_mips64le.go#L133 :

type Dirent struct {
	Ino       uint64
	Off       int64
	Reclen    uint16
	Name      [256]int8
	Type      uint8
	Pad_cgo_0 [5]byte
}

And this is wrong, because it has a Type field, which the the linux_dirent struct has not. Looks like a bug in the Go stdlib.

[rfjakob]

Reported upstream as golang/go#23624 .

The fix for gocryptfs will be to switch to unix.Getdents from package golang.org/x/sys/unix, which uses getdents64: https://github.com/golang/sys/blob/3dbebcf8efb6a5011a60c2b4591c1022a759af8a/unix/zsyscall_linux_mips64le.go#L655 :

r0, _, e1 := Syscall(SYS_GETDENTS64, uintptr(fd), uintptr(_p0), uintptr(len(buf)))

[bradfitz]

@rfjakob, indeed. Please use golang.org/x/sys/unix instead of Go's built-in syscall package.

/cc @tklauser

[lechner]

Thanks for addressing this so quickly! Would you like to see a pull request for the switch to x/sys/unix?

[rfjakob]

Thanks for the offer, but I think I'll have time later today to do it myself

[rfjakob]

@lechner Pushed to master. I'll assume this fixes the problem on mips - please reopen if it does not!

[rfjakob]

For go-fuse, this usage of syscall.Dirent is problematic: https://github.com/hanwen/go-fuse/blob/6975cb38d30430450455caad8a62d5eba93e0e4e/fuse/test/xfs_test.go#L34
Do the go-fuse tests fail on mips?

But it looks like this should be switched to unix.Dirent as well. I'll send a PR.

[lechner]

First of all, gocryptfs now builds on mips64le. As you can see in the log, all tests pass. Thank you!

The issue you found in Go-Fuse may be limited to the tests, which do not presently run in Debian. Perhaps I should connect with Hanwen about that.

In an unrelated issue gccgo 1.8.3 on s390x refused to initialize a syscall.Timespec from a unix.Timespec. I thought maybe we can sidestep that unexpected error by switching to x/sys/unix altogether.

Thank you for all your hard work, and also for providing such high quality software!

[lechner]

Here are some reasons why golang.org/x/sys/unix is an attractive alternative. A more recent discussion can be found here. Thank you!

[rfjakob]

Well, go-fuse uses syscall types for its API so switching everything will be difficult ;)

Stupid question: Why to you use gccgo? Acc. to https://golang.org/dl/ the standard Go compiler seems to support s390x since v1.7.1.

[rfjakob]

Timespec seems to be identical in syscall and unix, so I don't really understand where the error is coming from:

https://github.com/golang/go/blob/6f37fee354e941c6f143b34014c269943962b116/src/syscall/ztypes_linux_s390x.go#L22

type Timespec struct {
	Sec  int64
	Nsec int64
}

https://github.com/golang/sys/blob/8f27ce8a604014414f8dfffc25cbcde83a3f2216/unix/ztypes_linux_s390x.go#L24

type Timespec struct {
	Sec  int64
	Nsec int64
}

[lechner]

Great question! It may not be worth your time, but Debian's reasons for using gccgo on s390x are here.

As for Timespec, gccgo uses syscall in libgo, which generates them through scripts. Maybe it happens in a way (C import?) that there is no assignability guarantee. You can also see the behavior with gccgo on amd64.

[rfjakob]

I found out that gccgo provides the familiar "go" command as well, so I tried it on amd64. I got the Timespec error, but when I commented out this part, I got the next error:

2 jakob@brikett:~/go/src/github.com/rfjakob/gocryptfs$ go.gcc build
# github.com/rfjakob/gocryptfs/internal/nametransform
internal/nametransform/diriv.go:114:42: error: reference to undefined identifier ‘syscall.NAME_MAX’
  if be.longNames && len(cName) > syscall.NAME_MAX {
                                          ^
internal/nametransform/diriv.go:131:29: error: reference to undefined identifier ‘syscall.NAME_MAX’
  if len(baseName) > syscall.NAME_MAX {
                             ^

...and I suspect there are more to come. Getting gccgo to work will need a lot of workarounds...

[lechner]

Looks like the old s390x build log for 1.3. Maybe it's yet another reason to switch to golang.org/x/sys/unix? NAME_MAX is definitely available there.

[rfjakob]

I don't know, shouldn't this be fixed in gccgo?

[lechner]

Well, if they have the same policies regarding syscall as golang-go we could be waiting for a long time.

[rfjakob]

^^
Looks like gocryptfs compiles with gccgo when the NAME_MAX this is bypassed.

[lechner]

How about when you switch from syscall to unix? 😏

[rfjakob]

With commit 72815b2

I'm down to this error, which seems hard to fix:

$ go.gcc build
# github.com/rfjakob/gocryptfs/internal/syscallcompat
internal/syscallcompat/unix2syscall_linux.go:32:13: error: incompatible types in assignment (cannot use type int64 as type syscall.Timespec_sec_t)
  s.Atim.Sec = u.Atim.Sec
             ^
internal/syscallcompat/unix2syscall_linux.go:33:14: error: incompatible types in assignment (cannot use type int64 as type syscall.Timespec_nsec_t)
  s.Atim.Nsec = u.Atim.Nsec
              ^
internal/syscallcompat/unix2syscall_linux.go:35:13: error: incompatible types in assignment (cannot use type int64 as type syscall.Timespec_sec_t)
  s.Mtim.Sec = u.Mtim.Sec
             ^
internal/syscallcompat/unix2syscall_linux.go:36:14: error: incompatible types in assignment (cannot use type int64 as type syscall.Timespec_nsec_t)
  s.Mtim.Nsec = u.Mtim.Nsec
              ^
internal/syscallcompat/unix2syscall_linux.go:38:13: error: incompatible types in assignment (cannot use type int64 as type syscall.Timespec_sec_t)
  s.Ctim.Sec = u.Ctim.Sec
             ^
internal/syscallcompat/unix2syscall_linux.go:39:14: error: incompatible types in assignment (cannot use type int64 as type syscall.Timespec_nsec_t)
  s.Ctim.Nsec = u.Ctim.Nsec
              ^

[lechner]

How about getting rid of Unix2syscall altogether?

[lechner]

Looks like the C-defined types are not being resolved to elementary types. Maybe gccgo should use cgo -godefs too?

[rfjakob]

Reason for its existence is that I am using https://godoc.org/github.com/hanwen/go-fuse/fuse#Attr.FromStat , but I don't see why our own AttrFromUnixStat would be uglier than our Unix2syscall. I have created ticket #201 to track gccgo compat.