Bug 243155 - Linuxulator: broken fadvise64 for 32-bit applications on amd64
Summary: Linuxulator: broken fadvise64 for 32-bit applications on amd64
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 12.1-RELEASE
Hardware: amd64 Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-07 10:15 UTC by Alex S
Modified: 2020-04-03 16:41 UTC (History)
4 users (show)

See Also:


Attachments
proposed patch (3.43 KB, patch)
2020-01-13 19:42 UTC, Mark Johnston
no flags Details | Diff
full patch (20.66 KB, patch)
2020-01-16 19:01 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex S 2020-01-07 10:15:04 UTC
% cat fadvice_bug.c
#define _GNU_SOURCE

#include <fcntl.h>

int main() {
  posix_fadvise(1, 2, 3, 4);
  return 0;
}
% /compat/linux/bin/gcc -m64 fadvice_bug.c -o fadvice_bug64
% truss ./fadvice_bug64
...
linux_fadvise64(0x1,0x2,0x3,0x4)	ERR#-19 'Operation not supported by device' # ok
...
% /compat/linux/bin/gcc -m32 fadvice_bug.c -o fadvice_bug32
% truss ./fadvice_bug32
...
linux_fadvise64(0x1,0x2,0x0,0x3)	ERR#-19 'Operation not supported by device' # hmm
...
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2020-01-07 19:49:52 UTC
Are you sure that this isn't just a truss bug?  amd64's 32-bit Linux syscall definition for fadvise64 looks right.  I note that truss doesn't define argument types for linux_fadvise64 or linux_fadvise64_64.
Comment 2 Alex S 2020-01-07 20:10:14 UTC
(In reply to Mark Johnston from comment #1)

> Are you sure that this isn't just a truss bug?

Let's adjust the test a bit:

% cat fadvice_bug.c
#define _GNU_SOURCE

#include <assert.h>
#include <fcntl.h>

int main() {
  int fd = open("/etc/passwd", O_RDONLY);
  assert(fd > 0);
  posix_fadvise(fd, 0, 0, -1);
  return 0;
}
% /compat/linux/bin/gcc -m64 fadvice_bug.c -o fadvice_bug64
% truss ./fadvice_bug64
...
linux_fadvise64(0x3,0x0,0x0,0xffffffffffffffff)	 ERR#-22 'Invalid argument'
...
% /compat/linux/bin/gcc -m32 fadvice_bug.c -o fadvice_bug32
% truss ./fadvice_bug32
...
linux_fadvise64(0x3,0x0,0x0,0x0)		 = 0 (0x0)
...

> amd64's 32-bit Linux syscall definition for fadvise64 looks right.

That's the weird part. It looks right to me as well, at least at first sight.
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2020-01-07 20:30:57 UTC
I think this is a problem with truss rather than the kernel.  Really there's two problems:

1. We don't define distinct syscall argument structures for Linux32 vs Linux64.  That is, freebsd32_posix_fadvise_args splits the off_t arguments into two fields, but linux_fadvise64_args (used in the Linux32 sysent table) does not.  So for a posix_fadvise() call in a 32-bit FreeBSD binary on amd64, truss prints:

    freebsd32_posix_fadvise(0x1,0x2,0x0,0x3,0x0,0x4) ERR#19 'Operation not supported by device'

Obviously not perfect either, but at least you can see what's happening.

2. truss doesn't call quad_fixup() on 64-bit platforms even when the target ABI is freebsd32 or linux32.  Otherwise it could fix the splitting that I described above and always print four arguments like one would expect.
Comment 4 Mark Johnston freebsd_committer freebsd_triage 2020-01-07 20:34:49 UTC
(In reply to Alex S from comment #2)
Oh I see.  Problem 1 described in comment 3 is really a kernel bug.
Comment 5 Mark Johnston freebsd_committer freebsd_triage 2020-01-07 21:01:05 UTC
(In reply to Mark Johnston from comment #4)
Nope, that's not it.  The definition of linux_fadvise64_args is wrong for 32-bit Linux, it includes four bytes of padding following fd when it shouldn't.
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2020-01-13 19:42:23 UTC
Would you be willing to test this patch?  It is only compile-tested so far.  It changes the Linux32 syscall argument structures so that we only store one argument per 8 bytes, the same way 32-bit FreeBSD compatibility works.  I'll work on converting the other problematic syscalls assuming this works.
Comment 7 Mark Johnston freebsd_committer freebsd_triage 2020-01-13 19:42:42 UTC
Created attachment 210715 [details]
proposed patch
Comment 8 Alex S 2020-01-13 21:01:13 UTC
Seems to work as intended.
Comment 9 Mark Johnston freebsd_committer freebsd_triage 2020-01-16 19:01:41 UTC
Created attachment 210799 [details]
full patch

Thanks.  Could you also give this larger patch a try?  I'm not really set up to test Linux32 applications, though some quick tests seem to work as expected.
Comment 10 Alex S 2020-01-16 23:30:54 UTC
(In reply to Mark Johnston from comment #9)

> Could you also give this larger patch a try?
> I'm not really set up to test Linux32 applications,
> though some quick tests seem to work as expected.

Well, I'm not using CURRENT, so I'm not in position to properly test the larger patch. And although this report was prompted by a Steam bug, the Steam client is not a particularly suitable application to test this either. Let's just say that after applying this to 12.1-RELEASE sources (with the exception of linux_sync_file_range and systrace chunks) and reloading linux* modules, I don't see any new breakage.
Comment 11 commit-hook freebsd_committer freebsd_triage 2020-01-21 17:28:40 UTC
A commit references this bug:

Author: markj
Date: Tue Jan 21 17:28:23 UTC 2020
New revision: 356945
URL: https://svnweb.freebsd.org/changeset/base/356945

Log:
  Fix 64-bit syscall argument fetching in 32-bit Linux syscall handlers.

  The Linux32 system call argument fetcher places each argument (passed in
  registers in the Linux x86 system call convention) into an entry in the
  generic system call args array.  Each member of this array is 8 bytes
  wide, so this approach is broken for system calls that take off_t
  arguments.

  Fix the problem by splitting l_loff_t arguments in the 32-bit system
  call descriptions, the same as we do for FreeBSD32.  Change entry points
  to handle this using the PAIR32TO64 macro.

  Move linux_ftruncate64() into compat/linux.

  PR:		243155
  Reported by:	Alex S <iwtcex@gmail.com>
  Reviewed by:	kib (previous version)
  MFC after:	2 weeks
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D23210

Changes:
  head/sys/amd64/linux32/linux32_machdep.c
  head/sys/amd64/linux32/syscalls.master
  head/sys/compat/linux/linux_file.c
  head/sys/i386/linux/linux_machdep.c
Comment 12 Mark Johnston freebsd_committer freebsd_triage 2020-04-03 16:40:40 UTC
Merged to stable/12 in r359605.  Thanks for the report.
Comment 13 commit-hook freebsd_committer freebsd_triage 2020-04-03 16:41:36 UTC
A commit references this bug:

Author: markj
Date: Fri Apr  3 16:31:46 UTC 2020
New revision: 359605
URL: https://svnweb.freebsd.org/changeset/base/359605

Log:
  MFC r356945, r356946:
  Fix 64-bit syscall argument fetching in 32-bit Linux syscall handlers.

  PR:	243155

Changes:
_U  stable/12/
  stable/12/sys/amd64/linux32/linux32_machdep.c
  stable/12/sys/amd64/linux32/linux32_proto.h
  stable/12/sys/amd64/linux32/linux32_systrace_args.c
  stable/12/sys/amd64/linux32/syscalls.master
  stable/12/sys/compat/linux/linux_file.c
  stable/12/sys/i386/linux/linux_machdep.c