Re: [PATCH v8 24/26] mm/gup: track FOLL_PIN pages
On 12/10/19 5:39 AM, Jan Kara wrote:
...
>> +void grab_page(struct page *page, unsigned int flags)
>> +{
>> +if (flags & FOLL_GET)
>> +get_page(page);
>> +else if (flags & FOLL_PIN) {
>> +get_page(page);
>> +WARN_ON_ONCE(flags & FOLL_GET);
>> +/*
>> + * Use get_page(), above, to do the refcount error
>> + * checking. Then just add in the remaining references:
>> + */
>> +page_ref_add(page, GUP_PIN_COUNTING_BIAS - 1);
>
> This is wrong for two reasons:
>
> 1) You miss compound_head() indirection from get_page() for this
> page_ref_add().
whoops, yes that is missing.
>
> 2) page_ref_add() could overflow the counter without noticing.
>
> Especially with GUP_PIN_COUNTING_BIAS being non-trivial, it is realistic
> that an attacker might try to overflow the page refcount and we have to
> protect the kernel against that. So I think that all the places that would
> use grab_page() actually need to use try_grab_page() and then gracefully
> deal with the failure.
>
OK, I've replaced grab_page() everywhere with try_grab_page(), with the
above issues fixed. The v7 patchset had error handling for grab_page() failures,
that had been reviewed, so relevants parts of that have reappeared.
I had initially hesitated to do this, but now I've gone ahead and added:
#define page_ref_zero_or_close_to_bias_overflow(page) \
((unsigned int) page_ref_count(page) + \
GUP_PIN_COUNTING_BIAS <= GUP_PIN_COUNTING_BIAS)
...which is used in the new try_grab_page() for protection.
>> @@ -278,11 +425,23 @@ static struct page *follow_page_pte(struct
>> vm_area_struct *vma,
>> goto retry;
>> }
>>
>> -if (flags & FOLL_GET) {
>> +if (flags & (FOLL_PIN | FOLL_GET)) {
>> +/*
>> + * Allow try_get_page() to take care of error handling, for
>> + * both cases: FOLL_GET or FOLL_PIN:
>> + */
>> if (unlikely(!try_get_page(page))) {
>> page = ERR_PTR(-ENOMEM);
>> goto out;
>> }
>> +
>> +if (flags & FOLL_PIN) {
>> +WARN_ON_ONCE(flags & FOLL_GET);
>> +
>> +/* We got a +1 refcount from try_get_page(), above. */
>> +page_ref_add(page, GUP_PIN_COUNTING_BIAS - 1);
>> +__update_proc_vmstat(page, NR_FOLL_PIN_REQUESTED, 1);
>> +}
>> }
>
> The same problem here as above, plus this place should use the same
> try_grab..() helper, shouldn't it?
Yes, now that the new try_grab_page() has behavior that matches what
this call site needs. Done.
>
>> @@ -544,8 +703,8 @@ static struct page *follow_page_mask(struct
>> vm_area_struct *vma,
>> /* make this handle hugepd */
>> page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
>> if (!IS_ERR(page)) {
>> -BUG_ON(flags & FOLL_GET);
>> -return page;
>> +WARN_ON_ONCE(flags & (FOLL_GET | FOLL_PIN));
>> +return NULL;
>
> I agree with the change to WARN_ON_ONCE but why is correct the change of
> the return value? Note that this is actually a "success branch".
>
Good catch, thanks! I worked through the logic...correctly at first, but then I
must
have become temporarily dazed by the raw destructive power of the pre-existing
BUG_ON() statement, and screwed it up after all. :)
thanks,
--
John Hubbard
NVIDIA
Re: [PATCH v8 24/26] mm/gup: track FOLL_PIN pages
On Mon 09-12-19 14:53:42, John Hubbard wrote:
> Add tracking of pages that were pinned via FOLL_PIN.
>
> As mentioned in the FOLL_PIN documentation, callers who effectively set
> FOLL_PIN are required to ultimately free such pages via unpin_user_page().
> The effect is similar to FOLL_GET, and may be thought of as "FOLL_GET
> for DIO and/or RDMA use".
>
> Pages that have been pinned via FOLL_PIN are identifiable via a
> new function call:
>
>bool page_dma_pinned(struct page *page);
>
> What to do in response to encountering such a page, is left to later
> patchsets. There is discussion about this in [1], [2], and [3].
>
> This also changes a BUG_ON(), to a WARN_ON(), in follow_page_mask().
>
> [1] Some slow progress on get_user_pages() (Apr 2, 2019):
> https://lwn.net/Articles/784574/
> [2] DMA and get_user_pages() (LPC: Dec 12, 2018):
> https://lwn.net/Articles/774411/
> [3] The trouble with get_user_pages() (Apr 30, 2018):
> https://lwn.net/Articles/753027/
>
> Suggested-by: Jan Kara
> Suggested-by: Jérôme Glisse
> Signed-off-by: John Hubbard
Looks nice, some comments below...
> +/*
> + * try_grab_compound_head() - attempt to elevate a page's refcount, by a
> + * flags-dependent amount.
> + *
> + * This has a default assumption of "use FOLL_GET behavior, if FOLL_PIN is
> not
> + * set".
> + *
> + * "grab" names in this file mean, "look at flags to decide with to use
> FOLL_PIN
> + * or FOLL_GET behavior, when incrementing the page's refcount.
> + */
> +static struct page *try_grab_compound_head(struct page *page, int refs,
> +unsigned int flags)
> +{
> + if (flags & FOLL_PIN)
> + return try_pin_compound_head(page, refs);
> +
> + return try_get_compound_head(page, refs);
> +}
> +
> +/**
> + * grab_page() - elevate a page's refcount by a flag-dependent amount
> + *
> + * This might not do anything at all, depending on the flags argument.
> + *
> + * "grab" names in this file mean, "look at flags to decide with to use
> FOLL_PIN
^^^ whether
> + * or FOLL_GET behavior, when incrementing the page's refcount.
> + *
> + * @page:pointer to page to be grabbed
> + * @flags: gup flags: these are the FOLL_* flag values.
> + *
> + * Either FOLL_PIN or FOLL_GET (or neither) may be set, but not both at the
> same
> + * time. (That's true throughout the get_user_pages*() and pin_user_pages*()
> + * APIs.) Cases:
> + *
> + * FOLL_GET: page's refcount will be incremented by 1.
> + * FOLL_PIN: page's refcount will be incremented by GUP_PIN_COUNTING_BIAS.
> + */
> +void grab_page(struct page *page, unsigned int flags)
> +{
> + if (flags & FOLL_GET)
> + get_page(page);
> + else if (flags & FOLL_PIN) {
> + get_page(page);
> + WARN_ON_ONCE(flags & FOLL_GET);
> + /*
> + * Use get_page(), above, to do the refcount error
> + * checking. Then just add in the remaining references:
> + */
> + page_ref_add(page, GUP_PIN_COUNTING_BIAS - 1);
This is wrong for two reasons:
1) You miss compound_head() indirection from get_page() for this
page_ref_add().
2) page_ref_add() could overflow the counter without noticing.
Especially with GUP_PIN_COUNTING_BIAS being non-trivial, it is realistic
that an attacker might try to overflow the page refcount and we have to
protect the kernel against that. So I think that all the places that would
use grab_page() actually need to use try_grab_page() and then gracefully
deal with the failure.
> @@ -278,11 +425,23 @@ static struct page *follow_page_pte(struct
> vm_area_struct *vma,
> goto retry;
> }
>
> - if (flags & FOLL_GET) {
> + if (flags & (FOLL_PIN | FOLL_GET)) {
> + /*
> + * Allow try_get_page() to take care of error handling, for
> + * both cases: FOLL_GET or FOLL_PIN:
> + */
> if (unlikely(!try_get_page(page))) {
> page = ERR_PTR(-ENOMEM);
> goto out;
> }
> +
> + if (flags & FOLL_PIN) {
> + WARN_ON_ONCE(flags & FOLL_GET);
> +
> + /* We got a +1 refcount from try_get_page(), above. */
> + page_ref_add(page, GUP_PIN_COUNTING_BIAS - 1);
> + __update_proc_vmstat(page, NR_FOLL_PIN_REQUESTED, 1);
> + }
> }
The same problem here as above, plus this place should use the same
try_grab..() helper, shouldn't it?
> @@ -544,8 +703,8 @@ static struct page *follow_page_mask(struct
> vm_area_struct *vma,
> /* make this handle hugepd */
> page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
> if (!IS_ERR(page)) {
> - BUG_ON(flags & FOLL_GET);
> - return page;
> + WARN_ON_ONCE(flags & (FOLL_GET | FOLL_PIN));
> +
[PATCH v8 24/26] mm/gup: track FOLL_PIN pages
Add tracking of pages that were pinned via FOLL_PIN.
As mentioned in the FOLL_PIN documentation, callers who effectively set
FOLL_PIN are required to ultimately free such pages via unpin_user_page().
The effect is similar to FOLL_GET, and may be thought of as "FOLL_GET
for DIO and/or RDMA use".
Pages that have been pinned via FOLL_PIN are identifiable via a
new function call:
bool page_dma_pinned(struct page *page);
What to do in response to encountering such a page, is left to later
patchsets. There is discussion about this in [1], [2], and [3].
This also changes a BUG_ON(), to a WARN_ON(), in follow_page_mask().
[1] Some slow progress on get_user_pages() (Apr 2, 2019):
https://lwn.net/Articles/784574/
[2] DMA and get_user_pages() (LPC: Dec 12, 2018):
https://lwn.net/Articles/774411/
[3] The trouble with get_user_pages() (Apr 30, 2018):
https://lwn.net/Articles/753027/
Suggested-by: Jan Kara
Suggested-by: Jérôme Glisse
Signed-off-by: John Hubbard
---
Documentation/core-api/pin_user_pages.rst | 2 +-
include/linux/mm.h| 73 -
include/linux/mmzone.h| 2 +
include/linux/page_ref.h | 10 +
mm/gup.c | 338 +-
mm/huge_memory.c | 23 +-
mm/hugetlb.c | 15 +-
mm/vmstat.c | 2 +
8 files changed, 362 insertions(+), 103 deletions(-)
diff --git a/Documentation/core-api/pin_user_pages.rst
b/Documentation/core-api/pin_user_pages.rst
index bba96428ade7..6d93ef203561 100644
--- a/Documentation/core-api/pin_user_pages.rst
+++ b/Documentation/core-api/pin_user_pages.rst
@@ -53,7 +53,7 @@ Which flags are set by each wrapper
For these pin_user_pages*() functions, FOLL_PIN is OR'd in with whatever gup
flags the caller provides. The caller is required to pass in a non-null struct
pages* array, and the function then pin pages by incrementing each by a special
-value. For now, that value is +1, just like get_user_pages*().::
+value: GUP_PIN_COUNTING_BIAS.::
Function
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6a1a357e7d86..9666989d2b9e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1016,6 +1016,8 @@ static inline void get_page(struct page *page)
page_ref_inc(page);
}
+void grab_page(struct page *page, unsigned int flags);
+
static inline __must_check bool try_get_page(struct page *page)
{
page = compound_head(page);
@@ -1044,29 +1046,70 @@ static inline void put_page(struct page *page)
__put_page(page);
}
-/**
- * unpin_user_page() - release a gup-pinned page
- * @page:pointer to page to be released
+/*
+ * GUP_PIN_COUNTING_BIAS, and the associated functions that use it, overload
+ * the page's refcount so that two separate items are tracked: the original
page
+ * reference count, and also a new count of how many pin_user_pages() calls
were
+ * made against the page. ("gup-pinned" is another term for the latter).
+ *
+ * With this scheme, pin_user_pages() becomes special: such pages are marked as
+ * distinct from normal pages. As such, the unpin_user_page() call (and its
+ * variants) must be used in order to release gup-pinned pages.
+ *
+ * Choice of value:
*
- * Pages that were pinned via pin_user_pages*() must be released via either
- * unpin_user_page(), or one of the unpin_user_pages*() routines. This is so
- * that eventually such pages can be separately tracked and uniquely handled.
In
- * particular, interactions with RDMA and filesystems need special handling.
+ * By making GUP_PIN_COUNTING_BIAS a power of two, debugging of page reference
+ * counts with respect to pin_user_pages() and unpin_user_page() becomes
+ * simpler, due to the fact that adding an even power of two to the page
+ * refcount has the effect of using only the upper N bits, for the code that
+ * counts up using the bias value. This means that the lower bits are left for
+ * the exclusive use of the original code that increments and decrements by one
+ * (or at least, by much smaller values than the bias value).
*
- * unpin_user_page() and put_page() are not interchangeable, despite this early
- * implementation that makes them look the same. unpin_user_page() calls must
- * be perfectly matched up with pin*() calls.
+ * Of course, once the lower bits overflow into the upper bits (and this is
+ * OK, because subtraction recovers the original values), then visual
inspection
+ * no longer suffices to directly view the separate counts. However, for normal
+ * applications that don't have huge page reference counts, this won't be an
+ * issue.
+ *
+ * Locking: the lockless algorithm described in page_cache_get_speculative()
+ * and page_cache_gup_pin_speculative() provides safe operation for
+ * get_user_pages and page_mkclean and other calls that race to set up page
+ * table entries.
*/
-static inline void unp
