Message ID | 09f5540ab05de0a14992d8071f6feb0137775156.1575558989.git.fweimer@redhat.com |
---|---|
State | New |
Headers | show |
Series | Remove signal blocking from dlopen | expand |
On 05/12/2019 12:20, Florian Weimer wrote: > Blocking signals causes issues with certain anti-malware solutions > which rely on an unblocked SIGSYS signal for system calls they > intercept. > > This reverts commit a2e8aa0d9ea648068d8be52dd7b15f1b6a008e23 > ("Block signals during the initial part of dlopen") and adds > comments related to async signal safety to active_nodelete and > its caller. > > Note that this does not make lazy binding async-signal-safe with regards > to dlopen. It merely avoids introducing new async-signal-safety hazards > as part of the NODELETE changes. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > elf/dl-open.c | 37 +++++++++++-------------------------- > 1 file changed, 11 insertions(+), 26 deletions(-) > > diff --git a/elf/dl-open.c b/elf/dl-open.c > index c23341be58..5a1c5b5326 100644 > --- a/elf/dl-open.c > +++ b/elf/dl-open.c > @@ -34,7 +34,6 @@ > #include <atomic.h> > #include <libc-internal.h> > #include <array_length.h> > -#include <internal-signals.h> > > #include <dl-dst.h> > #include <dl-prop.h> > @@ -53,10 +52,6 @@ struct dl_open_args > /* Namespace ID. */ > Lmid_t nsid; > > - /* Original signal mask. Used for unblocking signal handlers before > - running ELF constructors. */ > - sigset_t original_signal_mask; > - > /* Original value of _ns_global_scope_pending_adds. Set by > dl_open_worker. Only valid if nsid is a real namespace > (non-negative). */ Ok. > @@ -446,6 +441,9 @@ activate_nodelete (struct link_map *new) > _dl_debug_printf ("activating NODELETE for %s [%lu]\n", > l->l_name, l->l_ns); > > + /* The flag can already be true at this point, e.g. a signal > + handler may have triggered lazy binding and set NODELETE > + status immediately. */ > l->l_nodelete_active = true; > > /* This is just a debugging aid, to indicate that Ok. > @@ -520,16 +518,12 @@ dl_open_worker (void *a) > if (new == NULL) > { > assert (mode & RTLD_NOLOAD); > - __libc_signal_restore_set (&args->original_signal_mask); > return; > } > > if (__glibc_unlikely (mode & __RTLD_SPROF)) > - { > - /* This happens only if we load a DSO for 'sprof'. */ > - __libc_signal_restore_set (&args->original_signal_mask); > - return; > - } > + /* This happens only if we load a DSO for 'sprof'. */ > + return; > > /* This object is directly loaded. */ > ++new->l_direct_opencount; Ok. > @@ -565,7 +559,6 @@ dl_open_worker (void *a) > > assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT); > > - __libc_signal_restore_set (&args->original_signal_mask); > return; > } > Ok. > @@ -712,6 +705,12 @@ dl_open_worker (void *a) > All memory allocations for new objects must have happened > before. */ > > + /* Finalize the NODELETE status first. This comes before > + update_scopes, so that lazy binding will not see pending NODELETE > + state for newly loaded objects. There is a compiler barrier in > + update_scopes which ensures that the changes from > + activate_nodelete are visible before new objects show up in the > + local scope. */ > activate_nodelete (new); > > /* Second stage after resize_scopes: Actually perform the scope Ok. > @@ -745,10 +744,6 @@ dl_open_worker (void *a) > if (mode & RTLD_GLOBAL) > add_to_global_resize (new); > > - /* Unblock signals. Data structures are now consistent, and > - application code may run. */ > - __libc_signal_restore_set (&args->original_signal_mask); > - > /* Run the initializer functions of new objects. Temporarily > disable the exception handler, so that lazy binding failures are > fatal. */ Ok. > @@ -838,10 +833,6 @@ no more namespaces available for dlmopen()")); > args.argv = argv; > args.env = env; > > - /* Recursive lazy binding during manipulation of the dynamic loader > - structures may result in incorrect behavior. */ > - __libc_signal_block_all (&args.original_signal_mask); > - > struct dl_exception exception; > int errcode = _dl_catch_exception (&exception, dl_open_worker, &args); > Ok. > @@ -882,16 +873,10 @@ no more namespaces available for dlmopen()")); > > _dl_close_worker (args.map, true); > > - /* Restore the signal mask. In the success case, this > - happens inside dl_open_worker. */ > - __libc_signal_restore_set (&args.original_signal_mask); > - > /* All l_nodelete_pending objects should have been deleted > at this point, which is why it is not necessary to reset > the flag here. */ > } > - else > - __libc_signal_restore_set (&args.original_signal_mask); > > assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT); > > Ok.
On 12/12/19 12:01 PM, Adhemerval Zanella wrote: > > > On 05/12/2019 12:20, Florian Weimer wrote: >> Blocking signals causes issues with certain anti-malware solutions >> which rely on an unblocked SIGSYS signal for system calls they >> intercept. >> >> This reverts commit a2e8aa0d9ea648068d8be52dd7b15f1b6a008e23 >> ("Block signals during the initial part of dlopen") and adds >> comments related to async signal safety to active_nodelete and >> its caller. >> >> Note that this does not make lazy binding async-signal-safe with regards >> to dlopen. It merely avoids introducing new async-signal-safety hazards >> as part of the NODELETE changes. > > LGTM, thanks. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> LGTM too. Reviewed-by: Carlos O'Donell <carlos@redhat.com> >> --- >> elf/dl-open.c | 37 +++++++++++-------------------------- >> 1 file changed, 11 insertions(+), 26 deletions(-) >> >> diff --git a/elf/dl-open.c b/elf/dl-open.c >> index c23341be58..5a1c5b5326 100644 >> --- a/elf/dl-open.c >> +++ b/elf/dl-open.c >> @@ -34,7 +34,6 @@ >> #include <atomic.h> >> #include <libc-internal.h> >> #include <array_length.h> >> -#include <internal-signals.h> >> >> #include <dl-dst.h> >> #include <dl-prop.h> >> @@ -53,10 +52,6 @@ struct dl_open_args >> /* Namespace ID. */ >> Lmid_t nsid; >> >> - /* Original signal mask. Used for unblocking signal handlers before >> - running ELF constructors. */ >> - sigset_t original_signal_mask; >> - >> /* Original value of _ns_global_scope_pending_adds. Set by >> dl_open_worker. Only valid if nsid is a real namespace >> (non-negative). */ > > Ok. > >> @@ -446,6 +441,9 @@ activate_nodelete (struct link_map *new) >> _dl_debug_printf ("activating NODELETE for %s [%lu]\n", >> l->l_name, l->l_ns); >> >> + /* The flag can already be true at this point, e.g. a signal >> + handler may have triggered lazy binding and set NODELETE >> + status immediately. */ >> l->l_nodelete_active = true; >> >> /* This is just a debugging aid, to indicate that > > Ok. > >> @@ -520,16 +518,12 @@ dl_open_worker (void *a) >> if (new == NULL) >> { >> assert (mode & RTLD_NOLOAD); >> - __libc_signal_restore_set (&args->original_signal_mask); >> return; >> } >> >> if (__glibc_unlikely (mode & __RTLD_SPROF)) >> - { >> - /* This happens only if we load a DSO for 'sprof'. */ >> - __libc_signal_restore_set (&args->original_signal_mask); >> - return; >> - } >> + /* This happens only if we load a DSO for 'sprof'. */ >> + return; >> >> /* This object is directly loaded. */ >> ++new->l_direct_opencount; > > Ok. > >> @@ -565,7 +559,6 @@ dl_open_worker (void *a) >> >> assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT); >> >> - __libc_signal_restore_set (&args->original_signal_mask); >> return; >> } >> > > Ok. > >> @@ -712,6 +705,12 @@ dl_open_worker (void *a) >> All memory allocations for new objects must have happened >> before. */ >> >> + /* Finalize the NODELETE status first. This comes before >> + update_scopes, so that lazy binding will not see pending NODELETE >> + state for newly loaded objects. There is a compiler barrier in >> + update_scopes which ensures that the changes from >> + activate_nodelete are visible before new objects show up in the >> + local scope. */ >> activate_nodelete (new); >> >> /* Second stage after resize_scopes: Actually perform the scope > > Ok. > >> @@ -745,10 +744,6 @@ dl_open_worker (void *a) >> if (mode & RTLD_GLOBAL) >> add_to_global_resize (new); >> >> - /* Unblock signals. Data structures are now consistent, and >> - application code may run. */ >> - __libc_signal_restore_set (&args->original_signal_mask); >> - >> /* Run the initializer functions of new objects. Temporarily >> disable the exception handler, so that lazy binding failures are >> fatal. */ > > Ok. > >> @@ -838,10 +833,6 @@ no more namespaces available for dlmopen()")); >> args.argv = argv; >> args.env = env; >> >> - /* Recursive lazy binding during manipulation of the dynamic loader >> - structures may result in incorrect behavior. */ >> - __libc_signal_block_all (&args.original_signal_mask); >> - >> struct dl_exception exception; >> int errcode = _dl_catch_exception (&exception, dl_open_worker, &args); >> > > Ok. > >> @@ -882,16 +873,10 @@ no more namespaces available for dlmopen()")); >> >> _dl_close_worker (args.map, true); >> >> - /* Restore the signal mask. In the success case, this >> - happens inside dl_open_worker. */ >> - __libc_signal_restore_set (&args.original_signal_mask); >> - >> /* All l_nodelete_pending objects should have been deleted >> at this point, which is why it is not necessary to reset >> the flag here. */ >> } >> - else >> - __libc_signal_restore_set (&args.original_signal_mask); >> >> assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT); >> >> > > Ok. >
diff --git a/elf/dl-open.c b/elf/dl-open.c index c23341be58..5a1c5b5326 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -34,7 +34,6 @@ #include <atomic.h> #include <libc-internal.h> #include <array_length.h> -#include <internal-signals.h> #include <dl-dst.h> #include <dl-prop.h> @@ -53,10 +52,6 @@ struct dl_open_args /* Namespace ID. */ Lmid_t nsid; - /* Original signal mask. Used for unblocking signal handlers before - running ELF constructors. */ - sigset_t original_signal_mask; - /* Original value of _ns_global_scope_pending_adds. Set by dl_open_worker. Only valid if nsid is a real namespace (non-negative). */ @@ -446,6 +441,9 @@ activate_nodelete (struct link_map *new) _dl_debug_printf ("activating NODELETE for %s [%lu]\n", l->l_name, l->l_ns); + /* The flag can already be true at this point, e.g. a signal + handler may have triggered lazy binding and set NODELETE + status immediately. */ l->l_nodelete_active = true; /* This is just a debugging aid, to indicate that @@ -520,16 +518,12 @@ dl_open_worker (void *a) if (new == NULL) { assert (mode & RTLD_NOLOAD); - __libc_signal_restore_set (&args->original_signal_mask); return; } if (__glibc_unlikely (mode & __RTLD_SPROF)) - { - /* This happens only if we load a DSO for 'sprof'. */ - __libc_signal_restore_set (&args->original_signal_mask); - return; - } + /* This happens only if we load a DSO for 'sprof'. */ + return; /* This object is directly loaded. */ ++new->l_direct_opencount; @@ -565,7 +559,6 @@ dl_open_worker (void *a) assert (_dl_debug_initialize (0, args->nsid)->r_state == RT_CONSISTENT); - __libc_signal_restore_set (&args->original_signal_mask); return; } @@ -712,6 +705,12 @@ dl_open_worker (void *a) All memory allocations for new objects must have happened before. */ + /* Finalize the NODELETE status first. This comes before + update_scopes, so that lazy binding will not see pending NODELETE + state for newly loaded objects. There is a compiler barrier in + update_scopes which ensures that the changes from + activate_nodelete are visible before new objects show up in the + local scope. */ activate_nodelete (new); /* Second stage after resize_scopes: Actually perform the scope @@ -745,10 +744,6 @@ dl_open_worker (void *a) if (mode & RTLD_GLOBAL) add_to_global_resize (new); - /* Unblock signals. Data structures are now consistent, and - application code may run. */ - __libc_signal_restore_set (&args->original_signal_mask); - /* Run the initializer functions of new objects. Temporarily disable the exception handler, so that lazy binding failures are fatal. */ @@ -838,10 +833,6 @@ no more namespaces available for dlmopen()")); args.argv = argv; args.env = env; - /* Recursive lazy binding during manipulation of the dynamic loader - structures may result in incorrect behavior. */ - __libc_signal_block_all (&args.original_signal_mask); - struct dl_exception exception; int errcode = _dl_catch_exception (&exception, dl_open_worker, &args); @@ -882,16 +873,10 @@ no more namespaces available for dlmopen()")); _dl_close_worker (args.map, true); - /* Restore the signal mask. In the success case, this - happens inside dl_open_worker. */ - __libc_signal_restore_set (&args.original_signal_mask); - /* All l_nodelete_pending objects should have been deleted at this point, which is why it is not necessary to reset the flag here. */ } - else - __libc_signal_restore_set (&args.original_signal_mask); assert (_dl_debug_initialize (0, args.nsid)->r_state == RT_CONSISTENT);