Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

On 22/11/2024 00:01, Matthew Wilcox wrote:
> On Thu, Nov 21, 2024 at 11:12:30AM -0800, Boqun Feng wrote:
>> On Thu, Nov 21, 2024 at 11:30:13AM +0200, Abdiel Janulgue wrote:
>>> Hi Boqun, Matthew:
>>>
>>> On 21/11/2024 02:24, Boqun Feng wrote:
>>>>>> So if I understand correctly, what Abdiel needs here is a way to convert
>>>>>> a virtual address to the corresponding page, would it make sense to just
>>>>>> use folio in this case? Abdiel, what's the operation you are going to
>>>>>> call on the page you get?
>>>>>
>>>>> Yes that's basically it. The goal here is represent those existing struct
>>>>> page within this rust Page abstraction but at the same time to avoid taking
>>>>> over its ownership.
>>>>>
>>>>> Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
>>>>>
>>>>
>>>> Could you use folio in your case? If so, we can provide a simple binding
>>>> for folio which should be `AlwaysRefcounted`, and re-investigate how
>>>> page should be wrapped.
>>>>
>>>
>>> I'm not sure. Is there a way to get the struct folio from a vmalloc'd
>>> address, e.g vmalloc_to_folio()?
>>>
>>
>> I think you can use page_folio(vmalloc_to_page(..)) to get the folio,
>> but one thing to notice is that folio is guaranteed to be a non-tail
>> page, so if you want to do something later for the particular page (if
>> it's a tail page), you will need to know the offset of the that page in
>> folio. You can do something like below:
> 
> This is one of those things which will work today, but will stop
> working in the future, and anyway will only appear to work for some
> users.
> 
> For example, both vmalloc and slab allocations do not use the refcount
> on the struct page for anything.  eg this will be a UAF (please excuse
> me writing in C):
> 
> 	char *a = kmalloc(256, GFP_KERNEL);
> 	struct page *page = get_page(virt_to_page(a));
> 	char *b = page_address(page) + offset_in_page(a);
> 	// a and b will now have the same bit pattern
> 	kfree(a);
> 	*b = 1;
> 
> Once you've called kfree(), slab is entitled to hand that memory out
> to any other user of kmalloc().  This might actually work to protect
> vmalloc() memory from going away under you, but I intend to change
> vmalloc so that it won't work (nothing to do with this patch series,
> rather an approach to make vmalloc more efficient).
> 
> One reason you're confused today is that we have a temporary ambiguity
> around what "folio" actually means.  The original definition (ie mine) was
> simply that it was a non-tail page.  We're moving towards the definition
> Johannes wanted, which is that it's only the memdesc for anonymous &
> file-backed memory [1].  So while vmalloc_to_folio() makes sense under
> the original definition, it's an absurdity under the new definition.
> 
> So, Abdiel, why are you trying to add this?  What are you actually
> trying to accomplish in terms of "I am writing a device driver for XXX
> and I need to ..."?  You've been very evasive up to now.

Background behind this is that we need this for the nova rust driver [0].

We need an abstraction of struct page to construct a scatterlist which 
is needed for an internal firmware structure. Now most of pages needed 
there come from vmalloc_to_page() which, unlike the current rust Page 
abstraction, not allocated on demand but is an existing mapping.

Hope that clears things up!

Regards,
Abdiel

[0] https://rust-for-linux.com/nova-gpu-driver