Re: proof for dlmem() (Re: [PATCH v9 0/13] implement dlmem() function)

[stsp <stsp2@yandex.ru>]

[-- Attachment #1: Type: text/plain, Size: 10381 bytes --]

Hi Zack,

02.05.2023 04:11, Zack Weinberg пишет:
> This discussion seems to have died down in the past two weeks but I'm
> going to make one last attempt to bridge the divide.  I would hate to
> see everyone walk away from this with a bad taste in their mouth.  Note
> cc: list aggressively pruned.

The discussion have died down because I
went implementing the RTLD_NORELOCATE
proposal - a replacement to the dlmem()'s
one. Currently its almost completed and I
will draft its spec at the bottom of this e-mail.


> Here's the thing: stsp, no one is intentionally trying to spread lies
> about your patch.  You have misunderstood Adhemerval here.  He was *not*
> saying that *your patch* required the caller to parse ELF headers
> themselves.  He was saying that your patch does a *different*
> unacceptable thing -- running user-supplied code with the loader lock
> held,

Firstly, let me note that we are mis-communicating.
I haven't received your very first e-mail on that
topic (Carlos told me later about it), and you seem
to have not received any (of many) mails I sent
to you both privately and on-list.
So please confirm the communication is OK now.

Now let me clarify.
Running a call-back with the lock held, was
the problem spelled out by Szabolcs - the only
person who did an actual review. That immediately
set me back to the scratch-pads, and I'll soon
introduce the RTLD_NORELOCATE proposal that
doesn't have that problem. So let me put that
straightly: no single problem was ever ignored
or misunderstood on my side. The valid problems
pointed out by Szabolcs, were discussed and
will be addresses (as I told you many times in
the private e-mails, but it seems you received
none of them).

Unfortunately there was also an activity on
"punishing me" running in parallel by others.
It started from this e-mail:
https://inbox.sourceware.org/libc-alpha/b6a96687-9a4b-414f-849a-45a305898274@redhat.com/
It does this strange claim:
---

To implement fdlopen on top of dlmem requires PT_LOAD processing and that
will duplicate into userspace a significant part of the complexity of ELF
loading and segment handling. The test case you include below is incomplete
when it comes to implementing a functional fdlopen() across the supported
architectures and toolchain variants.

---

To address that (and a few other valid problems)
I clarified in my spec that the mmap'ed buffer
should not be changed before passing to dlmem().
Then there came this e-mail from Jonathon:

https://inbox.sourceware.org/libc-alpha/630fa17528c6050d60f524aa88ad5a057cae1603.camel@rice.edu/

---

AFAICT, the first glaring issue with both of your implementations is
that you have neglected the case where p_offset != p_vaddr, i.e. a
segment is mmapped to a different location than its layout in the file.
There are a LOT of binaries out in the wild where this is the case.
Here's a quick one-liner to help you find some on your own box, I
have 11712 such binaries on my Debian system:

---

To address that (obviously false) claim, I posted
this proof:
https://inbox.sourceware.org/libc-alpha/d8c82663-d427-e03c-109c-95f6095ce6d6@yandex.ru/
and later this proof:
https://inbox.sourceware.org/libc-alpha/3f636504-818d-6520-6cf3-484503a8703c@yandex.ru/

After that, I explicitly asked Carlos many
times, both privately and on-list, whether
my proofs were sufficient, or should I
disprove more false claims that directly
contradict with my spec draft, to start with:
https://inbox.sourceware.org/libc-alpha/b2336861-9a01-c5ab-0d78-99ea3f920824@yandex.ru/

Carlos never replied to either of these
queries. And when Szabolcs started to
actually discuss my proposal (rather some
false claims around it), Adhemerval provoked
the conflict (by repeating the aforementioned
disproven claims indefinitely) with the only
goal of making Szabolcs to stop reviewing my
proposal! Fortunately that was unneeded at
that stage, as I was already set back to
scratches by an existing Szabolcs's review.


>   if I understood correctly -- and then he was trying to warn you
> that any *alternative* patch you might come up with would *also* be
> unacceptable if it required the caller to parse the DLL themselves.

As shown above, that applied to my existing
proposal, not to some future one:
https://inbox.sourceware.org/libc-alpha/b6a96687-9a4b-414f-849a-45a305898274@redhat.com/
This mail is very clear on what proposal is
concerned. And there was never a reason
to expect that any future proposal will include
that  problem.

Note that all proves I was posting, were just
silently ignored and the false statement were
repeated indefinitely:
https://inbox.sourceware.org/libc-alpha/a58c0c8c-4117-1a94-7285-40ba18555548@yandex.ru/
This mail is just one of many, many examples
where I asked to stop blindly repeating and
look at least at what proofs I provide. Eventually
it was summarized by Rich Felker this way:
https://inbox.sourceware.org/libc-alpha/20230331145803.GB3298@brightrain.aerifal.cx/

So as you can see, as surprising as it might
seem, it all was just a big campaign on punishing
me for not dropping the patches down. :(

> Do you understand the difference between what you thought Adhemerval was
> saying, and what I am saying he was trying to communicate?  Please
> confirm, or else describe why you don't see a difference so I can try to
> clarify again.

You can only try to clarify that, if you read
the whole thread. Its not possible to clarify
out-of-context. And once you read the whole
thread, you'll have a major difficulties in
clarifying what was done here. :(
I have never had to run through such a level
of abuse in any free-software community.
But it all doesn't matter.
What matters is to come up with the good
proposal, which is what I am working on now
and then, with a hope other people can became
more tolerant to a newcomers that only wanted
to contribute.

> Now. Another thing you must understand before you send in any further
> revisions of the patch is that glibc is extremely reluctant to add new
> APIs.  Witness how much we resisted adding strlcpy, a single function
> with a clear specification and no interdependencies with any other
> part of the C library.  This is why you are getting what probably
> seems to you like an absurd level of negative feedback.  We aren't
> trying to deny that you have a legitimate need, and we aren't saying
> you haven't managed to come up with a proof of concept implementation
> of how that need could be solved -- and yet we ARE saying, no, still
> not good enough.

The explanation of "not good enough" should
also be provided. Well, it was done by Szabolcs,
and so I am on the drawing boards and scratch
pads again.
Or alternatively glibc can add hooks needed
for an external libdl, and not accept any new
libdl-related API after that.
Or alternatively it should be possible to link
with the custom-patched static libc, and put
the resulting binary into a separate namespace.
I was working also in that direction and had
some progress, but I need to understand the
glibc's tls impl better. Overall that route seems
possible too.

Now to your proposal:

> ---
>
> Having said all that, I can imagine other uses for dlmem() or something like it, besides your specific program, so let me try to meet you in the middle here.  Suppose we gave you *this* API:
>
> /** Report the total amount of virtual address space required in order to load
>   *  the shared object referred to by FD.  FD must be a readable and seekable
>   *  file descriptor.  Returns a whole number of pages, or zero on error
>   *  (in which case errno will provide details).
>   */
> size_t dl_total_vsize(int fd);
>
> /** Load the shared object referred to by FD into this process's address
>   *  space.  FD must be a readable and seekable file descriptor.
>   *
>   *  If LOAD_ADDR is not NULL, it specifies the address at which to load
>   *  the shared object.  If this is not possible, for instance if there
>   *  are already memory mappings in the range from LOAD_ADDR to LOAD_ADDR
>   *  plus dl_total_vsize(FD), the operation will fail.  LOAD_ADDR must be
>   *  a multiple of the system page size.
>   *
>   *  MODE accepts the same set of RTLD_* flags as are documented for dlopen(),
>   *  plus one more: RTLD_READ directs the dynamic loader to read the shared
>   *  object into the process's address space as-if with read() rather than
>   *  with mmap().  When this flag is used, LOAD_ADDR may not be NULL, and must
>   *  already point to at least dl_total_vsize(FD) bytes of writable memory.
>   *  The dynamic loader will take ownership of that range of addresses and
>   *  may subsequently (e.g. upon a call to dlclose()) deallocate them as-if
>   *  with munmap().  Furthermore, the dynamic loader will change virtual
>   *  memory protection settings for sub-ranges of this space, so that
>   *  (for instance) the text segment of the shared object is read-only,
>   *  as-if by calling mprotect().
>   */
> void *dl_fdopen(int fd, int mode, void *load_addr);
>
> What would still be missing for your use case?
What's missing here is an ability to load the
library into an existing mapping without overwriting
it. This feature is needed so that the library can
be duplicated from "reloc_addr" to
"reloc_addr+VM_window_start".
So your proposal only adds 1 feature from the
needed 2.

The proposal I am working on now, looks like this:
RTLD_NORELOCATE flag defers an object relocation
upon the use of dlsym(), or an explicitly called
new dlrelocate() fn.
RTLD_DI_MAPINFO is a new dlinfo() cmd that
fills in this struct:

+typedef struct
+{
+  void *map_start;             /* Beginning of mapping containing 
address.  */
+  size_t map_length;           /* Length of mapping.  */
+  int relocated;               /* Indicates whether an object was 
relocated. */
+} Dl_mapinfo;

After getting this struct, the user can
just memcpy() the unrelocated image
where he wants it to be, and then call
dlset_object_address(handle, address);
to update the link-map.
Then he can either call dlrelocate()
or not do anything, in which case dlsym()
will relocate.
That seems to be a very flexible and small
extension: as it is up to the user to move
an object, almost no new code needs to be
added to glibc. All patches I now have, are
within the 200lines margin.

Do you know if such extension is acceptable?