mm: swapfile: avoid split_swap_cluster() NULL pointer dereference

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

CONFIG_THP_SWAP exposes cluster_info infrastructure to a broader number of use cases, and split_swap_cluster(), which is the counterpart of split_huge_page() for the THPs in the swapcache, misses checking the return of lock_cluster before operating on the cluster_info pointer.

This patch addresses that issue by adding a proper check for the pointer not being NULL in the wrappers cluster_{is,clear}_huge(), in order to avoid crashes similar to the one below:

...

Fixes 59807685a7e77 ("mm, THP, swap: support splitting THP for THP swap out")

Signed-off-by Rafael Aquini <[email protected]>

Quoted 14 lines · On Tue, 22 Sep 2020 14:48:38 -0400 Rafael Aquini <[email protected]> wrote:

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

CONFIG_THP_SWAP exposes cluster_info infrastructure to a broader number of use cases, and split_swap_cluster(), which is the counterpart of split_huge_page() for the THPs in the swapcache, misses checking the return of lock_cluster before operating on the cluster_info pointer.

This patch addresses that issue by adding a proper check for the pointer not being NULL in the wrappers cluster_{is,clear}_huge(), in order to avoid crashes similar to the one below:

Quoted 4 lines

...

Fixes 59807685a7e77 ("mm, THP, swap: support splitting THP for THP swap out")

Signed-off-by Rafael Aquini <[email protected]>

Did you consider cc:stable?

Quoted 18 lines · Andrew Morton

Quoted 16 lines · On Tue, 22 Sep 2020 14:48:38 -0400 Rafael Aquini <[email protected]> wrote:

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

CONFIG_THP_SWAP exposes cluster_info infrastructure to a broader number of use cases, and split_swap_cluster(), which is the counterpart of split_huge_page() for the THPs in the swapcache, misses checking the return of lock_cluster before operating on the cluster_info pointer.

This patch addresses that issue by adding a proper check for the pointer not being NULL in the wrappers cluster_{is,clear}_huge(), in order to avoid crashes similar to the one below:

...

Quoted 5 lines

Quoted 2 lines

Fixes 59807685a7e77 ("mm, THP, swap: support splitting THP for THP swap out")

Signed-off-by Rafael Aquini <[email protected]>

Did you consider cc:stable?

UGH! I missed adding it to my cc list. Shall I just forward it, now, or do you prefer a fresh repost?

Quoted 22 lines · On Wed, 23 Sep 2020 09:42:51 -0400 Rafael Aquini <[email protected]> wrote:

Quoted 21 lines · Andrew Morton

Quoted 18 lines · On Tue, 22 Sep 2020 14:48:38 -0400 Rafael Aquini <[email protected]> wrote:

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

CONFIG_THP_SWAP exposes cluster_info infrastructure to a broader number of use cases, and split_swap_cluster(), which is the counterpart of split_huge_page() for the THPs in the swapcache, misses checking the return of lock_cluster before operating on the cluster_info pointer.

This patch addresses that issue by adding a proper check for the pointer not being NULL in the wrappers cluster_{is,clear}_huge(), in order to avoid crashes similar to the one below:

...

Fixes 59807685a7e77 ("mm, THP, swap: support splitting THP for THP swap out")

Signed-off-by Rafael Aquini <[email protected]>

Quoted 5 lines

Quoted 2 lines

Did you consider cc:stable?

UGH! I missed adding it to my cc list. Shall I just forward it, now, or do you prefer a fresh repost?

I added the cc:stable to my copy.

Quoted 4 lines

Quoted 2 lines

UGH! I missed adding it to my cc list. Shall I just forward it, now, or do you prefer a fresh repost?

I added the cc:stable to my copy.

Please don't merge this patch. This patch doesn't fix the bug, but hide the real bug. I will work with Rafael on root causing and fixing.

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

CONFIG_THP_SWAP exposes cluster_info infrastructure to a broader number of use cases, and split_swap_cluster(), which is the counterpart of split_huge_page() for the THPs in the swapcache, misses checking the return of lock_cluster before operating on the cluster_info pointer.

This patch addresses that issue by adding a proper check for the pointer not being NULL in the wrappers cluster_{is,clear}_huge(), in order to avoid crashes similar to the one below:

[ 5758.157556] BUG: kernel NULL pointer dereference, address: 0000000000000007
[ 5758.165331] #PF: supervisor write access in kernel mode
[ 5758.171161] #PF: error_code(0x0002) - not-present page
[ 5758.176894] PGD 0 P4D 0
[ 5758.179721] Oops: 0002 [#1] SMP PTI
[ 5758.183614] CPU: 10 PID: 316 Comm: kswapd1 Kdump: loaded Tainted: G S               --------- ---  5.9.0-0.rc3.1.tst.el8.x86_64 #1
[ 5758.196717] Hardware name: Intel Corporation S2600CP/S2600CP, BIOS SE5C600.86B.02.01.0002.082220131453 08/22/2013
[ 5758.208176] RIP: 0010:split_swap_cluster+0x47/0x60
[ 5758.213522] Code: c1 e3 06 48 c1 eb 0f 48 8d 1c d8 48 89 df e8 d0 20 6a 00 80 63 07 fb 48 85 db 74 16 48 89 df c6 07 00 66 66 66 90 31 c0 5b c3 <80> 24 25 07 00 00 00 fb 31 c0 5b c3 b8 f0 ff ff ff 5b c3 66 0f 1f
[ 5758.234478] RSP: 0018:ffffb147442d7af0 EFLAGS: 00010246
[ 5758.240309] RAX: 0000000000000000 RBX: 000000000014b217 RCX: ffffb14779fd9000
[ 5758.248281] RDX: 000000000014b217 RSI: ffff9c52f2ab1400 RDI: 000000000014b217
[ 5758.256246] RBP: ffffe00c51168080 R08: ffffe00c5116fe08 R09: ffff9c52fffd3000
[ 5758.264208] R10: ffffe00c511537c8 R11: ffff9c52fffd3c90 R12: 0000000000000000
[ 5758.272172] R13: ffffe00c51170000 R14: ffffe00c51170000 R15: ffffe00c51168040
[ 5758.280134] FS:  0000000000000000(0000) GS:ffff9c52f2a80000(0000) knlGS:0000000000000000
[ 5758.289163] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5758.295575] CR2: 0000000000000007 CR3: 0000000022a0e003 CR4: 00000000000606e0
[ 5758.303538] Call Trace:
[ 5758.306273]  split_huge_page_to_list+0x88b/0x950
[ 5758.311433]  deferred_split_scan+0x1ca/0x310
[ 5758.316202]  do_shrink_slab+0x12c/0x2a0
[ 5758.320491]  shrink_slab+0x20f/0x2c0
[ 5758.324482]  shrink_node+0x240/0x6c0
[ 5758.328469]  balance_pgdat+0x2d1/0x550
[ 5758.332652]  kswapd+0x201/0x3c0
[ 5758.336157]  ? finish_wait+0x80/0x80
[ 5758.340147]  ? balance_pgdat+0x550/0x550
[ 5758.344525]  kthread+0x114/0x130
[ 5758.348126]  ? kthread_park+0x80/0x80
[ 5758.352214]  ret_from_fork+0x22/0x30
[ 5758.356203] Modules linked in: fuse zram rfkill sunrpc intel_rapl_msr intel_rapl_common sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp mgag200 iTCO_wdt crct10dif_pclmul iTCO_vendor_support drm_kms_helper crc32_pclmul ghash_clmulni_intel syscopyarea sysfillrect sysimgblt fb_sys_fops cec rapl joydev intel_cstate ipmi_si ipmi_devintf drm intel_uncore i2c_i801 ipmi_msghandler pcspkr lpc_ich mei_me i2c_smbus mei ioatdma ip_tables xfs libcrc32c sr_mod sd_mod cdrom t10_pi sg igb ahci libahci i2c_algo_bit crc32c_intel libata dca wmi dm_mirror dm_region_hash dm_log dm_mod
[ 5758.412673] CR2: 0000000000000007
[    0.000000] Linux version 5.9.0-0.rc3.1.tst.el8.x86_64 ([email protected]) (gcc (GCC) 8.3.1 20191121 (Red Hat 8.3.1-5), GNU ld version 2.30-79.el8) #1 SMP Wed Sep 9 16:03:34 EDT 2020

Fixes 59807685a7e77 ("mm, THP, swap: support splitting THP for THP swap out")

Signed-off-by Rafael Aquini <[email protected]>

---
 mm/swapfile.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

mm/swapfile.c +3 -2

index 12f59e641b5e..37ddf5e5c53b 100644
@@ -324,14 +324,15 @@ static inline void cluster_set_null(struct swap_cluster_info *info)
 
 static inline bool cluster_is_huge(struct swap_cluster_info *info)
 {
-	if (IS_ENABLED(CONFIG_THP_SWAP))
+	if (IS_ENABLED(CONFIG_THP_SWAP) && info)
 		return info->flags & CLUSTER_FLAG_HUGE;
 	return false;
 }
 
 static inline void cluster_clear_huge(struct swap_cluster_info *info)
 {
-	info->flags &= ~CLUSTER_FLAG_HUGE;
+	if (IS_ENABLED(CONFIG_THP_SWAP) && info)
+		info->flags &= ~CLUSTER_FLAG_HUGE;
 }
 
 static inline struct swap_cluster_info *lock_cluster(struct swap_info_struct *si,

Hi, Rafael,

Quoted 4 lines · Rafael Aquini

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

Thanks for reporting. But the bug looks strange. Because in a system with only HDD swap devices, during THP swap out, the swap cluster shouldn't be allocated, as in

shrink_page_list()
add_to_swap()

    get_swap_page()
      get_swap_pages()
        swap_alloc_cluster()

Quoted 13 lines · Ying

Hi, Rafael,

Quoted 4 lines · Rafael Aquini

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

Thanks for reporting. But the bug looks strange. Because in a system with only HDD swap devices, during THP swap out, the swap cluster shouldn't be allocated, as in

Quoted 6 lines

shrink_page_list()
add_to_swap()

    get_swap_page()
      get_swap_pages()
        swap_alloc_cluster()

The underlying problem is that swap_info_struct.cluster_info is always NULL on the rotational storage case.

So, it's very easy to follow that constructions
like this one, in split_swap_cluster

...

        ci = lock_cluster(si, offset);
        cluster_clear_huge(ci);

...

will go for a NULL pointer dereference, in that case, given that lock_cluster reads:

...

	struct swap_cluster_info *ci;
        ci = si->cluster_info;
        if (ci) {
                ci += offset / SWAPFILE_CLUSTER;
                spin_lock(&ci->lock);
        }
        return ci;

...

Quoted 21 lines · Rafael Aquini

Quoted 13 lines · Ying

Hi, Rafael,

Quoted 4 lines · Rafael Aquini

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

Thanks for reporting. But the bug looks strange. Because in a system with only HDD swap devices, during THP swap out, the swap cluster shouldn't be allocated, as in

Quoted 6 lines

shrink_page_list()
add_to_swap()

    get_swap_page()
      get_swap_pages()
        swap_alloc_cluster()

Quoted 2 lines

The underlying problem is that swap_info_struct.cluster_info is always NULL on the rotational storage case.

Yes.

Quoted 12 lines

So, it's very easy to follow that constructions
like this one, in split_swap_cluster

...

        ci = lock_cluster(si, offset);
        cluster_clear_huge(ci);

...

will go for a NULL pointer dereference, in that case, given that lock_cluster reads:

...

Quoted 8 lines

	struct swap_cluster_info *ci;
        ci = si->cluster_info;
        if (ci) {
                ci += offset / SWAPFILE_CLUSTER;
                spin_lock(&ci->lock);
        }
        return ci;

...

But on HDD, we shouldn't call split_swap_cluster() at all, because we will not allocate swap cluster firstly. So, if we run into this, there should be some other bug, we need to figure it out.

Quoted 49 lines · Ying

Quoted 21 lines · Rafael Aquini

Quoted 13 lines · Ying

Hi, Rafael,

Quoted 4 lines · Rafael Aquini

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

Thanks for reporting. But the bug looks strange. Because in a system with only HDD swap devices, during THP swap out, the swap cluster shouldn't be allocated, as in

Quoted 6 lines

shrink_page_list()
add_to_swap()

    get_swap_page()
      get_swap_pages()
        swap_alloc_cluster()

Quoted 2 lines

The underlying problem is that swap_info_struct.cluster_info is always NULL on the rotational storage case.

Yes.

Quoted 12 lines

So, it's very easy to follow that constructions
like this one, in split_swap_cluster

...

        ci = lock_cluster(si, offset);
        cluster_clear_huge(ci);

...

will go for a NULL pointer dereference, in that case, given that lock_cluster reads:

...

Quoted 8 lines

	struct swap_cluster_info *ci;
        ci = si->cluster_info;
        if (ci) {
                ci += offset / SWAPFILE_CLUSTER;
                spin_lock(&ci->lock);
        }
        return ci;

...

Quoted 4 lines

But on HDD, we shouldn't call split_swap_cluster() at all, because we will not allocate swap cluster firstly. So, if we run into this, there should be some other bug, we need to figure it out.

split_swap_cluster() gets called by split_huge_page_to_list(), if the page happens to be in the swapcache, and it will always go that way, regardless the backing storage type:

...

            __split_huge_page(page, list, end, flags);
            if (PageSwapCache(head)) {
                    swp_entry_t entry = { .val = page_private(head) };

                    ret = split_swap_cluster(entry);
            } else
                    ret = 0;

...

The problem is not about allocating the swap_cluster -- it's obviously not allocated in these cases. The problem is that on rotational storage you don't even have the base structure that allows you to keep the swap clusters (cluster_info does not get allocated, at all, so si->cluster_info is always NULL)

You can argue about other bugs all you want, it doesn't change the fact that this code is incomplete as it sits, because it misses checking for a real case where lock_cluster() will return NULL.

Quoted 75 lines · Rafael Aquini

Quoted 49 lines · Ying

Quoted 21 lines · Rafael Aquini

Quoted 13 lines · Ying

Hi, Rafael,

Quoted 4 lines · Rafael Aquini

The swap area descriptor only gets struct swap_cluster_info *cluster_info allocated if the swapfile is backed by non-rotational storage. When the swap area is laid on top of ordinary disk spindles, lock_cluster() will naturally return NULL.

Thanks for reporting. But the bug looks strange. Because in a system with only HDD swap devices, during THP swap out, the swap cluster shouldn't be allocated, as in

Quoted 6 lines

shrink_page_list()
add_to_swap()

    get_swap_page()
      get_swap_pages()
        swap_alloc_cluster()

Quoted 2 lines

The underlying problem is that swap_info_struct.cluster_info is always NULL on the rotational storage case.

Yes.

Quoted 12 lines

So, it's very easy to follow that constructions
like this one, in split_swap_cluster

...

        ci = lock_cluster(si, offset);
        cluster_clear_huge(ci);

...

will go for a NULL pointer dereference, in that case, given that lock_cluster reads:

...

Quoted 8 lines

	struct swap_cluster_info *ci;
        ci = si->cluster_info;
        if (ci) {
                ci += offset / SWAPFILE_CLUSTER;
                spin_lock(&ci->lock);
        }
        return ci;

...

Quoted 4 lines

But on HDD, we shouldn't call split_swap_cluster() at all, because we will not allocate swap cluster firstly. So, if we run into this, there should be some other bug, we need to figure it out.

split_swap_cluster() gets called by split_huge_page_to_list(), if the page happens to be in the swapcache, and it will always go that way, regardless the backing storage type:

...

            __split_huge_page(page, list, end, flags);
            if (PageSwapCache(head)) {
                    swp_entry_t entry = { .val = page_private(head) };

                    ret = split_swap_cluster(entry);
            } else
                    ret = 0;

...

The problem is not about allocating the swap_cluster -- it's obviously not allocated in these cases. The problem is that on rotational storage you don't even have the base structure that allows you to keep the swap clusters (cluster_info does not get allocated, at all, so si->cluster_info is always NULL)

Quoted 3 lines

You can argue about other bugs all you want, it doesn't change the fact that this code is incomplete as it sits, because it misses checking for a real case where lock_cluster() will return NULL.

I don't want to argue about anything. I just want to fix the bug. The fix here will hide the real bug instead of fixing it. For the situation you described (PageSwapCache() returns true for a THP backed by a normal swap entry (not swap cluster)), we will run into other troubles too. So we need to find the root cause and fix it.

The bug here is quite simple: split_swap_cluster() misses checking for lock_cluster() returning NULL before committing to change cluster_info->flags.

The fundamental problem has nothing to do with allocating, or not allocating a swap cluster, but it has to do with the fact that the THP deferred split scan can transiently race with swapcache insertion, and the fact that when you run your swap area on rotational storage cluster_info is _always_ NULL. split_swap_cluster() needs to check for lock_cluster() returning NULL because that's one possible case, and it clearly fails to do so.

Run a workload that cause multiple THP COW, and add a memory hogger to create memory pressure so you'll force the reclaimers to kick the registered shrinkers. The trigger is not heavy swapping, and that's probably why most swap test cases don't hit it. The window is tight, but you will get the NULL pointer dereference.

Regardless you find furhter bugs, or not, this patch is needed to correct a blunt coding mistake.

Quoted 2 lines · Rafael Aquini

The bug here is quite simple: split_swap_cluster() misses checking for lock_cluster() returning NULL before committing to change cluster_info->flags.

I don't think so. We shouldn't run into this situation firstly. So the "fix" hides the real bug instead of fixing it. Just like we call VM_BUG_ON_PAGE(!PageLocked(head), head) in split_huge_page_to_list() instead of returning if !PageLocked(head) silently.

Quoted 6 lines

The fundamental problem has nothing to do with allocating, or not allocating a swap cluster, but it has to do with the fact that the THP deferred split scan can transiently race with swapcache insertion, and the fact that when you run your swap area on rotational storage cluster_info is _always_ NULL. split_swap_cluster() needs to check for lock_cluster() returning NULL because that's one possible case, and it clearly fails to do so.

If there's a race, we should fix the race. But the code path for swapcache insertion is,

add_to_swap()
get_swap_page() /* Return if fails to allocate */ add_to_swap_cache()
SetPageSwapCache()

While the code path to split THP is,

split_huge_page_to_list()
if PageSwapCache()
split_swap_cluster()

Both code paths are protected by the page lock. So there should be some other reasons to trigger the bug.

And again, for HDD, a THP shouldn't have PageSwapCache() set at the first place. If so, the bug is that the flag is set and we should fix the setting.

Quoted 5 lines

Run a workload that cause multiple THP COW, and add a memory hogger to create memory pressure so you'll force the reclaimers to kick the registered shrinkers. The trigger is not heavy swapping, and that's probably why most swap test cases don't hit it. The window is tight, but you will get the NULL pointer dereference.

Do you have a script to reproduce the bug?

Quoted 2 lines

Regardless you find furhter bugs, or not, this patch is needed to correct a blunt coding mistake.

As above. I don't agree with that.

Quoted 15 lines

If there's a race, we should fix the race. But the code path for swapcache insertion is,

add_to_swap()
get_swap_page() /* Return if fails to allocate */ add_to_swap_cache()
SetPageSwapCache()

While the code path to split THP is,

split_huge_page_to_list()
if PageSwapCache()
split_swap_cluster()

Quoted 2 lines

Both code paths are protected by the page lock. So there should be some other reasons to trigger the bug.

As mentioned above, no they seem to not be protected (at least, not the same page, depending on the case). While add_to_swap() will assure a page_lock on the compound head, split_huge_page_to_list() does not.

Quoted 9 lines · Ying

Quoted 2 lines · Rafael Aquini

The bug here is quite simple: split_swap_cluster() misses checking for lock_cluster() returning NULL before committing to change cluster_info->flags.

I don't think so. We shouldn't run into this situation firstly. So the "fix" hides the real bug instead of fixing it. Just like we call VM_BUG_ON_PAGE(!PageLocked(head), head) in split_huge_page_to_list() instead of returning if !PageLocked(head) silently.

Not the same thing, obviously, as you are going for an apples-to-carrots comparison, but since you mentioned:

split_huge_page_to_list() asserts (in debug builds) *page is locked,

and later checks if *head bears the SwapCache flag. deferred_split_scan(), OTOH, doesn't hand down the compound head locked, but the 2nd page in the group instead.

This doesn't necessarely means it's a problem, though, but might help on hitting the issue.

Quoted 15 lines

Quoted 6 lines

The fundamental problem has nothing to do with allocating, or not allocating a swap cluster, but it has to do with the fact that the THP deferred split scan can transiently race with swapcache insertion, and the fact that when you run your swap area on rotational storage cluster_info is _always_ NULL. split_swap_cluster() needs to check for lock_cluster() returning NULL because that's one possible case, and it clearly fails to do so.

If there's a race, we should fix the race. But the code path for swapcache insertion is,

add_to_swap()
get_swap_page() /* Return if fails to allocate */ add_to_swap_cache()
SetPageSwapCache()

Quoted 8 lines

While the code path to split THP is,

split_huge_page_to_list()
if PageSwapCache()
split_swap_cluster()

Both code paths are protected by the page lock. So there should be some other reasons to trigger the bug.

As mentioned above, no they seem to not be protected (at least, not the same page, depending on the case). While add_to_swap() will assure a page_lock on the compound head, split_huge_page_to_list() does not.

Quoted 4 lines

And again, for HDD, a THP shouldn't have PageSwapCache() set at the first place. If so, the bug is that the flag is set and we should fix the setting.

I fail to follow your claim here. Where is the guarantee, in the code, that you'll never have a compound head in the swapcache?

Quoted 6 lines

Quoted 5 lines

Run a workload that cause multiple THP COW, and add a memory hogger to create memory pressure so you'll force the reclaimers to kick the registered shrinkers. The trigger is not heavy swapping, and that's probably why most swap test cases don't hit it. The window is tight, but you will get the NULL pointer dereference.

Quoted 2 lines

Do you have a script to reproduce the bug?

Nope, a convoluted set of internal regression tests we have usually triggers it. In the wild, customers running HANNA are seeing it, occasionally.

Quoted 5 lines

Quoted 2 lines

Regardless you find furhter bugs, or not, this patch is needed to correct a blunt coding mistake.

As above. I don't agree with that.

It's OK to disagree, split_swap_cluster still misses the cluster_info NULL check, though.

Quoted 11 lines · Rafael Aquini

Quoted 9 lines · Ying

Quoted 2 lines · Rafael Aquini

The bug here is quite simple: split_swap_cluster() misses checking for lock_cluster() returning NULL before committing to change cluster_info->flags.

I don't think so. We shouldn't run into this situation firstly. So the "fix" hides the real bug instead of fixing it. Just like we call VM_BUG_ON_PAGE(!PageLocked(head), head) in split_huge_page_to_list() instead of returning if !PageLocked(head) silently.

Quoted 4 lines

Not the same thing, obviously, as you are going for an apples-to-carrots comparison, but since you mentioned:

split_huge_page_to_list() asserts (in debug builds) *page is locked,

	VM_BUG_ON_PAGE(!PageLocked(head), head);

It asserts *head instead of *page.

Quoted 3 lines

and later checks if *head bears the SwapCache flag. deferred_split_scan(), OTOH, doesn't hand down the compound head locked, but the 2nd page in the group instead.

No. deferred_split_scan() will can trylock_page() on the 2nd page in the group, but

static inline int trylock_page(struct page *page)

{
	page = compound_head(page);
	return (likely(!test_and_set_bit_lock(PG_locked, &page->flags)));
}

So the head page will be locked instead.

Quoted 32 lines

This doesn't necessarely means it's a problem, though, but might help on hitting the issue.

Quoted 15 lines

Quoted 6 lines

The fundamental problem has nothing to do with allocating, or not allocating a swap cluster, but it has to do with the fact that the THP deferred split scan can transiently race with swapcache insertion, and the fact that when you run your swap area on rotational storage cluster_info is _always_ NULL. split_swap_cluster() needs to check for lock_cluster() returning NULL because that's one possible case, and it clearly fails to do so.

If there's a race, we should fix the race. But the code path for swapcache insertion is,

add_to_swap()
get_swap_page() /* Return if fails to allocate */ add_to_swap_cache()
SetPageSwapCache()

Quoted 8 lines

While the code path to split THP is,

split_huge_page_to_list()
if PageSwapCache()
split_swap_cluster()

Both code paths are protected by the page lock. So there should be some other reasons to trigger the bug.

As mentioned above, no they seem to not be protected (at least, not the same page, depending on the case). While add_to_swap() will assure a page_lock on the compound head, split_huge_page_to_list() does not.

Quoted 7 lines

Quoted 4 lines

And again, for HDD, a THP shouldn't have PageSwapCache() set at the first place. If so, the bug is that the flag is set and we should fix the setting.

I fail to follow your claim here. Where is the guarantee, in the code, that you'll never have a compound head in the swapcache?

We may have a THP in the swap cache, only if non-rotational disk is used as swap device. This is the design assumption of the THP swap support. And this is guaranteed via swap space allocation for THP will fail for HDD. If the implementation doesn't guarantee this, we will fix the implementation to guarantee this.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Run a workload that cause multiple THP COW, and add a memory hogger to create memory pressure so you'll force the reclaimers to kick the registered shrinkers. The trigger is not heavy swapping, and that's probably why most swap test cases don't hit it. The window is tight, but you will get the NULL pointer dereference.

Quoted 6 lines

Quoted 2 lines

Do you have a script to reproduce the bug?

Nope, a convoluted set of internal regression tests we have usually triggers it. In the wild, customers running HANNA are seeing it, occasionally.

So you haven't reproduce the bug on upstream kernel?

Or, can you help to run the test with a debug kernel based on upstream kernel. I can provide some debug patch.

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Regardless you find furhter bugs, or not, this patch is needed to correct a blunt coding mistake.

As above. I don't agree with that.

Quoted 2 lines

It's OK to disagree, split_swap_cluster still misses the cluster_info NULL check, though.

In contrast, if the checking is necessary, we shouldn't ignore it, but use something like

        ci = lock_cluster(si, offset);

+ VM_BUG_ON(!ci);

Quoted 3 lines

Or, can you help to run the test with a debug kernel based on upstream kernel. I can provide some debug patch.

Sure, I can set your patches to run with the test cases we have that tend to reproduce the issue with some degree of success.

Quoted 6 lines · Rafael Aquini

Quoted 3 lines

Or, can you help to run the test with a debug kernel based on upstream kernel. I can provide some debug patch.

Sure, I can set your patches to run with the test cases we have that tend to reproduce the issue with some degree of success.

Thanks!

I found a race condition. During THP splitting, "head" may be unlocked before calling split_swap_cluster(), because head != page during deferred splitting. So we should call split_swap_cluster() before unlocking. The debug patch to do that is as below. Can you help to test it?

Best Regards,
Huang, Ying

------------------------8<----------------------------
From 24ce0736a9f587d2dba12f12491c88d3e296a491 Mon Sep 17 00:00:00 2001
From: Huang Ying <[email protected]>
Date: Fri, 25 Sep 2020 11:10:56 +0800
Subject: [PATCH] dbg: Call split_swap_clsuter() before unlock page during
split THP

---
 mm/huge_memory.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

mm/huge_memory.c +7 -6

index faadc449cca5..8d79e5e6b46e 100644
@@ -2444,6 +2444,12 @@ static void __split_huge_page(struct page *page, struct list_head *list,
 
 	remap_page(head);
 
+	if (PageSwapCache(head)) {
+		swp_entry_t entry = { .val = page_private(head) };
+
+		split_swap_cluster(entry);
+	}
+
 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		struct page *subpage = head + i;
 		if (subpage == page)

@@ -2678,12 +2684,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		}
 
 		__split_huge_page(page, list, end, flags);
-		if (PageSwapCache(head)) {
-			swp_entry_t entry = { .val = page_private(head) };
-
-			ret = split_swap_cluster(entry);
-		} else
-			ret = 0;
+		ret = 0;
 	} else {
 		if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
 			pr_alert("total_mapcount: %u, page_count(): %u\n",

Signature · 1 line

2.28.0

Quoted 8 lines · Ying

Quoted 6 lines · Rafael Aquini

Quoted 3 lines

Or, can you help to run the test with a debug kernel based on upstream kernel. I can provide some debug patch.

Sure, I can set your patches to run with the test cases we have that tend to reproduce the issue with some degree of success.

Quoted 8 lines

Thanks!

I found a race condition. During THP splitting, "head" may be unlocked before calling split_swap_cluster(), because head != page during deferred splitting. So we should call split_swap_cluster() before unlocking. The debug patch to do that is as below. Can you help to test it?

I finally could grab a good crashdump and confirm that head is really not locked.

I still need to dig into it to figure out more about the crash. I guess that your patch will guarantee that lock on head, but it still doesn't help on explaining how did we get the THP marked as PG_swapcache, given that it should fail add_to_swap()->get_swap_page() right?

I'll give your patch a run over the weekend, hopefully we'll have more info on this next week.

Quoted 48 lines

Best Regards,
Huang, Ying

------------------------8<----------------------------
From 24ce0736a9f587d2dba12f12491c88d3e296a491 Mon Sep 17 00:00:00 2001
From: Huang Ying <[email protected]>
Date: Fri, 25 Sep 2020 11:10:56 +0800
Subject: [PATCH] dbg: Call split_swap_clsuter() before unlock page during
split THP

---
 mm/huge_memory.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

mm/huge_memory.c +7 -6

index faadc449cca5..8d79e5e6b46e 100644
@@ -2444,6 +2444,12 @@ static void __split_huge_page(struct page *page, struct list_head *list,
 
 	remap_page(head);
 
+	if (PageSwapCache(head)) {
+		swp_entry_t entry = { .val = page_private(head) };
+
+		split_swap_cluster(entry);
+	}
+
 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		struct page *subpage = head + i;
 		if (subpage == page)

@@ -2678,12 +2684,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		}
 
 		__split_huge_page(page, list, end, flags);
-		if (PageSwapCache(head)) {
-			swp_entry_t entry = { .val = page_private(head) };
-
-			ret = split_swap_cluster(entry);
-		} else
-			ret = 0;
+		ret = 0;
 	} else {
 		if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
 			pr_alert("total_mapcount: %u, page_count(): %u\n",

Signature · 1 line

2.28.0

Quoted 6 lines · Rafael Aquini

Quoted 3 lines

Or, can you help to run the test with a debug kernel based on upstream kernel. I can provide some debug patch.

Sure, I can set your patches to run with the test cases we have that tend to reproduce the issue with some degree of success.

Thanks!

I found a race condition. During THP splitting, "head" may be unlocked before calling split_swap_cluster(), because head != page during deferred splitting. So we should call split_swap_cluster() before unlocking. The debug patch to do that is as below. Can you help to test it?

Best Regards,
Huang, Ying

------------------------8<----------------------------
From 24ce0736a9f587d2dba12f12491c88d3e296a491 Mon Sep 17 00:00:00 2001
From: Huang Ying <[email protected]>
Date: Fri, 25 Sep 2020 11:10:56 +0800
Subject: [PATCH] dbg: Call split_swap_clsuter() before unlock page during
split THP

---
 mm/huge_memory.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

mm/huge_memory.c +7 -6

index faadc449cca5..8d79e5e6b46e 100644
@@ -2444,6 +2444,12 @@ static void __split_huge_page(struct page *page, struct list_head *list,
 
 	remap_page(head);
 
+	if (PageSwapCache(head)) {
+		swp_entry_t entry = { .val = page_private(head) };
+
+		split_swap_cluster(entry);
+	}
+
 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		struct page *subpage = head + i;
 		if (subpage == page)

@@ -2678,12 +2684,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		}
 
 		__split_huge_page(page, list, end, flags);
-		if (PageSwapCache(head)) {
-			swp_entry_t entry = { .val = page_private(head) };
-
-			ret = split_swap_cluster(entry);
-		} else
-			ret = 0;
+		ret = 0;
 	} else {
 		if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
 			pr_alert("total_mapcount: %u, page_count(): %u\n",

Signature · 1 line

2.28.0

Quoted 64 lines · Ying

Quoted 6 lines · Rafael Aquini

Quoted 3 lines

Or, can you help to run the test with a debug kernel based on upstream kernel. I can provide some debug patch.

Sure, I can set your patches to run with the test cases we have that tend to reproduce the issue with some degree of success.

Thanks!

I found a race condition. During THP splitting, "head" may be unlocked before calling split_swap_cluster(), because head != page during deferred splitting. So we should call split_swap_cluster() before unlocking. The debug patch to do that is as below. Can you help to test it?

Best Regards,
Huang, Ying

------------------------8<----------------------------
From 24ce0736a9f587d2dba12f12491c88d3e296a491 Mon Sep 17 00:00:00 2001
From: Huang Ying <[email protected]>
Date: Fri, 25 Sep 2020 11:10:56 +0800
Subject: [PATCH] dbg: Call split_swap_clsuter() before unlock page during
split THP

---
 mm/huge_memory.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

mm/huge_memory.c +7 -6

index faadc449cca5..8d79e5e6b46e 100644
@@ -2444,6 +2444,12 @@ static void __split_huge_page(struct page *page, struct list_head *list,
 
 	remap_page(head);
 
+	if (PageSwapCache(head)) {
+		swp_entry_t entry = { .val = page_private(head) };
+
+		split_swap_cluster(entry);
+	}
+
 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		struct page *subpage = head + i;
 		if (subpage == page)

@@ -2678,12 +2684,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		}
 
 		__split_huge_page(page, list, end, flags);
-		if (PageSwapCache(head)) {
-			swp_entry_t entry = { .val = page_private(head) };
-
-			ret = split_swap_cluster(entry);
-		} else
-			ret = 0;
+		ret = 0;
 	} else {
 		if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
 			pr_alert("total_mapcount: %u, page_count(): %u\n",

Signature · 1 line

2.28.0

I left it running for several days, on several systems that had seen the crash hitting before, and no crashes were observed for either the upstream kernel nor the distro build 4.18-based kernel.

I guess we can comfortably go with your patch. Thanks!

Quoted 70 lines · Rafael Aquini

Quoted 64 lines · Ying

Quoted 6 lines · Rafael Aquini

Quoted 3 lines

Or, can you help to run the test with a debug kernel based on upstream kernel. I can provide some debug patch.

Sure, I can set your patches to run with the test cases we have that tend to reproduce the issue with some degree of success.

Thanks!

I found a race condition. During THP splitting, "head" may be unlocked before calling split_swap_cluster(), because head != page during deferred splitting. So we should call split_swap_cluster() before unlocking. The debug patch to do that is as below. Can you help to test it?

Best Regards,
Huang, Ying

------------------------8<----------------------------
From 24ce0736a9f587d2dba12f12491c88d3e296a491 Mon Sep 17 00:00:00 2001
From: Huang Ying <[email protected]>
Date: Fri, 25 Sep 2020 11:10:56 +0800
Subject: [PATCH] dbg: Call split_swap_clsuter() before unlock page during
split THP

---
 mm/huge_memory.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

mm/huge_memory.c +7 -6

index faadc449cca5..8d79e5e6b46e 100644
@@ -2444,6 +2444,12 @@ static void __split_huge_page(struct page *page, struct list_head *list,
 
 	remap_page(head);
 
+	if (PageSwapCache(head)) {
+		swp_entry_t entry = { .val = page_private(head) };
+
+		split_swap_cluster(entry);
+	}
+
 	for (i = 0; i < HPAGE_PMD_NR; i++) {
 		struct page *subpage = head + i;
 		if (subpage == page)

@@ -2678,12 +2684,7 @@ int split_huge_page_to_list(struct page *page, struct list_head *list)
 		}
 
 		__split_huge_page(page, list, end, flags);
-		if (PageSwapCache(head)) {
-			swp_entry_t entry = { .val = page_private(head) };
-
-			ret = split_swap_cluster(entry);
-		} else
-			ret = 0;
+		ret = 0;
 	} else {
 		if (IS_ENABLED(CONFIG_DEBUG_VM) && mapcount) {
 			pr_alert("total_mapcount: %u, page_count(): %u\n",

Signature · 1 line

2.28.0

I left it running for several days, on several systems that had seen the crash hitting before, and no crashes were observed for either the upstream kernel nor the distro build 4.18-based kernel.

Quoted 3 lines

I guess we can comfortably go with your patch. Thanks!

Ping

Are you going to post this patchfix soon? Or do you rather have me posting it?