libdtrace symbol map [Was: RTLD_DEEPBIND question]

On 16/05/2025 17:11, Mark Johnston wrote:
> On Fri, May 16, 2025 at 03:22:52PM +0300, Andriy Gapon wrote:
>> On 27/04/2025 17:26, Mark Johnston wrote:
>>> On Thu, Apr 24, 2025 at 08:56:44AM +0300, Andriy Gapon wrote:
>>>> On 23/04/2025 21:56, Andriy Gapon wrote:
>>>>> BTW, I've been wondering how illumos avoids the problem even though they
>>>>> do not use any special dlopen flags.
>>>>> It turns out that they link almost all system shared libraries with
>>>>> -Bdirect option (which is Solaris/illumos specific).
>>>>> It's somewhat similar to, but different from, -Bsymbolic.
>>>>> https://docs.oracle.com/cd/E23824_01/html/819-0690/aehzq.html#scrolltoc
>>>>> https://docs.oracle.com/cd/E36784_01/html/E36857/gejfe.html
>>>>
>>>> Oh, and it looks like there is an even better explanation for illumos.
>>>> There is a version map file for libdtrace which explicitly lists API
>>>> functions and makes everything else local.
>>>> https://github.com/illumos/illumos-gate/blob/master/usr/src/lib/libdtrace/common/mapfile-vers
>>>>
>>>> I wonder why we didn't do the same when porting.
>>>> Maybe we should do that now?
>>>
>>> I don't have any objection, but I believe adding a version map after the
>>> fact doesn't accomplish much, assuming that we care deeply about ABI
>>> stability in libdtrace.so (I'm not sure we do though).
>>
>> My primary goal here is not ABI stability, but hiding symbols that really
>> should not be exported.  See more at the end.
>>
>> At the same time I am not sure why it could be too late to start caring
>> about ABI stability now.  Assuming we actually would want to do that.
> 
> I just mean that the version map helps only helps provide stability for
> binaries linked to libdtrace.so after the version map is introduced.

So, if we introduce it (and bump the library version as kib pointed out), then 
eventually it becomes useful (like it did for all other libraries that we 
versioned).

>> And I don't want to single out libtrace here.
>> It seems that the story is the same for all libraries that have been ported
>> from illumos.
>> E.g., libzfs_core was supposed to be a library that cares greatly about its
>> API / ABI stability.
>>
>>>>> I think that on FreeBSD we should use symbol visibility attributes or a
>>>>> symbol map to hide (make local) symbols that are not expected to be
>>>>> interposed or have a high chance to be interposed by accident.
>>>>>
>>>>> IMO, yyparse should definitely get that treatment.
>>>>>
>>>>> I think that approach would be better than magic rtld tricks.
>>>>> Especially because the tricks do not work with the current rtld.
>>>>> I'd rather make a change to libdtrace.so than to rtld.
>>>>
>>>> This, while not as nice as the illumos solution, fixes my specific issue:
>>>> diff --git a/cddl/lib/libdtrace/Makefile b/cddl/lib/libdtrace/Makefile
>>>> index d086fffb07bc..58054d129b49 100644
>>>> --- a/cddl/lib/libdtrace/Makefile
>>>> +++ b/cddl/lib/libdtrace/Makefile
>>>> @@ -146,7 +146,8 @@ CFLAGS+=    -fsanitize=address -fsanitize=undefined
>>>>    LDFLAGS+=      -fsanitize=address -fsanitize=undefined
>>>>    .endif
>>>>
>>>> -LIBADD=        ctf elf proc pthread rtld_db xo
>>>> +VERSION_MAP=   ${.CURDIR}/Symbol.map
>>>> +LIBADD=                ctf elf proc pthread rtld_db xo
>>>>
>>>>    CLEANFILES=    dt_errtags.c dt_names.c
>>>>
>>>> diff --git a/cddl/lib/libdtrace/Symbol.map b/cddl/lib/libdtrace/Symbol.map
>>>> new file mode 100644
>>>> index 000000000000..89ee9de65209
>>>> --- /dev/null
>>>> +++ b/cddl/lib/libdtrace/Symbol.map
>>>> @@ -0,0 +1,4 @@
>>>> +{
>>>> +       local:
>>>> +               yy*;
>>>> +};
>>>
>>> This just gives the lexer/parser symbols in libdtrace.so local
>>> visibility?  I think that's fine.
>> Yes, that's the intention.
>>
>> I tested locally two versions of Symbol.map for libdtrace.
>> The basic one quoted here and a more extended one based on illumos
>> lib/libdtrace/common/mapfile-vers.
>> The latter version does not define any symbol versions, its purpose is only
>> to be a white-list of things to make public / global:
>> https://people.freebsd.org/~avg/libdtrace-Symbol.map
> 
> Do we really want to export _dtrace_debug?

Hmm, I don't know, that came from illumos.
However, I do not see any references to _dtrace_debug outside of libdtrace 
neither in FreeBSD nor in illumos.
So, I guess that it can be removed.

Maybe it was exposed for potential libdtrace consumers that might want to enable 
debug directly rather than via environment.

>> Comparing to illumos I only had to add 3 dtrace_oformat* symbols,
>>
>> Both versions worked equally well in my tests, but maybe I missed more of
>> FreeBSD extensions.
>>
>> Which one would be better to get into the tree?
> 
> Having a full whitelist seems preferable to me.  Did you test lockstat
> as well?  I believe it and dtrace(8) are the only users of libdtrace.so
> in the base system.
I didn't before, I've just done now, it works.
I'll post a review request over the weekend (hopefully).

LD_PRELOAD=/usr/obj/amd64.amd64/cddl/lib/libdtrace/libdtrace.so.2 lockstat -H -D 
10 -x aggsize=100m -l smp_ipi_mtx -s 20 -P sleep 5

Spin lock hold: 106 events in 5.021 seconds (21 events/sec)

-------------------------------------------------------------------------------
Count indv cuml rcnt     nsec Lock                   Caller
    11  53%  53% 0.00    99541 smp rendezvous         __mtx_unlock_spin_flags+0xe3

       nsec ------ Time Distribution ------ count     Stack
      16384 |@@                             1         smp_rendezvous_cpus+0x187
      32768 |@@@@@@@@@@@@@@@@@@@            7         dtrace_sync+0x39
      65536 |                               0         dtrace_state_deadman+0x13
     131072 |                               0         softclock_call_cc+0x1d9
     262144 |@@                             1         softclock_thread+0xc6
     524288 |@@@@@                          2         fork_exit+0x82
                                                      0xffffffff81030a3e
-------------------------------------------------------------------------------
...

-- 
Andriy Gapon