On Tue, 2018-12-04 at 16:01 -0800, Nadav Amit wrote: > > On Dec 4, 2018, at 3:51 PM, Edgecombe, Rick P <rick.p.edgecombe@xxxxxxxxx> > > wrote: > > > > On Tue, 2018-12-04 at 12:36 -0800, Nadav Amit wrote: > > > > On Dec 4, 2018, at 12:02 PM, Edgecombe, Rick P < > > > > rick.p.edgecombe@xxxxxxxxx> > > > > wrote: > > > > > > > > On Tue, 2018-12-04 at 16:03 +0000, Will Deacon wrote: > > > > > On Mon, Dec 03, 2018 at 05:43:11PM -0800, Nadav Amit wrote: > > > > > > > On Nov 27, 2018, at 4:07 PM, Rick Edgecombe < > > > > > > > rick.p.edgecombe@xxxxxxxxx> > > > > > > > wrote: > > > > > > > > > > > > > > Since vfree will lazily flush the TLB, but not lazily free the > > > > > > > underlying > > > > > > > pages, > > > > > > > it often leaves stale TLB entries to freed pages that could get > > > > > > > re- > > > > > > > used. > > > > > > > This is > > > > > > > undesirable for cases where the memory being freed has special > > > > > > > permissions > > > > > > > such > > > > > > > as executable. > > > > > > > > > > > > So I am trying to finish my patch-set for preventing transient W+X > > > > > > mappings > > > > > > from taking space, by handling kprobes & ftrace that I missed > > > > > > (thanks > > > > > > again > > > > > > for > > > > > > pointing it out). > > > > > > > > > > > > But all of the sudden, I don’t understand why we have the problem > > > > > > that > > > > > > this > > > > > > (your) patch-set deals with at all. We already change the mappings > > > > > > to > > > > > > make > > > > > > the memory writable before freeing the memory, so why can’t we make > > > > > > it > > > > > > non-executable at the same time? Actually, why do we make the module > > > > > > memory, > > > > > > including its data executable before freeing it??? > > > > > > > > > > Yeah, this is really confusing, but I have a suspicion it's a > > > > > combination > > > > > of the various different configurations and hysterical raisins. We > > > > > can't > > > > > rely on module_alloc() allocating from the vmalloc area (see nios2) > > > > > nor > > > > > can we rely on disable_ro_nx() being available at build time. > > > > > > > > > > If we *could* rely on module allocations always using vmalloc(), then > > > > > we could pass in Rick's new flag and drop disable_ro_nx() altogether > > > > > afaict -- who cares about the memory attributes of a mapping that's > > > > > about > > > > > to disappear anyway? > > > > > > > > > > Is it just nios2 that does something different? > > > > > > > > > > Will > > > > > > > > Yea it is really intertwined. I think for x86, set_memory_nx everywhere > > > > would > > > > solve it as well, in fact that was what I first thought the solution > > > > should > > > > be > > > > until this was suggested. It's interesting that from the other thread > > > > Masami > > > > Hiramatsu referenced, set_memory_nx was suggested last year and would > > > > have > > > > inadvertently blocked this on x86. But, on the other architectures I > > > > have > > > > since > > > > learned it is a bit different. > > > > > > > > It looks like actually most arch's don't re-define set_memory_*, and so > > > > all > > > > of > > > > the frob_* functions are actually just noops. In which case allocating > > > > RWX > > > > is > > > > needed to make it work at all, because that is what the allocation is > > > > going > > > > to > > > > stay at. So in these archs, set_memory_nx won't solve it because it will > > > > do > > > > nothing. > > > > > > > > On x86 I think you cannot get rid of disable_ro_nx fully because there > > > > is > > > > the > > > > changing of the permissions on the directmap as well. You don't want > > > > some > > > > other > > > > caller getting a page that was left RO when freed and then trying to > > > > write > > > > to > > > > it, if I understand this. > > > > > > > > The other reasoning was that calling set_memory_nx isn't doing what we > > > > are > > > > actually trying to do which is prevent the pages from getting released > > > > too > > > > early. > > > > > > > > A more clear solution for all of this might involve refactoring some of > > > > the > > > > set_memory_ de-allocation logic out into __weak functions in either > > > > modules > > > > or > > > > vmalloc. As Jessica points out in the other thread though, modules does > > > > a > > > > lot > > > > more stuff there than the other module_alloc callers. I think it may > > > > take > > > > some > > > > thought to centralize AND make it optimal for every > > > > module_alloc/vmalloc_exec > > > > user and arch. > > > > > > > > But for now with the change in vmalloc, we can block the executable > > > > mapping > > > > freed page re-use issue in a cross platform way. > > > > > > Please understand me correctly - I didn’t mean that your patches are not > > > needed. > > > > Ok, I think I understand. I have been pondering these same things after > > Masami > > Hiramatsu's comments on this thread the other day. > > > > > All I did is asking - how come the PTEs are executable when they are > > > cleared > > > they are executable, when in fact we manipulate them when the module is > > > removed. > > > > I think the directmap used to be RWX so maybe historically its trying to > > return > > it to its default state? Not sure. > > > > > I think I try to deal with a similar problem to the one you encounter - > > > broken W^X. The only thing that bothered me in regard to your patches (and > > > only after I played with the code) is that there is still a time-window in > > > which W^X is broken due to disable_ro_nx(). > > > > Totally agree there is overlap in the fixes and we should sync. > > > > What do you think about Andy's suggestion for doing the vfree cleanup in > > vmalloc > > with arch hooks? So the allocation goes into vfree fully setup and vmalloc > > frees > > it and on x86 resets the direct map. > > As long as you do it, I have no problem ;-) > > You would need to consider all the callers of module_memfree(), and probably > to untangle at least part of the mess in pageattr.c . If you are up to it, > just say so, and I’ll drop this patch. All I can say is “good luck with all > that”. > I thought you were trying to prevent having any memory that at any time was W+X, how does vfree help with the module load time issues, where it starts WRX on x86?
