| Message ID | 20250310142750.1209192-1-willy@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm: Decline to manipulate the refcount on a slab page | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 3/10/25 15:27, Matthew Wilcox (Oracle) wrote: > Slab pages now have a refcount of 0, so nobody should be trying to > manipulate the refcount on them. Doing so has little effect; the object > could be freed and reallocated to a different purpose, although the slab > itself would not be until the refcount was put making it behave rather > like TYPESAFE_BY_RCU. > > Unfortunately, __iov_iter_get_pages_alloc() does take a refcount. > Fix that to not change the refcount, and make put_page() silently not > change the refcount. get_page() warns so that we can fix any other > callers that need to be changed. > > Long-term, networking needs to stop taking a refcount on the pages that > it uses and rely on the caller to hold whatever references are necessary > to make the memory stable. In the medium term, more page types are going > to hav a zero refcount, so we'll want to move get_page() and put_page() > out of line. > > Reported-by: Hannes Reinecke <hare@suse.de> > Fixes: 9aec2fb0fd5e (slab: allocate frozen pages) > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > include/linux/mm.h | 7 ++++++- > lib/iov_iter.c | 8 ++++++-- > 2 files changed, 12 insertions(+), 3 deletions(-) > I assume we will have a discussion at LSF around frozen pages/slab behaviour? It's not just networking, also every driver using iov_alloc_pages() and friends is potentially affected. And it would be good to clarify rules how these iterators should be used. But that doesn't affect this patch, so: Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On Mon, Mar 10, 2025 at 05:57:51PM +0100, Hannes Reinecke wrote: > On 3/10/25 15:27, Matthew Wilcox (Oracle) wrote: > I assume we will have a discussion at LSF around frozen pages/slab > behaviour? > It's not just networking, also every driver using iov_alloc_pages() and > friends is potentially affected. > And it would be good to clarify rules how these iterators should be > used. Sure, we can do that. I haven't conducted a deep survey of how many page users really need a refcount, so I'll have things to learn too.
On 3/10/25 19:28, Matthew Wilcox wrote: > On Mon, Mar 10, 2025 at 05:57:51PM +0100, Hannes Reinecke wrote: >> On 3/10/25 15:27, Matthew Wilcox (Oracle) wrote: >> I assume we will have a discussion at LSF around frozen pages/slab >> behaviour? >> It's not just networking, also every driver using iov_alloc_pages() and >> friends is potentially affected. >> And it would be good to clarify rules how these iterators should be >> used. > > Sure, we can do that. I haven't conducted a deep survey of how many > page users really need a refcount, so I'll have things to learn too. That would be awesome. Cheers, Hannes
diff --git a/include/linux/mm.h b/include/linux/mm.h index 61de65c4e430..4e118cbe0556 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1539,7 +1539,10 @@ static inline void folio_get(struct folio *folio) static inline void get_page(struct page *page) { - folio_get(page_folio(page)); + struct folio *folio = page_folio(page); + if (WARN_ON_ONCE(folio_test_slab(folio))) + return; + folio_get(folio); } static inline __must_check bool try_get_page(struct page *page) @@ -1633,6 +1636,8 @@ static inline void put_page(struct page *page) { struct folio *folio = page_folio(page); + if (folio_test_slab(folio)) + return; folio_put(folio); } diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 65f550cb5081..8c7fdb7d8c8f 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -1190,8 +1190,12 @@ static ssize_t __iov_iter_get_pages_alloc(struct iov_iter *i, if (!n) return -ENOMEM; p = *pages; - for (int k = 0; k < n; k++) - get_page(p[k] = page + k); + for (int k = 0; k < n; k++) { + struct folio *folio = page_folio(page); + p[k] = page + k; + if (!folio_test_slab(folio)) + folio_get(folio); + } maxsize = min_t(size_t, maxsize, n * PAGE_SIZE - *start); i->count -= maxsize; i->iov_offset += maxsize;
Slab pages now have a refcount of 0, so nobody should be trying to manipulate the refcount on them. Doing so has little effect; the object could be freed and reallocated to a different purpose, although the slab itself would not be until the refcount was put making it behave rather like TYPESAFE_BY_RCU. Unfortunately, __iov_iter_get_pages_alloc() does take a refcount. Fix that to not change the refcount, and make put_page() silently not change the refcount. get_page() warns so that we can fix any other callers that need to be changed. Long-term, networking needs to stop taking a refcount on the pages that it uses and rely on the caller to hold whatever references are necessary to make the memory stable. In the medium term, more page types are going to hav a zero refcount, so we'll want to move get_page() and put_page() out of line. Reported-by: Hannes Reinecke <hare@suse.de> Fixes: 9aec2fb0fd5e (slab: allocate frozen pages) Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- include/linux/mm.h | 7 ++++++- lib/iov_iter.c | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-)