Re: [PATCH 06/15] elf: Fix comments and logic in _dl_add_to_slotinfo

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 04/02/2021 17:50, Adhemerval Zanella via Libc-alpha wrote:
> On 15/02/2021 08:59, Szabolcs Nagy via Libc-alpha wrote:
> > From: Szabolcs Nagy <szabolcs dot nagy at arm dot com>
> > 
> > Since
> > 
> >   commit a509eb117fac1d764b15eba64993f4bdb63d7f3c
> >   Avoid late dlopen failure due to scope, TLS slotinfo updates [BZ #25112]
> > 
> > the generation counter update is not needed in the failure path.
> 
> It is not clear to me from just the commit reference why it would
> be safe to remove the GL(dl_tls_generation) update on 
> _dl_add_to_slotinfo.
> 
> The dl_open_worker calls update_tls_slotinfo which in turn call
> might call _dl_add_to_slotinfo *after* the demarcation point.  Will
> it terminate the process?

in that commit the logic got changed such that allocations
happen before the demarcation point in resize_tls_slotinfo.

this is the reason for the do_add bool argument in
_dl_add_to_slotinfo: it's called twice and the first call
with do_add==false is only there to ensure everything is
allocated before the demarcation point (so module loading
can be reverted, no need to bump the generation count).

i guess this is not visible by just looking at the
_dl_add_to_slotinfo code.

note that adding some asserts to ensure there is no allocation
when do_add==true does not work: rtld uses the same api, but
without the do_add==false preallocation step since at startup
time allocation failure is fatal anyway.

> > ---
> >  elf/dl-tls.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> > 
> > diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> > index 79b93ad91b..24d00c14ef 100644
> > --- a/elf/dl-tls.c
> > +++ b/elf/dl-tls.c
> > @@ -998,16 +998,7 @@ _dl_add_to_slotinfo (struct link_map *l, bool do_add)
> >  		+ TLS_SLOTINFO_SURPLUS * sizeof (struct dtv_slotinfo));
> >        if (listp == NULL)
> >  	{
> > -	  /* We ran out of memory.  We will simply fail this
> > -	     call but don't undo anything we did so far.  The
> > -	     application will crash or be terminated anyway very
> > -	     soon.  */
> > -
> > -	  /* We have to do this since some entries in the dtv
> > -	     slotinfo array might already point to this
> > -	     generation.  */
> > -	  ++GL(dl_tls_generation);
> > -
> > +	  /* We ran out of memory while resizing the dtv slotinfo list.  */
> >  	  _dl_signal_error (ENOMEM, "dlopen", NULL, N_("\
> >  cannot create TLS data structures"));
> >  	}