mm/thp: fix deferred split queue not partially_mapped: fix

[PATCH hotfix] mm/thp: fix deferred split queue not partially_mapped: fix

Posted by Hugh Dickins 1 year, 1 month ago

Though even more elusive than before, list_del corruption has still been
seen on THP's deferred split queue.

The idea in commit e66f3185fa04 was right, but its implementation wrong.
The context omitted an important comment just before the critical test:
"split_folio() removes folio from list on success."  In ignoring that
comment, when a THP split succeeded, the code went on to release the
preceding safe folio, preserving instead an irrelevant (formerly head)
folio: which gives no safety because it's not on the list. Fix the logic.

Fixes: e66f3185fa04 ("mm/thp: fix deferred split queue not partially_mapped")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
 mm/huge_memory.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 03fd4bc39ea1..5734d5d5060f 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3790,7 +3790,9 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
 		 * in the case it was underused, then consider it used and
 		 * don't add it back to split_queue.
 		 */
-		if (!did_split && !folio_test_partially_mapped(folio)) {
+		if (did_split) {
+			; /* folio already removed from list */
+		} else if (!folio_test_partially_mapped(folio)) {
 			list_del_init(&folio->_deferred_list);
 			removed++;
 		} else {
-- 
2.35.3

Re: [PATCH hotfix] mm/thp: fix deferred split queue not partially_mapped: fix

Posted by Chris Li 1 year, 1 month ago

Hi Hugh,

LGTM.

Acked-by: Chris Li <chrisl@kernel.org>

Chris

On Sun, Nov 10, 2024 at 1:11 PM Hugh Dickins <hughd@google.com> wrote:
>
> Though even more elusive than before, list_del corruption has still been
> seen on THP's deferred split queue.
>
> The idea in commit e66f3185fa04 was right, but its implementation wrong.
> The context omitted an important comment just before the critical test:
> "split_folio() removes folio from list on success."  In ignoring that
> comment, when a THP split succeeded, the code went on to release the
> preceding safe folio, preserving instead an irrelevant (formerly head)
> folio: which gives no safety because it's not on the list. Fix the logic.
>
> Fixes: e66f3185fa04 ("mm/thp: fix deferred split queue not partially_mapped")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
>  mm/huge_memory.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 03fd4bc39ea1..5734d5d5060f 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3790,7 +3790,9 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>                  * in the case it was underused, then consider it used and
>                  * don't add it back to split_queue.
>                  */
> -               if (!did_split && !folio_test_partially_mapped(folio)) {
> +               if (did_split) {
> +                       ; /* folio already removed from list */
> +               } else if (!folio_test_partially_mapped(folio)) {
>                         list_del_init(&folio->_deferred_list);
>                         removed++;
>                 } else {
> --
> 2.35.3

Re: [PATCH hotfix] mm/thp: fix deferred split queue not partially_mapped: fix

Posted by Baolin Wang 1 year, 1 month ago


On 2024/11/11 05:11, Hugh Dickins wrote:
> Though even more elusive than before, list_del corruption has still been
> seen on THP's deferred split queue.
> 
> The idea in commit e66f3185fa04 was right, but its implementation wrong.
> The context omitted an important comment just before the critical test:
> "split_folio() removes folio from list on success."  In ignoring that
> comment, when a THP split succeeded, the code went on to release the
> preceding safe folio, preserving instead an irrelevant (formerly head)
> folio: which gives no safety because it's not on the list. Fix the logic.
> 
> Fixes: e66f3185fa04 ("mm/thp: fix deferred split queue not partially_mapped")
> Signed-off-by: Hugh Dickins <hughd@google.com>

Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

> ---
>   mm/huge_memory.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 03fd4bc39ea1..5734d5d5060f 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3790,7 +3790,9 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>   		 * in the case it was underused, then consider it used and
>   		 * don't add it back to split_queue.
>   		 */
> -		if (!did_split && !folio_test_partially_mapped(folio)) {
> +		if (did_split) {
> +			; /* folio already removed from list */
> +		} else if (!folio_test_partially_mapped(folio)) {
>   			list_del_init(&folio->_deferred_list);
>   			removed++;
>   		} else {

Re: [PATCH hotfix] mm/thp: fix deferred split queue not partially_mapped: fix

Posted by Zi Yan 1 year, 1 month ago

On 10 Nov 2024, at 16:11, Hugh Dickins wrote:

> Though even more elusive than before, list_del corruption has still been
> seen on THP's deferred split queue.
>
> The idea in commit e66f3185fa04 was right, but its implementation wrong.
> The context omitted an important comment just before the critical test:
> "split_folio() removes folio from list on success."  In ignoring that
> comment, when a THP split succeeded, the code went on to release the
> preceding safe folio, preserving instead an irrelevant (formerly head)
> folio: which gives no safety because it's not on the list. Fix the logic.
>
> Fixes: e66f3185fa04 ("mm/thp: fix deferred split queue not partially_mapped")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
>  mm/huge_memory.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks. Reviewed-by: Zi Yan <ziy@nvidia.com>

--
Best Regards,
Yan, Zi

Re: [PATCH hotfix] mm/thp: fix deferred split queue not partially_mapped: fix

Posted by Usama Arif 1 year, 1 month ago


On 10/11/2024 21:11, Hugh Dickins wrote:
> Though even more elusive than before, list_del corruption has still been
> seen on THP's deferred split queue.
> 
> The idea in commit e66f3185fa04 was right, but its implementation wrong.
> The context omitted an important comment just before the critical test:
> "split_folio() removes folio from list on success."  In ignoring that
> comment, when a THP split succeeded, the code went on to release the
> preceding safe folio, preserving instead an irrelevant (formerly head)
> folio: which gives no safety because it's not on the list. Fix the logic.
> 
> Fixes: e66f3185fa04 ("mm/thp: fix deferred split queue not partially_mapped")
> Signed-off-by: Hugh Dickins <hughd@google.com>

Acked-by: Usama Arif <usamaarif642@gmail.com>


> ---
>  mm/huge_memory.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 03fd4bc39ea1..5734d5d5060f 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3790,7 +3790,9 @@ static unsigned long deferred_split_scan(struct shrinker *shrink,
>  		 * in the case it was underused, then consider it used and
>  		 * don't add it back to split_queue.
>  		 */
> -		if (!did_split && !folio_test_partially_mapped(folio)) {
> +		if (did_split) {
> +			; /* folio already removed from list */
> +		} else if (!folio_test_partially_mapped(folio)) {
>  			list_del_init(&folio->_deferred_list);
>  			removed++;
>  		} else {