In D10799#225176, @dumbbell wrote:

Hi!

I started to think about this patch and I have some questions.

If I understand correctly, old_fstat.c is used to "patch" the rust-std bootstrap so that it calls the compatibility syscalls. Is that correct?

Yes.

Handle emaste comments.

Only activate -p if binary name does not contain '/' at all, not only if the patch is absolute. This is how execp(3) works.

In D10850#224831, @rwatson wrote:

Sounds plausible, but I do wonder if the sysctl is currently a sufficient mature way to enable application development. Enabling it requires root, so it's not directly usable by end users on multiuser systems, and it also has global scope rather than just affecting applications that the developer is working on, which could change failure modes for a range of applications (such as desktop applications) that the developer has no interest in debugging and fixing. Is there some other mechanism we can add -- e.g., using ptrace(2) -- or setting an environmental variable that causes rtld to itself twiddle a per-process setting, that might offer a better real-world debugging experience?

In D10798#224628, @pgj wrote:

Okay, but I still do not understand something: is there a problem, and if yes, what is the problem with the way rebuilding the bootstrap compiler as I drafted above?

I think I understand what you are saying, but I do not get why this would cause any problems. I assumed that if you add those static compat shims to the current version of the bootstrap compiler libraries (like you are doing in the patch), then one is able to build a fully working version of GHC. To me, this seems to be sufficient to rebuild the bootstrap compiler from the sources that could be then used to replace the existing bootstrap for systems that are newer than a given __FreeBSD_version (post-ino64). Obviously, the old version of the bootstrap would be kept used for the older ones (pre-ino64), since the new version would not work there. You can see an example of that in some older version of the port's Makefile.

In D10798#224614, @pgj wrote:

In D10798#224610, @kib wrote:

It is due to the fact that runtime shipped with the bootstrap compiler, as well as installed by the port, are static, cause problem.

Are you sure of that?

Absolutely.

In D10798#224572, @pgj wrote:

I think building a new bootstrap becomes easy with this change.

Would ghc shipped with the dynamically-linked runtime, the change appeared to be not necessary. It is due to the fact that runtime shipped with the bootstrap compiler, as well as installed by the port, are static, cause problem. There is no mechanism to ensure ABI compatibility for the static libraries, nor it might appear in the future. On the other hand, we do (try to) maintain ABI backward compatbility for dynamic linking.

Thank you for the prompt response.

Perhaps commit the changes which do not cause a discussion, now, to reduce the patch size.

In D10800#223740, @hselasky wrote:

Linux defines dev_t like this, and I think webcamd wants to keep it 32-bit.

include/linux/types.h:typedef u32 kernel_dev_t;
include/linux/types.h:typedef __kernel_dev_t dev_t;

Is it impossible to have webcamd define its own dev_t due to the header file pollution made by the ino64 patch?

ino64 patch does not introduce any header pollution.

In D10800#223737, @hselasky wrote:

dev_t is used in conjunction with MKDEV() which is 32-bit. Did you test if webcamd breaks w/o this patch or is this patch a simple result of "grep -r" ? The dev_t should not be used in any FreeBSD kernel interfaces, only Linux ones.

In D10795#223708, @kwm wrote:

Ok, I will update it later with my patch when I get to doing my maintaince.

Sorry, it is not clear to me what should I do. Should I commit the patch from this review, and then you re-apply your preferred patch ? Or just delegate the port to you and presume that you will fix it shortly after ino64 src commit ?

In D10800#223705, @hselasky wrote:

@kib

Is the patch written in a way that it can be used when the inode size is 32-bit ?

Or are ifdefs needed ?

In D10795#223701, @kwm wrote:

I would prefer the patch for libgtop I added to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218320 to be committed instead. But this whole efford is a lot of work and I can always svn mv the patch later, and replace it with the one with __FreeBSD_version checks. I need to do maintainance on the port anyway, so it not that big of a deal to fix up the patch later.

I would like to thank you for providing the patch!

Commit candidate.

In D10170#223438, @allanjude wrote:

The two 'sysent' files should be done as a second commit, is that all that needs to be done?

For new functions in rtld, I use normal style(9), so rtld slowly migrates to proper indentation. For smaller changes to existing functions I do follow existing style of 4 spaces indent/2 spaces continuation.

Overall this looks good.

I gnerally like this simplification.

What about arm64 ?

Update the message text.

In D4030#222841, @alc wrote:

Kostik, I didn't understand your last comment. Can you please elaborate?

Cover Ed notes.

Two small notes, this is not a review. Mostly to inform you about D10750.

In D10751#222780, @jonathan wrote:

Were you thinking of getopt-style parsing (but without pulling in getopt) or environment variables (e.g., LD_BINARY_FD=xx)? The environment variable approach has the benefit of not requiring any new parsing code, but an explicit --fd xx will look more familiar. Then again, if we provide options people might come to expect --help and --usage options. :)

I do not like these argv0 tricks, I intend to implement normal options parsing for the direct mode. One of the options would take the file descriptor number and do what your trick does. I think that this is better than the trick since it allows to easily invoke and test the functionality from the shell.

In D10689#222507, @cem wrote:

In D10689#222497, @kib wrote:

You need to regen syscall tables both in sys/kern and in sys/compat32. This is noted in /testing.txt.

I did try to do that. I may be invoking it wrong:

% make -C sys/kern syscalls
cc -O2 -pipe  syscalls.c  -o syscalls
/usr/lib/crt1.o: In function `_start':
/usr/src/lib/csu/amd64/crt1.c:(.text+0x17b): undefined reference to `main'
cc: error: linker command failed with exit code 1 (use -v to see invocation)
*** Error code 1

make -C sys/kern sysent
make -C sys/compat/freebsd32 sysent

In D10689#222486, @cem wrote:

Attempting to build test on ino64 branch runs into some failures apparently unrelated to this patch:

/home/cem/freebsd/sys/kern/kern_descrip.c:1309:43: error: declaration of 'struct freebsd11_fstat_args' will not be visible outside of this function [-Werror,-Wvisibility]
freebsd11_fstat(struct thread *td, struct freebsd11_fstat_args *uap)
                                          ^

Update after merge with r318298.

s/0/NULL/

Do malloc() in realpath(), as before. Pass only 'resolved' to realpath1(), all logic to malloc and free is now located in realpath().

In D9030#221857, @jonathan wrote:

Move the shift before first getenv() call in the rtld, to not depend on details of getenv() implementation.
Do not forget to reset 'env' after environment is moved,not only environ (this caused env -i /libexec/ld-elf.so.1 /bin/ls segfault in jemalloc initializer).
Fix off-by-half in copying the auxv (this caused the assertion reported by Ed).

Update comment, remove stray space after copy.
Correct (remove) re-calculation of main_argv, it was shifted right too much.
Noted by emaste.

Re-merge after the locals reordering changes were committed.

pooma% ls -l ~/build/bsd/DEV/stuff/tests/args
---x--x--x  1 kostik  kostik  6234 Apr 18  2010 /usr/home/kostik/build/bsd/DEV/stuff/tests/args
pooma% LD_LIBRARY_PATH=/usr/lib32 obj-i386/i386.i386/usr/home/kostik/work/build/bsd/DEV/src/libexec/rtld-elf/ld-elf.so.1 ~/build/bsd/DEV/stuff/tests/args 1 2 3
Fatal error

I.e. as expected, userspace needs to read executing binary.

In D9030#221771, @jonathan wrote:

In D9030#221740, @kib wrote:

Let's split two things. I thought that your issue at hand was the conflict between the nature of capability mode disallowing implicit root and absolute lookups, badly interfering with the typical absolute path specification for ELF interpreters.

Fair enough: that was, indeed, the original motivation. However, as soon as we start building more sophisticated compartments out of sandboxed-from-inception processes, I'm going to want to explore more interesting uses like the LLVM fat binary case (in which we will want a different linker from the one specified by ELF brandinfo). I see three mutually-compatible requirements:

• must be able to execute dynamically-linked applications from capability mode,
• must be able to choose a non-standard run-time linker and
• should not require root privilege to do it.
  
  I think that an ffexecve(2) system call is a clean, fairly simple mechanism for supporting these and other requirements: it's a policy-agnostic mechanism that one might use to explore many different ideas in sandboxing. The approach that you suggested ("make kernel verify that the interpreter path in the binary PT_INTERP is equal to the ABI brandinfo interp_path and execute namei() for interp_path in non-cap mode") makes the ABI more complicated for non-expert developers to understand and also (this is the part that makes me nervous) deliberately violates Rule #1 of capability mode: no access to global namespaces.

My point is that ffexecve(2) -like syscall is not needed.

The patch as-is is fine.

Peter, could you, please, test this change ? The interesting load is the same as was used for testing r244240, and the UP config is probably most important, but I do not expect surprises.