On Fri, Feb 2, 2024 at 10:52 AM Pedro Falcato <pedro.falcato@xxxxxxxxx> wrote: > > On Fri, Feb 2, 2024 at 5:59 PM Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote: > > > > On Thu, Feb 1, 2024 at 9:00 PM Theo de Raadt <deraadt@xxxxxxxxxxx> wrote: > > > > > > Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote: > > > > > > > Even without free. > > > > I personally do not like the heap getting sealed like that. > > > > > > > > Component A. > > > > p=malloc(4096); > > > > writing something to p. > > > > > > > > Compohave nent B: > > > > mprotect(p,4096, RO) > > > > mseal(p,4096) > > > > > > > > This will split the heap VMA, and prevent the heap from shrinking, if > > > > this is in a frequent code path, then it might hurt the process's > > > > memory usage. > > > > > > > > The existing code is more likely to use malloc than mmap(), so it is > > > > easier for dev to seal a piece of data belonging to another component. > > > > I hope this pattern is not wide-spreading. > > > > > > > > The ideal way will be just changing the library A to use mmap. > > > > > > I think you are lacking some test programs to see how it actually > > > behaves; the effect is worse than you think, and the impact is immediately > > > visible to the programmer, and the lesson is clear: > > > > > > you can only seal objects which you gaurantee never get recycled. > > > > > > Pushing a sealed object back into reuse is a disasterous bug. > > > > > > Noone should call this interface, unless they understand that. > > > > > > I'll say again, you don't have a test program for various allocators to > > > understand how it behaves. The failure modes described in your docuemnts > > > are not correct. > > > > > I understand what you mean: I will add that part to the document: > > Try to recycle a sealed memory is disastrous, e.g. > > p=malloc(4096); > > mprotect(p,4096,RO) > > mseal(p,4096) > > free(p); > > > > My point is: > > I think sealing an object from the heap is a bad pattern in general, > > even dev doesn't free it. That was one of the reasons for the sealable > > flag, I hope saying this doesn't be perceived as looking for excuses. > > The point you're missing is that adding MAP_SEALABLE reduces > composability. With MAP_SEALABLE, everything that mmaps some part of > the address space that may ever be sealed will need to be modified to > know about MAP_SEALABLE. > > Say you did the same thing for mprotect. MAP_PROTECT would control the > mprotectability of the map. You'd stop: > > p = malloc(4096); > mprotect(p, 4096, PROT_READ); > free(p); > > ! But you'd need to change every spot that mmap()'s something to know > about and use MAP_PROTECT: all "producers" of mmap memory would need > to know about the consumers doing mprotect(). So now either all mmap() > callers mindlessly add MAP_PROTECT out of fear the consumers do > mprotect (and you gain nothing from MAP_PROTECT), or the mmap() > callers need to know the consumers call mprotect(), and thus you > introduce a huge layering violation (and you actually lose from having > MAP_PROTECT). > > Hopefully you can map the above to MAP_SEALABLE. Or to any other m*() > operation. For example, if chrome runs on an older glibc that does not > know about MAP_SEALABLE, it will not be able to mseal() its own shared > libraries' .text (even if, yes, that should ideally be left to ld.so). > I think I have heard enough complaints about MAP_SEALABLE from Linux developers and Linus in the last two days to convince myself that it is a bad idea :) For the last time, I was trying to limit the scope of mseal() limited to two known cases. And MAP_SEALABLE is a reversible decision, a system ctrl can turn it off, or we can obsolete it in future. (this was mentioned in the document of V8). I will rest my case. Obviously from the feedback, it is loud and clear that we want to be able to seal all the memory. > IMO, UNIX API design has historically mostly been "play stupid games, > win stupid prizes", which is e.g: why things like close(STDOUT_FILENO) > work. If you close stdout (and don't dup/reopen something to stdout) > and printf(), things will break, and you get to keep both pieces. > There's no O_CLOSEABLE, just as there's no O_DUPABLE. > > -- > Pedro
