On Fri, Oct 07, 2022 at 04:43:11PM -0700, Song Liu wrote:
> Changes RFC v1 => RFC v2:
> 1. Major rewrite of the logic of vmalloc_exec and vfree_exec. They now
> work fine with BPF programs (patch 1, 2, 4). But module side (patch 3)
> still need some work.
Can you please move the changelog under the description of WTF the
series actually does like the normal kernel process? Explaining the
changes from a previous version before you even describe what the series
does is completely incoherent.
> This set is a prototype that allows dynamic kernel text (modules, bpf
> programs, various trampolines, etc.) to share huge pages. The idea is
> similar to Peter's suggestion in [1]. Please refer to each patch for
> more detais.
Well, nothing explains what the method is to avoid having memory
that is mapped writable and executable at the same time, which really
could use some explanation here (and in the main patch as well).
Hi Chritoph,
> On Oct 17, 2022, at 12:26 AM, Christoph Hellwig <[email protected]> wrote:
>
> On Fri, Oct 07, 2022 at 04:43:11PM -0700, Song Liu wrote:
>> Changes RFC v1 => RFC v2:
>> 1. Major rewrite of the logic of vmalloc_exec and vfree_exec. They now
>> work fine with BPF programs (patch 1, 2, 4). But module side (patch 3)
>> still need some work.
>
> Can you please move the changelog under the description of WTF the
> series actually does like the normal kernel process? Explaining the
> changes from a previous version before you even describe what the series
> does is completely incoherent.
Will fix in the next version.
>
>> This set is a prototype that allows dynamic kernel text (modules, bpf
>> programs, various trampolines, etc.) to share huge pages. The idea is
>> similar to Peter's suggestion in [1]. Please refer to each patch for
>> more detais.
>
> Well, nothing explains what the method is to avoid having memory
> that is mapped writable and executable at the same time, which really
> could use some explanation here (and in the main patch as well).
Thanks for the feedback. I will add this.
Does the code look good to you? I personally think patch 1, 2, 4 could
ship with a little more work.
Thanks,
Song
On Mon, Oct 17, 2022 at 04:23:52PM +0000, Song Liu wrote:
> > Well, nothing explains what the method is to avoid having memory
> > that is mapped writable and executable at the same time, which really
> > could use some explanation here (and in the main patch as well).
>
> Thanks for the feedback. I will add this.
>
> Does the code look good to you? I personally think patch 1, 2, 4 could
> ship with a little more work.
I only took a quick look and I'm not sure how the W^X actually works.
Yes, it alls into the text poke helpers, but how do these work on
less than page sized allocations?
> On Oct 18, 2022, at 7:50 AM, Christoph Hellwig <[email protected]> wrote:
>
> On Mon, Oct 17, 2022 at 04:23:52PM +0000, Song Liu wrote:
>>> Well, nothing explains what the method is to avoid having memory
>>> that is mapped writable and executable at the same time, which really
>>> could use some explanation here (and in the main patch as well).
>>
>> Thanks for the feedback. I will add this.
>>
>> Does the code look good to you? I personally think patch 1, 2, 4 could
>> ship with a little more work.
>
> I only took a quick look and I'm not sure how the W^X actually works.
> Yes, it alls into the text poke helpers, but how do these work on
> less than page sized allocations?
Aha, I guess I understand your point (and concern) now.
It is the same as text poke into static kernel text: we create a local
writable mapping to the memory we need to update. For less than page
sized allocation, this mapping does have access to X memory that may
belong to a different allocation, just like text poke into static
kernel text.
Maybe we need something like vcopy_exec(x_mem, tmp_buf, size), where
we explicitly check the allowed memory of x_mem is bigger or equal to
size. And users of vmalloc_exec should only use vcopy_exec to update
memory from vmalloc_exec.
Does this make sense? Did I understand your concern correctly?
Thanks,
Song