[PATCH -next] mm/ksm: Add missing IS_ERR_OR_NULL check for stable_tree_search()

[PATCH -next] mm/ksm: Add missing IS_ERR_OR_NULL check for stable_tree_search()

[Gaosheng Cui]

The stable_tree_search() maybe return -EBUSY if the stable node's page
is being migrated or nullptr, we need to check kfolio with
IS_ERR_OR_NULL() before dereference it.

To mitigate this, add IS_ERR_OR_NULL check for stable_tree_search().

Fixes: 15605244fdce ("ksm: convert cmp_and_merge_page() to use a folio")
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
 mm/ksm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 4d482d011745..7ac59cde626c 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -1787,7 +1787,7 @@ static __always_inline struct folio *chain(struct ksm_stable_node **s_n_d,
  * with identical content to the page that we are scanning right now.
  *
  * This function returns the stable tree node of identical content if found,
- * NULL otherwise.
+ * -EBUSY if the stable node's page is being migrated, NULL otherwise.
  */
 static struct folio *stable_tree_search(struct page *page)
 {
@@ -2261,7 +2261,8 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
 
 	/* Start by searching for the folio in the stable tree */
 	kfolio = stable_tree_search(page);
-	if (&kfolio->page == page && rmap_item->head == stable_node) {
+	if (!IS_ERR_OR_NULL(kfolio) && &kfolio->page == page &&
+	    rmap_item->head == stable_node) {
 		folio_put(kfolio);
 		return;
 	}
-- 
2.25.1

Re: [PATCH -next] mm/ksm: Add missing IS_ERR_OR_NULL check for stable_tree_search()

[David Hildenbrand]

On 24.10.24 05:23, Gaosheng Cui wrote:
> The stable_tree_search() maybe return -EBUSY if the stable node's page
> is being migrated or nullptr, we need to check kfolio with
> IS_ERR_OR_NULL() before dereference it.
> 
> To mitigate this, add IS_ERR_OR_NULL check for stable_tree_search().
> 
> Fixes: 15605244fdce ("ksm: convert cmp_and_merge_page() to use a folio")
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
>   mm/ksm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 4d482d011745..7ac59cde626c 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1787,7 +1787,7 @@ static __always_inline struct folio *chain(struct ksm_stable_node **s_n_d,
>    * with identical content to the page that we are scanning right now.
>    *
>    * This function returns the stable tree node of identical content if found,
> - * NULL otherwise.
> + * -EBUSY if the stable node's page is being migrated, NULL otherwise.
>    */
>   static struct folio *stable_tree_search(struct page *page)
>   {
> @@ -2261,7 +2261,8 @@ static void cmp_and_merge_page(struct page *page, struct ksm_rmap_item *rmap_ite
>   
>   	/* Start by searching for the folio in the stable tree */
>   	kfolio = stable_tree_search(page);
> -	if (&kfolio->page == page && rmap_item->head == stable_node) {
> +	if (!IS_ERR_OR_NULL(kfolio) && &kfolio->page == page &&
> +	    rmap_item->head == stable_node) {
>   		folio_put(kfolio);
>   		return;
>   	}

I'm late, but LGTM

Acked-by: David Hildenbrand <david@redhat.com>


-- 
Cheers,

David / dhildenb

Re: [PATCH -next] mm/ksm: Add missing IS_ERR_OR_NULL check for stable_tree_search()

[Matthew Wilcox]

On Fri, Nov 15, 2024 at 11:44:10AM +0100, David Hildenbrand wrote:
> On 24.10.24 05:23, Gaosheng Cui wrote:
> > The stable_tree_search() maybe return -EBUSY if the stable node's page
> > is being migrated or nullptr, we need to check kfolio with
> > IS_ERR_OR_NULL() before dereference it.

Argh.  I doidn't notice this when it came through.  Andrew already
folded it in, but it's utter garbage.  We don't dereference folio.
NAK.

Re: [PATCH -next] mm/ksm: Add missing IS_ERR_OR_NULL check for stable_tree_search()

[David Hildenbrand]

On 15.11.24 16:41, Matthew Wilcox wrote:
> On Fri, Nov 15, 2024 at 11:44:10AM +0100, David Hildenbrand wrote:
>> On 24.10.24 05:23, Gaosheng Cui wrote:
>>> The stable_tree_search() maybe return -EBUSY if the stable node's page
>>> is being migrated or nullptr, we need to check kfolio with
>>> IS_ERR_OR_NULL() before dereference it.
> 
> Argh.  I doidn't notice this when it came through.  Andrew already
> folded it in, but it's utter garbage.  We don't dereference folio.
> NAK.

Ah, &kfolio->page indeed doesn't de-reference. I was blindly assuming 
someone actually ran into an issue here.

Thanks for double-checking ...

-- 
Cheers,

David / dhildenb

Re: [PATCH] ksm: Fix potential NULL pointer dereference

[Fushuai Wang]

>>  The stable_tree_search() function may return an error pointer
>>  (NULL or ERR_PTR(...)). The current code does not check for
>>  these cases before dereferencing the returned value.
> 
> We discussed this in [1] where Willy clarified that there is no 
> de-referencing happening [2].
> 
> So no, this is not required.
> 
> Consider "&kfolio->page" currently being a "(struct page *) kfolio", 
> which is just a pointer cast and no de-referencing.
> 
> [1] 
> https://lore.kernel.org/linux-mm/20241024032300.2501949-1-cuigaosheng1@huawei.com/
> [2] https://lore.kernel.org/linux-mm/Zzdrnod2zcTeI-Nt@casper.infradead.org/

Thanks!

Pease ignore this.
I'll be more careful in next time.

Regards,
Wang.