2022-07-13 10:49:39

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH bpf-next 1/3] mm/vmalloc: introduce vmalloc_exec which allocates RO+X memory

NAK. This is not something that should be an exported public API
ever.


2022-07-13 16:12:45

by Song Liu

[permalink] [raw]
Subject: Re: [PATCH bpf-next 1/3] mm/vmalloc: introduce vmalloc_exec which allocates RO+X memory



> On Jul 13, 2022, at 3:08 AM, Christoph Hellwig <[email protected]> wrote:
>
> NAK. This is not something that should be an exported public API
> ever.

Hmm.. I will remove EXPORT_SYMBOL_GPL (if we ever do a v2 of this..)

Thanks,
Song

2022-07-14 05:00:15

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH bpf-next 1/3] mm/vmalloc: introduce vmalloc_exec which allocates RO+X memory

On Wed, Jul 13, 2022 at 03:49:45PM +0000, Song Liu wrote:
>
>
> > On Jul 13, 2022, at 3:08 AM, Christoph Hellwig <[email protected]> wrote:
> >
> > NAK. This is not something that should be an exported public API
> > ever.
>
> Hmm.. I will remove EXPORT_SYMBOL_GPL (if we ever do a v2 of this..)

Even without that it really is not a vmalloc API anyway. Executable
memory needs to be written first, so we should allocate it in that state
and only mark it executable after that write has completed.

2022-07-14 05:38:16

by Song Liu

[permalink] [raw]
Subject: Re: [PATCH bpf-next 1/3] mm/vmalloc: introduce vmalloc_exec which allocates RO+X memory



> On Jul 13, 2022, at 9:23 PM, Christoph Hellwig <[email protected]> wrote:
>
> On Wed, Jul 13, 2022 at 03:49:45PM +0000, Song Liu wrote:
>>
>>
>>> On Jul 13, 2022, at 3:08 AM, Christoph Hellwig <[email protected]> wrote:
>>>
>>> NAK. This is not something that should be an exported public API
>>> ever.
>>
>> Hmm.. I will remove EXPORT_SYMBOL_GPL (if we ever do a v2 of this..)
>
> Even without that it really is not a vmalloc API anyway.

This ...

> Executable
> memory needs to be written first, so we should allocate it in that state
> and only mark it executable after that write has completed.

... and this are two separate NAKs.

For the first NAK, I agree that my version is another layer on top of
vmalloc. But what do you think about Peter's idea? AFAICT, that fits
well in vmalloc logic.

For the second NAK, I acknowledge the concern. However, I think we
will need some mechanism to update text without flipping W and X bit in
the page table. Otherwise, set_memory_* w/ alias will fragment the
direct map, and cause significant performance drop over time. If this
really doesn't work because of this concern, we will need to look into
other solutions discussed in LSFMMBPF [1].

Thanks,
Song
[1] https://lwn.net/Articles/894557/


2022-07-14 18:28:40

by Uladzislau Rezki

[permalink] [raw]
Subject: Re: [PATCH bpf-next 1/3] mm/vmalloc: introduce vmalloc_exec which allocates RO+X memory

On Thu, Jul 14, 2022 at 04:54:40AM +0000, Song Liu wrote:
>
>
> > On Jul 13, 2022, at 9:23 PM, Christoph Hellwig <[email protected]> wrote:
> >
> > On Wed, Jul 13, 2022 at 03:49:45PM +0000, Song Liu wrote:
> >>
> >>
> >>> On Jul 13, 2022, at 3:08 AM, Christoph Hellwig <[email protected]> wrote:
> >>>
> >>> NAK. This is not something that should be an exported public API
> >>> ever.
> >>
> >> Hmm.. I will remove EXPORT_SYMBOL_GPL (if we ever do a v2 of this..)
> >
> > Even without that it really is not a vmalloc API anyway.
>
> This ...
>
> > Executable
> > memory needs to be written first, so we should allocate it in that state
> > and only mark it executable after that write has completed.
>
> ... and this are two separate NAKs.
>
> For the first NAK, I agree that my version is another layer on top of
> vmalloc. But what do you think about Peter's idea? AFAICT, that fits
> well in vmalloc logic.
>
I am not able to find the patch/change to see what you have done. But
please do not build a new allocator on top of vmalloc code. We have
three different ones what make things to be complicated :)

--
Uladzislau Rezki

2022-07-15 00:54:37

by Song Liu

[permalink] [raw]
Subject: Re: [PATCH bpf-next 1/3] mm/vmalloc: introduce vmalloc_exec which allocates RO+X memory



> On Jul 14, 2022, at 11:15 AM, Uladzislau Rezki <[email protected]> wrote:
>
> On Thu, Jul 14, 2022 at 04:54:40AM +0000, Song Liu wrote:
>>
>>
>>> On Jul 13, 2022, at 9:23 PM, Christoph Hellwig <[email protected]> wrote:
>>>
>>> On Wed, Jul 13, 2022 at 03:49:45PM +0000, Song Liu wrote:
>>>>
>>>>
>>>>> On Jul 13, 2022, at 3:08 AM, Christoph Hellwig <[email protected]> wrote:
>>>>>
>>>>> NAK. This is not something that should be an exported public API
>>>>> ever.
>>>>
>>>> Hmm.. I will remove EXPORT_SYMBOL_GPL (if we ever do a v2 of this..)
>>>
>>> Even without that it really is not a vmalloc API anyway.
>>
>> This ...
>>
>>> Executable
>>> memory needs to be written first, so we should allocate it in that state
>>> and only mark it executable after that write has completed.
>>
>> ... and this are two separate NAKs.
>>
>> For the first NAK, I agree that my version is another layer on top of
>> vmalloc. But what do you think about Peter's idea? AFAICT, that fits
>> well in vmalloc logic.
>>
> I am not able to find the patch/change to see what you have done.

vger dropped my patch again. :(

> But
> please do not build a new allocator on top of vmalloc code. We have
> three different ones what make things to be complicated :)

It was a bpf_prog_pack like allocator, but named as vmalloc_exec(),
vfree_exec(). I guess I have got enough NAKs for it.

Thanks,
Song