Hi Linus, > On Apr 21, 2022, at 3:30 PM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, Apr 21, 2022 at 2:53 PM Song Liu <songliubraving@xxxxxx> wrote: >> >> However, we cannot really use the same function at free time. The >> huge page is RO+X at free time, but we are only zeroing out a chunk >> of it. So regular memset/memcpy won’t work. Instead, we will need >> something like bpf_arch_text_copy(). > > I actually think bpf_arch_text_copy() is another horribly badly done thing. > > It seems only implemented on x86 (I'm not sure how anything else is > supposed to work, I didn't go look), and there it is horribly badly > done, using __text_poke() that does all these magical things just to > make it atomic wrt concurrent code execution. > > None of which is *AT*ALL* relevant for this case, since concurrent > code execution simply isn't a thing (and if it were, you would already > have lost). > > And if that wasn't pointless enough, it does all that magic "map the > page writably at a different virtual address using poking_addr in > poking_mm" and a different address space entirely. > > All of that is required for REAL KERNEL CODE. > > But the thing is, for bpf_prog_pack, all of that is just completely > pointless and stupid complexity. > > We already *have* the other non-executable address that is writable: > it's the actual pages that got vmalloc'ed. Just use vmalloc_to_page() > and it's RIGHT THERE. I think this won’t work, as set_memory_ro makes all the aliases of these pages read only. We can probably add set_memory_ro_noalias(), which will be similar to set_memory_np_noalias(). This approach was NACKed by Peter[1]. So we went with the bpf_arch_text_copy approach. If we can loosen the W^X requirement for BPF programs, other parts of bpf_prog_pack could also be simplified. Thanks, Song [1] https://lore.kernel.org/netdev/20211116080051.GU174703@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > At which point you just use the same bpf_jit_fill_hole() function, and > you're done. > > In other words, all of this seems excessively stupidly done, for no > good reason. It's only making it much too complicated, and just not > doing the right thing at all. > > Linus
