[PATCH v2] mm/page-writeback: document folio_mark_dirty() locking more explicitly

[PATCH v2] mm/page-writeback: document folio_mark_dirty() locking more explicitly

[Jann Horn]

We have had bugs where set_page_dirty() was used on a page from GUP without
appropriate locking, leading to UAF, in:

 - KVM, see
   https://lore.kernel.org/r/20260810-x86-kvm-setpagedirty-v1-1-85f180892d4f@google.com
 - i915, see commit 0d4bbe3d407f ("drm/i915/userptr: Try to acquire the
   page lock around set_page_dirty()").
 - VMCI, see commit 5a16c535409f ("VMCI: Use set_page_dirty_lock() when
   unregistering guest memory")
 - kpc2000 staging driver, see commit b6d13bd9f2c1 ("staging: kpc2000:
   kpc_dma: Convert set_page_dirty() --> set_page_dirty_lock()")

I think set_page_dirty() and folio_mark_dirty() need more explicit
documentation on how they should be used with pages from GUP; so add a
comment on top of set_page_dirty() and make the comment above
folio_mark_dirty() more explicit.

Signed-off-by: Jann Horn <jannh@google.com>
---
Changes in v2:
- reword commit message to drop references to out-of-tree drivers, and
  instead add more examples of upstream bugs
- Link to v1: https://patch.msgid.link/20260810-set-page-dirty-warnings-v1-1-53cad4890808@google.com
---
 mm/folio-compat.c   | 1 +
 mm/page-writeback.c | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/mm/folio-compat.c b/mm/folio-compat.c
index a02179a0bded..6212fdd6761a 100644
--- a/mm/folio-compat.c
+++ b/mm/folio-compat.c
@@ -41,6 +41,7 @@ void set_page_writeback(struct page *page)
 }
 EXPORT_SYMBOL(set_page_writeback);
 
+/* Read the comment above folio_mark_dirty() regarding required locks! */
 bool set_page_dirty(struct page *page)
 {
 	return folio_mark_dirty(page_folio(page));
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index e98748112d1e..b0ab687c83be 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -2773,6 +2773,11 @@ EXPORT_SYMBOL(folio_redirty_for_writepage);
  * in this folio.  Truncation will block on the page table lock as it
  * unmaps pages before removing the folio from its mapping.
  *
+ * .. DANGER::
+ *    Do not use this on a folio obtained from a function like
+ *    get_user_pages_fast() without holding appropriate locks; you might want to
+ *    use set_page_dirty_lock() or folio_mark_dirty_lock() instead.
+ *
  * Return: True if the folio was newly dirtied, false if it was already dirty.
  */
 bool folio_mark_dirty(struct folio *folio)

---
base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
change-id: 20260810-set-page-dirty-warnings-4000f0015394

Best regards,
--  
Jann Horn <jannh@google.com>

Re: [PATCH v2] mm/page-writeback: document folio_mark_dirty() locking more explicitly

[Jan Kara]

On Mon 10-08-26 20:10:01, Jann Horn wrote:
> We have had bugs where set_page_dirty() was used on a page from GUP without
> appropriate locking, leading to UAF, in:
> 
>  - KVM, see
>    https://lore.kernel.org/r/20260810-x86-kvm-setpagedirty-v1-1-85f180892d4f@google.com
>  - i915, see commit 0d4bbe3d407f ("drm/i915/userptr: Try to acquire the
>    page lock around set_page_dirty()").
>  - VMCI, see commit 5a16c535409f ("VMCI: Use set_page_dirty_lock() when
>    unregistering guest memory")
>  - kpc2000 staging driver, see commit b6d13bd9f2c1 ("staging: kpc2000:
>    kpc_dma: Convert set_page_dirty() --> set_page_dirty_lock()")
> 
> I think set_page_dirty() and folio_mark_dirty() need more explicit
> documentation on how they should be used with pages from GUP; so add a
> comment on top of set_page_dirty() and make the comment above
> folio_mark_dirty() more explicit.
> 
> Signed-off-by: Jann Horn <jannh@google.com>

Thanks! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> Changes in v2:
> - reword commit message to drop references to out-of-tree drivers, and
>   instead add more examples of upstream bugs
> - Link to v1: https://patch.msgid.link/20260810-set-page-dirty-warnings-v1-1-53cad4890808@google.com
> ---
>  mm/folio-compat.c   | 1 +
>  mm/page-writeback.c | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/mm/folio-compat.c b/mm/folio-compat.c
> index a02179a0bded..6212fdd6761a 100644
> --- a/mm/folio-compat.c
> +++ b/mm/folio-compat.c
> @@ -41,6 +41,7 @@ void set_page_writeback(struct page *page)
>  }
>  EXPORT_SYMBOL(set_page_writeback);
>  
> +/* Read the comment above folio_mark_dirty() regarding required locks! */
>  bool set_page_dirty(struct page *page)
>  {
>  	return folio_mark_dirty(page_folio(page));
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index e98748112d1e..b0ab687c83be 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2773,6 +2773,11 @@ EXPORT_SYMBOL(folio_redirty_for_writepage);
>   * in this folio.  Truncation will block on the page table lock as it
>   * unmaps pages before removing the folio from its mapping.
>   *
> + * .. DANGER::
> + *    Do not use this on a folio obtained from a function like
> + *    get_user_pages_fast() without holding appropriate locks; you might want to
> + *    use set_page_dirty_lock() or folio_mark_dirty_lock() instead.
> + *
>   * Return: True if the folio was newly dirtied, false if it was already dirty.
>   */
>  bool folio_mark_dirty(struct folio *folio)
> 
> ---
> base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
> change-id: 20260810-set-page-dirty-warnings-4000f0015394
> 
> Best regards,
> --  
> Jann Horn <jannh@google.com>
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] mm/page-writeback: document folio_mark_dirty() locking more explicitly

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>