[DTrace-devel] [PATCH 1/2] dt_link: retry elf_update() without mmaps if writing fails

[Steven Sistare]

On 5/23/2023 12:26 PM, Steven Sistare wrote:
> On 5/22/2023 6:24 PM, Nick Alcock wrote:
>> elfutils has an option ELF_C_RDWR_MMAP which uses mmap() to update ELF
>> files, which dtrace uses in dtrace -G's process_obj().  This is usually
>> desirable because most of the file is not altered, so the writeout is
>> (for large files, much) faster than doing anything else (it's even
>> faster than the update-in-place which elfutils otherwise does).
>> Unfortunately, elfutils calls mremap() to expand the file if needed, and
>> does not pass MREMAP_MAYMOVE, probably because it maintains pointers
>> into the ELF buffers and cannot cope with them moving.
>>
>> Unfortunately this failure happens only at elf_update() time, which
>> means that process_obj() finds out that it has to reopen a file without
>> mmap only when it's about ready to close it.  Adapt to this by havinb
>> process_obj() reinvoke itself when this error is found, forcing the
>> non-use of mmap() in this case.  Fortunately the link operation is
>> almost entirely idempotent: everything we do is either done to the ELF
>> file (and thus thrown away when elf_update() fails) or is allocations
>> that are discarded as process_obj() exits, or is dependent on the ELF
>> file (like offsets, etc).  So we don't need to reset the eprobesp
>> boolean parameter (if it was changed before, it will be changed
>> identically again).  The only thing we must suppress is the call to
>> dt_probe_define(), which actually does the DOF-creation work: that is
>> tied to the probe definition and dtrace handle, and thus is not
>> discarded when process_obj() ends (indeed, we need it not to be, because
>> that's what creates the data that dtrace_dof_create() writes out).
>>
>> But we know that calls in a subsequent process_obj() invocation on the
>> same file will be identical to the calls that already happened: it's
>> just that dt_probe_define() doesn't check for existing offsets.  Maybe
>> it should, but doing so efficiently is quite a lot more work than just
>> noting that this is a reinvocation of process_obj() and skipping the
>> define on the grounds that it already happened.
>>
>> Orabug: 35416271
>> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> 
> Just a heads up, I applied this patch and dlink has munged the file that previously
> failed with ELF_C_RDWR_MMAP.
> 
> /usr/bin/ld: nbd/server.o: attempt to load strings from a non-string section (number 35)
> nbd/server.o: file not recognized: Bad value
> 
> I will debug it.

The problem is that libelf writes to the object before failing, corrupting it.  Using
the un-patched dtrace:

$ cksum nbd/server.o
1545565795 548696 nbd/server.o
$ dtrace -G -64 -s all-probes.dtrace nbd/server.o
dtrace: failed to link script all-probes.dtrace: an error was encountered while processing nbd/server.o
$ cksum nbd/server.o
545777071 549632 nbd/server.o

To fix, we would need to proactively make a backup copy of the object, to be restored 
before retrying.  Which defeats the purpose of ELF_C_RDWR_MMAP.

So, back to supplying an env var or command-line option to disable ELF_C_RDWR_MMAP?

- Steve