FOLL_LONGTERM will avoid share memory alloc from CMA region, which may be used in secure playback case. if part of CMA region taken by share memory for long term usage, CMA will failed to get whole buffer back.
Signed-off-by: Xiaoming Ding xiaoming.ding@mediatek.com --- drivers/tee/tee_shm.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 673cf0359494..ddd3947e2229 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -223,6 +223,7 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr, size_t num_pages; void *ret; int rc; + u32 page_flag = FOLL_WRITE;
if (!tee_device_get(teedev)) return ERR_PTR(-EINVAL); @@ -255,9 +256,11 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr, ret = ERR_PTR(-ENOMEM); goto err_free_shm; } - +#if IS_ENABLED(CONFIG_CMA) + page_flag |= FOLL_LONGTERM; +#endif if (flags & TEE_SHM_USER_MAPPED) - rc = pin_user_pages_fast(start, num_pages, FOLL_WRITE, + rc = pin_user_pages_fast(start, num_pages, page_flag, shm->pages); else rc = shm_get_kernel_pages(start, num_pages, shm->pages);
- u32 page_flag = FOLL_WRITE;
if (!tee_device_get(teedev)) return ERR_PTR(-EINVAL); @@ -255,9 +256,11 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr, ret = ERR_PTR(-ENOMEM); goto err_free_shm; }
+#if IS_ENABLED(CONFIG_CMA)
- page_flag |= FOLL_LONGTERM;
+#endif if (flags & TEE_SHM_USER_MAPPED)
If this mapping is long live it should always use FOLL_LONGTERM.
The ifdef does not make sense.
On Wed, 17 May 2023 at 13:04, Christoph Hellwig hch@infradead.org wrote:
u32 page_flag = FOLL_WRITE; if (!tee_device_get(teedev)) return ERR_PTR(-EINVAL);@@ -255,9 +256,11 @@ register_shm_helper(struct tee_context *ctx, unsigned long addr, ret = ERR_PTR(-ENOMEM); goto err_free_shm; }
+#if IS_ENABLED(CONFIG_CMA)
page_flag |= FOLL_LONGTERM;+#endif if (flags & TEE_SHM_USER_MAPPED)
If this mapping is long live it should always use FOLL_LONGTERM.
It depends on the userspace application needs. However, I think it should be safe to use FOLL_LONGTERM by default to serve cases like secure media playback.
The ifdef does not make sense.
Agree.
-Sumit
On Wed, May 17, 2023 at 01:22:51PM +0530, Sumit Garg wrote:
+#if IS_ENABLED(CONFIG_CMA)
page_flag |= FOLL_LONGTERM;+#endif if (flags & TEE_SHM_USER_MAPPED)
If this mapping is long live it should always use FOLL_LONGTERM.
It depends on the userspace application needs. However, I think it should be safe to use FOLL_LONGTERM by default to serve cases like secure media playback.
long term is defined as won't automatically go away during the same syscall.
On Wed, 17 May 2023 at 13:38, Christoph Hellwig hch@infradead.org wrote:
On Wed, May 17, 2023 at 01:22:51PM +0530, Sumit Garg wrote:
+#if IS_ENABLED(CONFIG_CMA)
page_flag |= FOLL_LONGTERM;+#endif if (flags & TEE_SHM_USER_MAPPED)
If this mapping is long live it should always use FOLL_LONGTERM.
It depends on the userspace application needs. However, I think it should be safe to use FOLL_LONGTERM by default to serve cases like secure media playback.
long term is defined as won't automatically go away during the same syscall.
Do you mean a pinned user-space page can be paged out automatically? The documentation [1] isn't very helpful here either since it talks about "short term" vs "long term" in abstract terms.
Just FYI, the underlying use-case for TEE registered shared memory is that the references to pinned pages are provided to TEE implementation to operate upon. This can happen over multiple syscalls and we want the pinned pages to be always in RAM as otherwise the physical addresses may change if they are paged out in between. If this is only supported reliably with a long term flag then this patch should be tagged as a fix and requires stable backports.
[1] Documentation/core-api/pin_user_pages.rst
-Sumit
On Wed, May 17, 2023 at 02:56:13PM +0530, Sumit Garg wrote:
Do you mean a pinned user-space page can be paged out automatically?
No, pinned pages can't be paged out.
But a short term pin implies it will be release after a short delay, and it is feasible for wait for the pin to go away.
For a long term pin waiting is not an option, and anyone wanting to do something with the pinned page that requires it to not be pinned must simply give up.
Just FYI, the underlying use-case for TEE registered shared memory is that the references to pinned pages are provided to TEE implementation to operate upon. This can happen over multiple syscalls and we want the pinned pages to be always in RAM as otherwise the physical addresses may change if they are paged out in between.
That's a very use clear case for a long term pin.
On Wed, 17 May 2023 at 15:06, Christoph Hellwig hch@infradead.org wrote:
On Wed, May 17, 2023 at 02:56:13PM +0530, Sumit Garg wrote:
Do you mean a pinned user-space page can be paged out automatically?
No, pinned pages can't be paged out.
But a short term pin implies it will be release after a short delay, and it is feasible for wait for the pin to go away.
Okay, I see. I would be interested to know the ranges for that short delay. I guess it may depend on how much memory pressure there is...
For a long term pin waiting is not an option, and anyone wanting to do something with the pinned page that requires it to not be pinned must simply give up.
Just FYI, the underlying use-case for TEE registered shared memory is that the references to pinned pages are provided to TEE implementation to operate upon. This can happen over multiple syscalls and we want the pinned pages to be always in RAM as otherwise the physical addresses may change if they are paged out in between.
That's a very use clear case for a long term pin.
...however, thanks for the insights.
@Xiaoming,
Please use the following fixes tag for the v2 along with extending the commit description regarding the reliability provided by the long term flag.
Fixes: 033ddf12bcf5 ("tee: add register user memory")
-Sumit
On 17.05.23 12:19, Sumit Garg wrote:
On Wed, 17 May 2023 at 15:06, Christoph Hellwig hch@infradead.org wrote:
On Wed, May 17, 2023 at 02:56:13PM +0530, Sumit Garg wrote:
Do you mean a pinned user-space page can be paged out automatically?
No, pinned pages can't be paged out.
But a short term pin implies it will be release after a short delay, and it is feasible for wait for the pin to go away.
Okay, I see. I would be interested to know the ranges for that short delay. I guess it may depend on how much memory pressure there is...
In general: if user space controls it -> possibly forever -> long-term. Even if in most cases it's a short delay: there is no trusting on user space.
For example, iouring fixed buffers keep pages pinned until user space decides to unregistered the buffers -> long-term.
Short-term is, for example, something like O_DIRECT where we pin -> DMA -> unpin in essentially one operation.
On Wed, May 17, 2023 at 08:23:33PM +0200, David Hildenbrand wrote:
In general: if user space controls it -> possibly forever -> long-term. Even if in most cases it's a short delay: there is no trusting on user space.
For example, iouring fixed buffers keep pages pinned until user space decides to unregistered the buffers -> long-term.
Short-term is, for example, something like O_DIRECT where we pin -> DMA -> unpin in essentially one operation.
Btw, one thing that's been on my mind is that I think we got the polarity on FOLL_LONGTERM wrong. Instead of opting into the long term behavior it really should be the default, with a FOLL_EPHEMERAL flag to opt out of it. And every users of this flag is required to have a comment explaining the life time rules for the pin..
On Thu, 18 May 2023 at 09:51, Christoph Hellwig hch@infradead.org wrote:
On Wed, May 17, 2023 at 08:23:33PM +0200, David Hildenbrand wrote:
In general: if user space controls it -> possibly forever -> long-term. Even if in most cases it's a short delay: there is no trusting on user space.
For example, iouring fixed buffers keep pages pinned until user space decides to unregistered the buffers -> long-term.
Short-term is, for example, something like O_DIRECT where we pin -> DMA -> unpin in essentially one operation.
Btw, one thing that's been on my mind is that I think we got the polarity on FOLL_LONGTERM wrong. Instead of opting into the long term behavior it really should be the default, with a FOLL_EPHEMERAL flag to opt out of it. And every users of this flag is required to have a comment explaining the life time rules for the pin..
It does look like a better approach to me given the very nature of user space pages.
-Sumit
On 18.05.23 08:08, Sumit Garg wrote:
On Thu, 18 May 2023 at 09:51, Christoph Hellwig hch@infradead.org wrote:
On Wed, May 17, 2023 at 08:23:33PM +0200, David Hildenbrand wrote:
In general: if user space controls it -> possibly forever -> long-term. Even if in most cases it's a short delay: there is no trusting on user space.
For example, iouring fixed buffers keep pages pinned until user space decides to unregistered the buffers -> long-term.
Short-term is, for example, something like O_DIRECT where we pin -> DMA -> unpin in essentially one operation.
Btw, one thing that's been on my mind is that I think we got the polarity on FOLL_LONGTERM wrong. Instead of opting into the long term behavior it really should be the default, with a FOLL_EPHEMERAL flag to opt out of it. And every users of this flag is required to have a comment explaining the life time rules for the pin..
It does look like a better approach to me given the very nature of user space pages.
Yeah, there is a lot of historical baggage. For example, FOLL_GET should be inaccessible to kernel modules completely at one point, to be only used by selected core-mm pieces.
Maybe we should even disallow passing in FOLL_LONGTERM as a flag and only provide functions like pin_user_pages() vs. pin_user_pages_longterm(). Then, discussions about conditional flag-setting are no more :)
... or even use pin_user_pages_shortterm() vs. pin_user_pages() ... to make the default be longterm.
On 5/18/23 06:56, David Hildenbrand wrote:
On 18.05.23 08:08, Sumit Garg wrote:
On Thu, 18 May 2023 at 09:51, Christoph Hellwig hch@infradead.org wrote:
On Wed, May 17, 2023 at 08:23:33PM +0200, David Hildenbrand wrote:
In general: if user space controls it -> possibly forever -> long-term. Even if in most cases it's a short delay: there is no trusting on user space.
For example, iouring fixed buffers keep pages pinned until user space decides to unregistered the buffers -> long-term.
Short-term is, for example, something like O_DIRECT where we pin -> DMA -> unpin in essentially one operation.
Btw, one thing that's been on my mind is that I think we got the polarity on FOLL_LONGTERM wrong. Instead of opting into the long term behavior it really should be the default, with a FOLL_EPHEMERAL flag to opt out of it. And every users of this flag is required to have a comment explaining the life time rules for the pin..
I see maybe 10 or 20 call sites today. So it is definitely feasible to add documentation at each, explaining the why it wants a long term pin.
It does look like a better approach to me given the very nature of user space pages.
Yeah, there is a lot of historical baggage. For example, FOLL_GET should be inaccessible to kernel modules completely at one point, to be only used by selected core-mm pieces.
Yes. When I first mass-converted call sites from gup to pup, I just preserved FOLL_GET behavior in order to keep from changing too much at once. But I agree that that it would be nice to make FOLL_GET an mm internal-only flag like FOLL_PIN.
Maybe we should even disallow passing in FOLL_LONGTERM as a flag and only provide functions like pin_user_pages() vs. pin_user_pages_longterm(). Then, discussions about conditional flag-setting are no more :)
... or even use pin_user_pages_shortterm() vs. pin_user_pages() ... to make the default be longterm.
Yes, it is true that having most gup flags be internal to mm does tend to avoid some bugs. But it's also a lot of churn. I'm still on the fence as to whether it's really a good move to do this for FOLL_LONGTERM or not. But it's really easy to push me off of fences. :)
thanks,
On Mon, May 22, 2023 at 06:54:29PM -0700, John Hubbard wrote:
On 5/18/23 06:56, David Hildenbrand wrote:
On 18.05.23 08:08, Sumit Garg wrote:
On Thu, 18 May 2023 at 09:51, Christoph Hellwig hch@infradead.org wrote:
On Wed, May 17, 2023 at 08:23:33PM +0200, David Hildenbrand wrote:
In general: if user space controls it -> possibly forever -> long-term. Even if in most cases it's a short delay: there is no trusting on user space.
For example, iouring fixed buffers keep pages pinned until user space decides to unregistered the buffers -> long-term.
Short-term is, for example, something like O_DIRECT where we pin -> DMA -> unpin in essentially one operation.
Btw, one thing that's been on my mind is that I think we got the polarity on FOLL_LONGTERM wrong. Instead of opting into the long term behavior it really should be the default, with a FOLL_EPHEMERAL flag to opt out of it. And every users of this flag is required to have a comment explaining the life time rules for the pin..
I couldn't agree more, based on my recent forays into GUP the interface continues to strike me as odd:-
- FOLL_GET is a wing and a prayer that nothing that [folio|page]_maybe_dma_pinned() prevents happens in the brief period the page is pinned/manipulated. So agree completely with David's concept of unexporting that and perhaps carefully considering our use of it. Obviously the comments around functions like gup_remote() make clear that 'this page not be what you think it is' but I wonder whether many callers of GUP _truly_ take that on board.
- FOLL_LONGTERM is entirely optional for PUP and you can just go ahead and fragment page blocks to your heart's content. Of course this would be an abuse, but abuses happen.
- With the recent change to PUP/FOLL_LONGTERM disallowing dirty tracked file-backed mappings we're now really relying on this flag indicating a _long term_ pin semantically. By defaulting to this being switched on, we avoid cases of callers who might end up treating the won't reclaim/etc. aspect of PUP as all they care about while ignoring the MIGRATE_MOVABLE aspect.
I see maybe 10 or 20 call sites today. So it is definitely feasible to add documentation at each, explaining the why it wants a long term pin.
Yeah, my efforts at e.g. dropping vmas has been eye-opening in actually quite how often a refactoring like this often ends up being more straightforward than you might imagine.
It does look like a better approach to me given the very nature of user space pages.
Yeah, there is a lot of historical baggage. For example, FOLL_GET should be inaccessible to kernel modules completely at one point, to be only used by selected core-mm pieces.
Yes. When I first mass-converted call sites from gup to pup, I just preserved FOLL_GET behavior in order to keep from changing too much at once. But I agree that that it would be nice to make FOLL_GET an mm internal-only flag like FOLL_PIN.
Very glad you did that work! And totally understandable as to you being conservative with that, but I think we're at a point where there's more acceptance of incremental improvements to GUP as a whole.
I have another patch series saved up for _yet more_ changes on this. But mindful of churn I am trying to space them out... until Jason nudges me of course :)
Maybe we should even disallow passing in FOLL_LONGTERM as a flag and only provide functions like pin_user_pages() vs. pin_user_pages_longterm(). Then, discussions about conditional flag-setting are no more :)
... or even use pin_user_pages_shortterm() vs. pin_user_pages() ... to make the default be longterm.
Yes, it is true that having most gup flags be internal to mm does tend to avoid some bugs. But it's also a lot of churn. I'm still on the fence as to whether it's really a good move to do this for FOLL_LONGTERM or not. But it's really easy to push me off of fences. :)
*nudge* ;)
thanks,
John Hubbard NVIDIA
Looking at non-fast, non-FOLL_LONGTERM PUP callers (forgive me if I missed any):-
- pin_user_pages_remote() in process_vm_rw_single_vec() for the process_vm_access functionality.
- pin_user_pages_remote() in user_event_enabler_write() in kernel/trace/trace_events_user.c.
- pin_user_pages_unlocked() in ivtv_udma_setup() in drivers/media/pci/ivtv/ivtv-udma.c and ivtv_yuv_prep_user_dma() in ivtv-yuv.c.
And none that actually directly invoke PUP without FOLL_LOGNTERM... That suggests that we could simply disallow non-FOLL_LONGTERM non-fast PUP calls altogether and move to pin_user_pages_longterm() [I'm happy to write a patch series doing this].
The ivtv callers look like they really actually want FOLL_LONGTERM unless I'm missing something so we should probably change that too?
I haven't surveyed the fast versions, but I think defaulting to FOLL_LONGTERM on them also makes sense.
op-tee@lists.trustedfirmware.org
-
Christoph Hellwig -
David Hildenbrand -
John Hubbard -
Lorenzo Stoakes -
Sumit Garg -
Xiaoming Ding