Page MenuHomeFreeBSD
Differential D23347

Add support for batching interrupts in system interrupt code
Needs ReviewPublic
Actions

Authored by • hselasky on Jan 24 2020, 11:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 19, 1:02 PM
Unknown Object (File)
Fri, Apr 18, 5:57 AM
Unknown Object (File)
Wed, Apr 16, 7:55 PM
Unknown Object (File)
Feb 25 2025, 8:29 AM
Unknown Object (File)
Feb 16 2025, 9:20 AM
Unknown Object (File)
Jan 31 2025, 8:08 PM
Unknown Object (File)
Jan 5 2025, 8:56 AM
Unknown Object (File)
Jan 5 2025, 8:55 AM
View All 99 Files
Subscribers
imp

Details

Reviewers
gallatin
kib
glebius
bz
mav
Summary

Implement intr_handler_needs_execution() to serve the purpose of allowing high-frequency interrupts to be batched.
By calling intr_handler_needs_execution() the driver acknowledge interrupts that occurred after the invocation of the current interrupt handler call.

This change basically reverts r357004.

Interrupt handlers can do batching like this:

static void interrupt_handler(void *arg)
{
  int to = 1000;

  do {
       /* interrupt code */
   } while (to-- && intr_handler_needs_execution(ih_cookie));
}

Sponsored by: Mellanox Technologies

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 28944

Event Timeline

• hselasky created this revision.Jan 24 2020, 11:05 AM
Herald added a subscriber: imp. · View Herald TranscriptJan 24 2020, 11:05 AM
Harbormaster completed remote builds in B28893: Diff 67246.Jan 24 2020, 11:05 AM
• hselasky updated this revision to Diff 67247.Jan 24 2020, 11:10 AM

Fix compilation issue and make sure network epoch is available when used!

Harbormaster completed remote builds in B28894: Diff 67247.Jan 24 2020, 11:10 AM
• hselasky added a reviewer: bz.Jan 24 2020, 12:20 PM
gallatin added a comment.Jan 24 2020, 2:11 PM

Some feedback:

  • This essentially removes the epoch optimization from all network drivers because it becomes opt-in, rather than automatic. I don't like that
  • You've replaced a flag with an 8 byte field in a struct which can be touched hundreds of thousands or millions of times per second. I don't like that.
  • You've removed interrupt storm epoch hogging protection. I don't like that.

To do what you want, can you instead make a negative flag for opting opt and leave most the rest of the code intact?

gallatin requested changes to this revision.Jan 24 2020, 2:11 PM
This revision now requires changes to proceed.Jan 24 2020, 2:11 PM
• hselasky added a comment.Jan 24 2020, 2:18 PM

Hi,

This essentially removes the epoch optimization from all network drivers because it becomes opt-in, rather than automatic. I don't like that

Yes, this feature should be op-in until someone sits down and fixes all the network driver's input paths.

FreeBSD is a general purpose operating system. It shall not only be hard-optimised for network drivers, like the patch Gleb S. made.
There might be more subsystems which want another epoch than the network epoch and my patch addresses that.

You've replaced a flag with an 8 byte field in a struct which can be touched hundreds of thousands or millions of times per second. I don't like that.

This field should not be touched more than 8k times per second. Else you have a network card you should not use for heavy network traffic or you have not tuned the RX queues appropriately.

You've removed interrupt storm epoch hogging protection. I don't like that.

The epoch wrapper should be per-interrupt function call and that's it.

To do what you want, can you instead make a negative flag for opting opt and leave most the rest of the code intact?

I disagree.

kib added a comment.Jan 24 2020, 3:52 PM

To add some colors to the discussion, I disagree with both approaches. My main complain is that *net epoch* should not be married with the interrupts handlers infrastructure, neither in code nor in data. I agree with people that mark this as layering violations.

If you want automatic epoch enter, with possible rate or interval limiting, you need a hooks that are executed before and after the thread handler loop is performed. We already have examples of such hooks with pre- and post- filter and ithread hooks.

And yes, it should be opt-in and not automatic on INTR_TYPE_NET

• hselasky updated this revision to Diff 67349.Jan 27 2020, 12:16 PM
• hselasky retitled this revision from Add support for subsystem specific epoch flag when requesting interrupts to Add support for batching interrupts to system interrupt code.
• hselasky edited the summary of this revision. (Show Details)
• hselasky added a reviewer: mav.

Implement feedback.

Harbormaster completed remote builds in B28943: Diff 67349.Jan 27 2020, 12:16 PM
• hselasky updated this revision to Diff 67350.Jan 27 2020, 12:18 PM

Minor compile fix.

Harbormaster completed remote builds in B28944: Diff 67350.Jan 27 2020, 12:18 PM
• hselasky retitled this revision from Add support for batching interrupts to system interrupt code to Add support for batching interrupts in system interrupt code.Jan 27 2020, 12:18 PM
• hselasky edited the summary of this revision. (Show Details)Jan 27 2020, 6:08 PM
• hselasky edited the summary of this revision. (Show Details)
• hselasky mentioned this in D23348: Widen EPOCH(9) usage in network drivers (as a pre-step for D23347).Jan 27 2020, 11:20 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
47 lines
sys/
2 lines

Diff 67350

View Options

sys/kern/kern_intr.c

Show First 20 Lines • Show All 333 Lines • ▼ Show 20 Lines
* exit procedure pointer and the third to a place to store the main * exit procedure pointer and the third to a place to store the main
* program's object. * program's object.
* *
* The return value is the main program's entry point. * The return value is the main program's entry point.
*/ */
func_ptr_type func_ptr_type
_rtld(Elf_Addr *sp, func_ptr_type *exit_proc, Obj_Entry **objp) _rtld(Elf_Addr *sp, func_ptr_type *exit_proc, Obj_Entry **objp)
{ {
Elf_Auxinfo *aux, *auxp, *aux_info[AT_COUNT]; Elf_Auxinfo *aux, *auxp, *auxpf, *aux_info[AT_COUNT];
Objlist_Entry *entry; Objlist_Entry *entry;
Obj_Entry *last_interposer, *obj, *preload_tail; Obj_Entry *last_interposer, *obj, *preload_tail;
const Elf_Phdr *phdr; const Elf_Phdr *phdr;
Objlist initlist; Objlist initlist;
RtldLockState lockstate; RtldLockState lockstate;
char **argv, *argv0, **env, *kexecpath, *library_path_rpath; Elf_Addr *argcp;
char **argv, *argv0, **env, **envp, *kexecpath, *library_path_rpath;
caddr_t imgentry; caddr_t imgentry;
char buf[MAXPATHLEN]; char buf[MAXPATHLEN];
int argc, fd, i, mib[2], phnum; int argc, fd, i, mib[2], phnum;
size_t len; size_t len;
emasteUnsubmitted
Not Done
Inline Actions

Perhaps commit the re-sorting independently first?

emaste: Perhaps commit the re-sorting independently first?
kibAuthorUnsubmitted
Not Done
Inline Actions

This is not just resorting, I moved locals from the nested blocks and added some variables as well.

I can produce separate patch to reorder existing local section, but the bits in the patch are not extractable.

kib: This is not just resorting, I moved locals from the nested blocks and added some variables as…
emasteUnsubmitted
Not Done
Inline Actions

Right, I mean there is both resorting and other changes and I think the resorting part could be committed by itself - it would make it more clear what the other changes are.

emaste: Right, I mean there is both resorting and other changes and I think the resorting part could be…
/* /*
* On entry, the dynamic linker itself has not been relocated yet. * On entry, the dynamic linker itself has not been relocated yet.
* Be very careful not to reference any global data until after * Be very careful not to reference any global data until after
* init_rtld has returned. It is OK to reference file-scope statics * init_rtld has returned. It is OK to reference file-scope statics
* and string constants, and to call static and global functions. * and string constants, and to call static and global functions.
*/ */
/* Find the auxiliary vector on the stack. */ /* Find the auxiliary vector on the stack. */
argcp = sp;
argc = *sp++; argc = *sp++;
argv = (char **) sp; argv = (char **) sp;
sp += argc + 1; /* Skip over arguments and NULL terminator */ sp += argc + 1; /* Skip over arguments and NULL terminator */
env = (char **) sp; env = (char **) sp;
while (*sp++ != 0) /* Skip over environment, and NULL terminator */ while (*sp++ != 0) /* Skip over environment, and NULL terminator */
; ;
aux = (Elf_Auxinfo *) sp; aux = (Elf_Auxinfo *) sp;
/* Digest the auxiliary vector. */ /* Digest the auxiliary vector. */
for (i = 0; i < AT_COUNT; i++) for (i = 0; i < AT_COUNT; i++)
aux_info[i] = NULL; aux_info[i] = NULL;
for (auxp = aux; auxp->a_type != AT_NULL; auxp++) { for (auxp = aux; auxp->a_type != AT_NULL; auxp++) {
if (auxp->a_type < AT_COUNT) if (auxp->a_type < AT_COUNT)
aux_info[auxp->a_type] = auxp; aux_info[auxp->a_type] = auxp;
} }
/* Initialize and relocate ourselves. */ /* Initialize and relocate ourselves. */
assert(aux_info[AT_BASE] != NULL); assert(aux_info[AT_BASE] != NULL);
emasteUnsubmitted
Not Done
Inline Actions

With the patch this assert triggers via

% /libexec/ld-elf.so.1 /libexec/ld-elf.so.1
ld-elf.so.1: assert failed: /usr/home/emaste/src/freebsd/libexec/rtld-elf/rtld.c:381
emaste: With the patch this assert triggers via ``` % /libexec/ld-elf.so.1 /libexec/ld-elf.so.1 ld-elf.
init_rtld((caddr_t) aux_info[AT_BASE]->a_un.a_ptr, aux_info); init_rtld((caddr_t) aux_info[AT_BASE]->a_un.a_ptr, aux_info);
__progname = obj_rtld.path; __progname = obj_rtld.path;
argv0 = argv[0] != NULL ? argv[0] : "(null)"; argv0 = argv[0] != NULL ? argv[0] : "(null)";
environ = env; environ = env;
main_argc = argc; main_argc = argc;
main_argv = argv; main_argv = argv;
Show All 17 Lines if (sysctl(mib, 2, __stack_chk_guard, &len, NULL, 0) == -1 ||
((unsigned char *)(void *)__stack_chk_guard)[3] = 255; ((unsigned char *)(void *)__stack_chk_guard)[3] = 255;
} }
} }
trust = !issetugid(); trust = !issetugid();
md_abi_variant_hook(aux_info); md_abi_variant_hook(aux_info);
fd = -1;
if (aux_info[AT_EXECFD] != NULL) {
fd = aux_info[AT_EXECFD]->a_un.a_val;
} else {
assert(aux_info[AT_PHDR] != NULL);
phdr = (const Elf_Phdr *)aux_info[AT_PHDR]->a_un.a_ptr;
if (phdr == obj_rtld.phdr) {
dbg("opening main program in direct exec mode");
if (argc >= 2) {
argv0 = argv[1];
fd = open(argv0, O_RDONLY | O_CLOEXEC | O_VERIFY);
if (fd == -1) {
rtld_printf("Opening %s: %s\n", argv0,
rtld_strerror(errno));
rtld_die();
}
/*
* For direct exec mode, argv[0] is the interpreter
* name, we must remove it and shift arguments left by
* 1 before invoking binary main. Since stack layout
* places environment pointers and aux vectors right
* after the terminating NULL, we must shift
* environment and aux as well.
* XXX Shift will be > 1 when options are implemented.
*/
do {
*argv = *(argv + 1);
argv++;
} while (*argv != 0);
emasteUnsubmitted
Done
Inline Actions

!= NULL?

also envp below

emaste: `!= NULL`? also envp below
*argcp -= 1;
main_argc = argc - 1;
environ = env = envp = argv;
do {
*envp = *(envp + 1);
envp++;
} while (*envp != 0);
aux = auxp = (Elf_Auxinfo *)envp;
auxpf = (Elf_Auxinfo *)(envp + 1);
for (;; auxp++, auxpf++) {
*auxp = *auxpf;
if (auxp->a_type == AT_NULL)
break;
}
} else {
rtld_printf("no binary\n");
rtld_die();
}
}
}
ld_bind_now = getenv(_LD("BIND_NOW")); ld_bind_now = getenv(_LD("BIND_NOW"));
/* /*
* If the process is tainted, then we un-set the dangerous environment * If the process is tainted, then we un-set the dangerous environment
* variables. The process will be marked as tainted until setuid(2) * variables. The process will be marked as tainted until setuid(2)
* is called. If any child process calls setuid(2) we do not want any * is called. If any child process calls setuid(2) we do not want any
* future processes to honor the potentially un-safe variables. * future processes to honor the potentially un-safe variables.
*/ */
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines (caddr_t) aux_info[AT_BASE]->a_un.a_ptr);
dbg("initializing thread locks"); dbg("initializing thread locks");
lockdflt_init(); lockdflt_init();
/* /*
* Load the main program, or process its program header if it is * Load the main program, or process its program header if it is
* already loaded. * already loaded.
*/ */
if (aux_info[AT_EXECFD] != NULL) { /* Load the main program. */ if (fd != -1) { /* Load the main program. */
fd = aux_info[AT_EXECFD]->a_un.a_val;
dbg("loading main program"); dbg("loading main program");
obj_main = map_object(fd, argv0, NULL); obj_main = map_object(fd, argv0, NULL);
close(fd); close(fd);
emasteUnsubmitted
Not Done
Inline Actions

extra space before aux_info?

emaste: extra space before `aux_info`?
if (obj_main == NULL) if (obj_main == NULL)
rtld_die(); rtld_die();
max_stack_flags = obj->stack_flags; max_stack_flags = obj->stack_flags;
} else { /* Main program already loaded. */ } else { /* Main program already loaded. */
dbg("processing main program's program header"); dbg("processing main program's program header");
assert(aux_info[AT_PHDR] != NULL); assert(aux_info[AT_PHDR] != NULL);
emasteUnsubmitted
Not Done
Inline Actions

As an aside it seems O_VERIFY is not documented in open.2. I see that this came from https://github.com/freebsd/freebsd/pull/27 - paging @stevek

Also, what happens if we try to ld-elf.so.1 foo where foo has ---x--x--x perms?

emaste: As an aside it seems `O_VERIFY` is not documented in `open.2`. I see that this came from https…
kibAuthorUnsubmitted
Not Done
Inline Actions

It is something internal from J that got committed but never get the flesh in our repository.

kib: It is something internal from J that got committed but never get the flesh in our repository.
stevekUnsubmitted
Not Done
Inline Actions

See D2902, which has the changes. I guess after all this time people have had enough time to comment. I'll see about committing that set of changes.

stevek: See D2902, which has the changes. I guess after all this time people have had enough time to…
emasteUnsubmitted
Not Done
Inline Actions

Ah. I added a comment in there - I'd suggest we should go ahead with the open.2 changes right now, since the corresponding source changes were made some time ago, and follow up with a D2902 rebase.

emaste: Ah. I added a comment in there - I'd suggest we should go ahead with the open.2 changes right…
stevekUnsubmitted
Not Done
Inline Actions

Sure, makes sense. I committed just the open.2 change and will rebase D2902.

stevek: Sure, makes sense. I committed just the open.2 change and will rebase D2902.
phdr = (const Elf_Phdr *) aux_info[AT_PHDR]->a_un.a_ptr; phdr = (const Elf_Phdr *) aux_info[AT_PHDR]->a_un.a_ptr;
assert(aux_info[AT_PHNUM] != NULL); assert(aux_info[AT_PHNUM] != NULL);
phnum = aux_info[AT_PHNUM]->a_un.a_val; phnum = aux_info[AT_PHNUM]->a_un.a_val;
assert(aux_info[AT_PHENT] != NULL); assert(aux_info[AT_PHENT] != NULL);
assert(aux_info[AT_PHENT]->a_un.a_val == sizeof(Elf_Phdr)); assert(aux_info[AT_PHENT]->a_un.a_val == sizeof(Elf_Phdr));
assert(aux_info[AT_ENTRY] != NULL); assert(aux_info[AT_ENTRY] != NULL);
imgentry = (caddr_t) aux_info[AT_ENTRY]->a_un.a_ptr; imgentry = (caddr_t) aux_info[AT_ENTRY]->a_un.a_ptr;
if ((obj_main = digest_phdr(phdr, phnum, imgentry, argv0)) == NULL) if ((obj_main = digest_phdr(phdr, phnum, imgentry, argv0)) == NULL)
emasteUnsubmitted
Not Done
Inline Actions

I think this comment could be expanded a bit to separately explain main_arg[cv] and the argv/envp/auxp manipulation that follows it.

emaste: I think this comment could be expanded a bit to separately explain main_arg[cv] and the…
rtld_die(); rtld_die();
} }
if (aux_info[AT_EXECPATH] != NULL) { if (aux_info[AT_EXECPATH] != NULL && fd == -1) {
kexecpath = aux_info[AT_EXECPATH]->a_un.a_ptr; kexecpath = aux_info[AT_EXECPATH]->a_un.a_ptr;
dbg("AT_EXECPATH %p %s", kexecpath, kexecpath); dbg("AT_EXECPATH %p %s", kexecpath, kexecpath);
if (kexecpath[0] == '/') if (kexecpath[0] == '/')
obj_main->path = kexecpath; obj_main->path = kexecpath;
else if (getcwd(buf, sizeof(buf)) == NULL || else if (getcwd(buf, sizeof(buf)) == NULL ||
strlcat(buf, "/", sizeof(buf)) >= sizeof(buf) || strlcat(buf, "/", sizeof(buf)) >= sizeof(buf) ||
strlcat(buf, kexecpath, sizeof(buf)) >= sizeof(buf)) strlcat(buf, kexecpath, sizeof(buf)) >= sizeof(buf))
obj_main->path = xstrdup(argv0); obj_main->path = xstrdup(argv0);
else else
obj_main->path = xstrdup(buf); obj_main->path = xstrdup(buf);
} else { } else {
dbg("No AT_EXECPATH"); dbg("No AT_EXECPATH or direct exec");
obj_main->path = xstrdup(argv0); obj_main->path = xstrdup(argv0);
} }
dbg("obj_main path %s", obj_main->path); dbg("obj_main path %s", obj_main->path);
obj_main->mainprog = true; obj_main->mainprog = true;
if (aux_info[AT_STACKPROT] != NULL && if (aux_info[AT_STACKPROT] != NULL &&
aux_info[AT_STACKPROT]->a_un.a_val != 0) aux_info[AT_STACKPROT]->a_un.a_val != 0)
stack_prot = aux_info[AT_STACKPROT]->a_un.a_val; stack_prot = aux_info[AT_STACKPROT]->a_un.a_val;
▲ Show 20 Lines • Show All 4,764 Lines • Show Last 20 Lines
View Options

sys/sys/interrupt.h

Show First 20 Lines • Show All 333 Lines • ▼ Show 20 Lines
* exit procedure pointer and the third to a place to store the main * exit procedure pointer and the third to a place to store the main
* program's object. * program's object.
* *
* The return value is the main program's entry point. * The return value is the main program's entry point.
*/ */
func_ptr_type func_ptr_type
_rtld(Elf_Addr *sp, func_ptr_type *exit_proc, Obj_Entry **objp) _rtld(Elf_Addr *sp, func_ptr_type *exit_proc, Obj_Entry **objp)
{ {
Elf_Auxinfo *aux, *auxp, *aux_info[AT_COUNT]; Elf_Auxinfo *aux, *auxp, *auxpf, *aux_info[AT_COUNT];
Objlist_Entry *entry; Objlist_Entry *entry;
Obj_Entry *last_interposer, *obj, *preload_tail; Obj_Entry *last_interposer, *obj, *preload_tail;
const Elf_Phdr *phdr; const Elf_Phdr *phdr;
Objlist initlist; Objlist initlist;
RtldLockState lockstate; RtldLockState lockstate;
char **argv, *argv0, **env, *kexecpath, *library_path_rpath; Elf_Addr *argcp;
char **argv, *argv0, **env, **envp, *kexecpath, *library_path_rpath;
caddr_t imgentry; caddr_t imgentry;
char buf[MAXPATHLEN]; char buf[MAXPATHLEN];
int argc, fd, i, mib[2], phnum; int argc, fd, i, mib[2], phnum;
size_t len; size_t len;
emasteUnsubmitted
Not Done
Inline Actions

Perhaps commit the re-sorting independently first?

emaste: Perhaps commit the re-sorting independently first?
kibAuthorUnsubmitted
Not Done
Inline Actions

This is not just resorting, I moved locals from the nested blocks and added some variables as well.

I can produce separate patch to reorder existing local section, but the bits in the patch are not extractable.

kib: This is not just resorting, I moved locals from the nested blocks and added some variables as…
emasteUnsubmitted
Not Done
Inline Actions

Right, I mean there is both resorting and other changes and I think the resorting part could be committed by itself - it would make it more clear what the other changes are.

emaste: Right, I mean there is both resorting and other changes and I think the resorting part could be…
/* /*
* On entry, the dynamic linker itself has not been relocated yet. * On entry, the dynamic linker itself has not been relocated yet.
* Be very careful not to reference any global data until after * Be very careful not to reference any global data until after
* init_rtld has returned. It is OK to reference file-scope statics * init_rtld has returned. It is OK to reference file-scope statics
* and string constants, and to call static and global functions. * and string constants, and to call static and global functions.
*/ */
/* Find the auxiliary vector on the stack. */ /* Find the auxiliary vector on the stack. */
argcp = sp;
argc = *sp++; argc = *sp++;
argv = (char **) sp; argv = (char **) sp;
sp += argc + 1; /* Skip over arguments and NULL terminator */ sp += argc + 1; /* Skip over arguments and NULL terminator */
env = (char **) sp; env = (char **) sp;
while (*sp++ != 0) /* Skip over environment, and NULL terminator */ while (*sp++ != 0) /* Skip over environment, and NULL terminator */
; ;
aux = (Elf_Auxinfo *) sp; aux = (Elf_Auxinfo *) sp;
/* Digest the auxiliary vector. */ /* Digest the auxiliary vector. */
for (i = 0; i < AT_COUNT; i++) for (i = 0; i < AT_COUNT; i++)
aux_info[i] = NULL; aux_info[i] = NULL;
for (auxp = aux; auxp->a_type != AT_NULL; auxp++) { for (auxp = aux; auxp->a_type != AT_NULL; auxp++) {
if (auxp->a_type < AT_COUNT) if (auxp->a_type < AT_COUNT)
aux_info[auxp->a_type] = auxp; aux_info[auxp->a_type] = auxp;
} }
/* Initialize and relocate ourselves. */ /* Initialize and relocate ourselves. */
assert(aux_info[AT_BASE] != NULL); assert(aux_info[AT_BASE] != NULL);
emasteUnsubmitted
Not Done
Inline Actions

With the patch this assert triggers via

% /libexec/ld-elf.so.1 /libexec/ld-elf.so.1
ld-elf.so.1: assert failed: /usr/home/emaste/src/freebsd/libexec/rtld-elf/rtld.c:381
emaste: With the patch this assert triggers via ``` % /libexec/ld-elf.so.1 /libexec/ld-elf.so.1 ld-elf.
init_rtld((caddr_t) aux_info[AT_BASE]->a_un.a_ptr, aux_info); init_rtld((caddr_t) aux_info[AT_BASE]->a_un.a_ptr, aux_info);
__progname = obj_rtld.path; __progname = obj_rtld.path;
argv0 = argv[0] != NULL ? argv[0] : "(null)"; argv0 = argv[0] != NULL ? argv[0] : "(null)";
environ = env; environ = env;
main_argc = argc; main_argc = argc;
main_argv = argv; main_argv = argv;
Show All 17 Lines if (sysctl(mib, 2, __stack_chk_guard, &len, NULL, 0) == -1 ||
((unsigned char *)(void *)__stack_chk_guard)[3] = 255; ((unsigned char *)(void *)__stack_chk_guard)[3] = 255;
} }
} }
trust = !issetugid(); trust = !issetugid();
md_abi_variant_hook(aux_info); md_abi_variant_hook(aux_info);
fd = -1;
if (aux_info[AT_EXECFD] != NULL) {
fd = aux_info[AT_EXECFD]->a_un.a_val;
} else {
assert(aux_info[AT_PHDR] != NULL);
phdr = (const Elf_Phdr *)aux_info[AT_PHDR]->a_un.a_ptr;
if (phdr == obj_rtld.phdr) {
dbg("opening main program in direct exec mode");
if (argc >= 2) {
argv0 = argv[1];
fd = open(argv0, O_RDONLY | O_CLOEXEC | O_VERIFY);
if (fd == -1) {
rtld_printf("Opening %s: %s\n", argv0,
rtld_strerror(errno));
rtld_die();
}
/*
* For direct exec mode, argv[0] is the interpreter
* name, we must remove it and shift arguments left by
* 1 before invoking binary main. Since stack layout
* places environment pointers and aux vectors right
* after the terminating NULL, we must shift
* environment and aux as well.
* XXX Shift will be > 1 when options are implemented.
*/
do {
*argv = *(argv + 1);
argv++;
} while (*argv != 0);
emasteUnsubmitted
Done
Inline Actions

!= NULL?

also envp below

emaste: `!= NULL`? also envp below
*argcp -= 1;
main_argc = argc - 1;
environ = env = envp = argv;
do {
*envp = *(envp + 1);
envp++;
} while (*envp != 0);
aux = auxp = (Elf_Auxinfo *)envp;
auxpf = (Elf_Auxinfo *)(envp + 1);
for (;; auxp++, auxpf++) {
*auxp = *auxpf;
if (auxp->a_type == AT_NULL)
break;
}
} else {
rtld_printf("no binary\n");
rtld_die();
}
}
}
ld_bind_now = getenv(_LD("BIND_NOW")); ld_bind_now = getenv(_LD("BIND_NOW"));
/* /*
* If the process is tainted, then we un-set the dangerous environment * If the process is tainted, then we un-set the dangerous environment
* variables. The process will be marked as tainted until setuid(2) * variables. The process will be marked as tainted until setuid(2)
* is called. If any child process calls setuid(2) we do not want any * is called. If any child process calls setuid(2) we do not want any
* future processes to honor the potentially un-safe variables. * future processes to honor the potentially un-safe variables.
*/ */
▲ Show 20 Lines • Show All 44 Lines • ▼ Show 20 Lines (caddr_t) aux_info[AT_BASE]->a_un.a_ptr);
dbg("initializing thread locks"); dbg("initializing thread locks");
lockdflt_init(); lockdflt_init();
/* /*
* Load the main program, or process its program header if it is * Load the main program, or process its program header if it is
* already loaded. * already loaded.
*/ */
if (aux_info[AT_EXECFD] != NULL) { /* Load the main program. */ if (fd != -1) { /* Load the main program. */
fd = aux_info[AT_EXECFD]->a_un.a_val;
dbg("loading main program"); dbg("loading main program");
obj_main = map_object(fd, argv0, NULL); obj_main = map_object(fd, argv0, NULL);
close(fd); close(fd);
emasteUnsubmitted
Not Done
Inline Actions

extra space before aux_info?

emaste: extra space before `aux_info`?
if (obj_main == NULL) if (obj_main == NULL)
rtld_die(); rtld_die();
max_stack_flags = obj->stack_flags; max_stack_flags = obj->stack_flags;
} else { /* Main program already loaded. */ } else { /* Main program already loaded. */
dbg("processing main program's program header"); dbg("processing main program's program header");
assert(aux_info[AT_PHDR] != NULL); assert(aux_info[AT_PHDR] != NULL);
emasteUnsubmitted
Not Done
Inline Actions

As an aside it seems O_VERIFY is not documented in open.2. I see that this came from https://github.com/freebsd/freebsd/pull/27 - paging @stevek

Also, what happens if we try to ld-elf.so.1 foo where foo has ---x--x--x perms?

emaste: As an aside it seems `O_VERIFY` is not documented in `open.2`. I see that this came from https…
kibAuthorUnsubmitted
Not Done
Inline Actions

It is something internal from J that got committed but never get the flesh in our repository.

kib: It is something internal from J that got committed but never get the flesh in our repository.
stevekUnsubmitted
Not Done
Inline Actions

See D2902, which has the changes. I guess after all this time people have had enough time to comment. I'll see about committing that set of changes.

stevek: See D2902, which has the changes. I guess after all this time people have had enough time to…
emasteUnsubmitted
Not Done
Inline Actions

Ah. I added a comment in there - I'd suggest we should go ahead with the open.2 changes right now, since the corresponding source changes were made some time ago, and follow up with a D2902 rebase.

emaste: Ah. I added a comment in there - I'd suggest we should go ahead with the open.2 changes right…
stevekUnsubmitted
Not Done
Inline Actions

Sure, makes sense. I committed just the open.2 change and will rebase D2902.

stevek: Sure, makes sense. I committed just the open.2 change and will rebase D2902.
phdr = (const Elf_Phdr *) aux_info[AT_PHDR]->a_un.a_ptr; phdr = (const Elf_Phdr *) aux_info[AT_PHDR]->a_un.a_ptr;
assert(aux_info[AT_PHNUM] != NULL); assert(aux_info[AT_PHNUM] != NULL);
phnum = aux_info[AT_PHNUM]->a_un.a_val; phnum = aux_info[AT_PHNUM]->a_un.a_val;
assert(aux_info[AT_PHENT] != NULL); assert(aux_info[AT_PHENT] != NULL);
assert(aux_info[AT_PHENT]->a_un.a_val == sizeof(Elf_Phdr)); assert(aux_info[AT_PHENT]->a_un.a_val == sizeof(Elf_Phdr));
assert(aux_info[AT_ENTRY] != NULL); assert(aux_info[AT_ENTRY] != NULL);
imgentry = (caddr_t) aux_info[AT_ENTRY]->a_un.a_ptr; imgentry = (caddr_t) aux_info[AT_ENTRY]->a_un.a_ptr;
if ((obj_main = digest_phdr(phdr, phnum, imgentry, argv0)) == NULL) if ((obj_main = digest_phdr(phdr, phnum, imgentry, argv0)) == NULL)
emasteUnsubmitted
Not Done
Inline Actions

I think this comment could be expanded a bit to separately explain main_arg[cv] and the argv/envp/auxp manipulation that follows it.

emaste: I think this comment could be expanded a bit to separately explain main_arg[cv] and the…
rtld_die(); rtld_die();
} }
if (aux_info[AT_EXECPATH] != NULL) { if (aux_info[AT_EXECPATH] != NULL && fd == -1) {
kexecpath = aux_info[AT_EXECPATH]->a_un.a_ptr; kexecpath = aux_info[AT_EXECPATH]->a_un.a_ptr;
dbg("AT_EXECPATH %p %s", kexecpath, kexecpath); dbg("AT_EXECPATH %p %s", kexecpath, kexecpath);
if (kexecpath[0] == '/') if (kexecpath[0] == '/')
obj_main->path = kexecpath; obj_main->path = kexecpath;
else if (getcwd(buf, sizeof(buf)) == NULL || else if (getcwd(buf, sizeof(buf)) == NULL ||
strlcat(buf, "/", sizeof(buf)) >= sizeof(buf) || strlcat(buf, "/", sizeof(buf)) >= sizeof(buf) ||
strlcat(buf, kexecpath, sizeof(buf)) >= sizeof(buf)) strlcat(buf, kexecpath, sizeof(buf)) >= sizeof(buf))
obj_main->path = xstrdup(argv0); obj_main->path = xstrdup(argv0);
else else
obj_main->path = xstrdup(buf); obj_main->path = xstrdup(buf);
} else { } else {
dbg("No AT_EXECPATH"); dbg("No AT_EXECPATH or direct exec");
obj_main->path = xstrdup(argv0); obj_main->path = xstrdup(argv0);
} }
dbg("obj_main path %s", obj_main->path); dbg("obj_main path %s", obj_main->path);
obj_main->mainprog = true; obj_main->mainprog = true;
if (aux_info[AT_STACKPROT] != NULL && if (aux_info[AT_STACKPROT] != NULL &&
aux_info[AT_STACKPROT]->a_un.a_val != 0) aux_info[AT_STACKPROT]->a_un.a_val != 0)
stack_prot = aux_info[AT_STACKPROT]->a_un.a_val; stack_prot = aux_info[AT_STACKPROT]->a_un.a_val;
▲ Show 20 Lines • Show All 4,764 Lines • Show Last 20 Lines