[PATCH] ext4: Prevent ext4_kvmalloc re-entring the filesystem and deadlocking

[PATCH] ext4: Prevent ext4_kvmalloc re-entring the filesystem and deadlocking

[Naoto Kobayashi]

Although __vmalloc() doesn't support GFP_NOFS[1],
ext4_kvmalloc/kvzalloc caller may be under GFP_NOFS context
(e.g. fs/ext4/resize.c::add_new_gdb). In such cases, the memory
reclaim can re-entr the filesystem and potentially deadlock.

To prevent the memory relcaim re-entring the filesystem,
use memalloc_nofs_save/restore that gets __vmalloc() to drop
__GFP_FS flag.

[1] linux-tree/Documentation/core-api/gfp-mask-fs-io.rst

Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
---
 fs/ext4/super.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 46b6d5b150ac..4a3c9ee63a34 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -43,6 +43,7 @@
 #include <linux/uaccess.h>
 #include <linux/iversion.h>
 #include <linux/unicode.h>
+#include <linux/sched/mm.h>

 #include <linux/kthread.h>
 #include <linux/freezer.h>
@@ -204,13 +205,32 @@ void ext4_superblock_csum_set(struct super_block *sb)
 	es->s_checksum = ext4_superblock_csum(sb, es);
 }

+static inline void *ext4_vmalloc(size_t size, gfp_t flags, pgprot_t prot)
+{
+	unsigned int nofs_flag = 0;
+	void *ret;
+
+	/**
+	 * Although __vmalloc() doesn't support GFP_NOFS, we may be under
+	 * GFP_NOFS context here. Hence we need to use memalloc_nofs_save()
+	 * to prevent memory reclaim re-entring the filesystem here and
+	 * potentially deadlocking.
+	 */
+	if (!(flags & __GFP_FS))
+		nofs_flag = memalloc_nofs_save();
+	ret = __vmalloc(size, flags, prot);
+	if (!(flags & __GFP_FS))
+		memalloc_nofs_restore(nofs_flag);
+	return ret;
+}
+
 void *ext4_kvmalloc(size_t size, gfp_t flags)
 {
 	void *ret;

 	ret = kmalloc(size, flags | __GFP_NOWARN);
 	if (!ret)
-		ret = __vmalloc(size, flags, PAGE_KERNEL);
+		ret = ext4_vmalloc(size, flags, PAGE_KERNEL);
 	return ret;
 }

@@ -220,7 +240,7 @@ void *ext4_kvzalloc(size_t size, gfp_t flags)

 	ret = kzalloc(size, flags | __GFP_NOWARN);
 	if (!ret)
-		ret = __vmalloc(size, flags | __GFP_ZERO, PAGE_KERNEL);
+		ret = ext4_vmalloc(size, flags | __GFP_ZERO, PAGE_KERNEL);
 	return ret;
 }

--
2.16.5

Re: [PATCH] ext4: Prevent ext4_kvmalloc re-entring the filesystem and deadlocking

[Theodore Y. Ts'o]

On Thu, Dec 26, 2019 at 04:10:08PM +0900, Naoto Kobayashi wrote:
> Although __vmalloc() doesn't support GFP_NOFS[1],
> ext4_kvmalloc/kvzalloc caller may be under GFP_NOFS context
> (e.g. fs/ext4/resize.c::add_new_gdb). In such cases, the memory
> reclaim can re-entr the filesystem and potentially deadlock.
> 
> To prevent the memory relcaim re-entring the filesystem,
> use memalloc_nofs_save/restore that gets __vmalloc() to drop
> __GFP_FS flag.
> 
> [1] linux-tree/Documentation/core-api/gfp-mask-fs-io.rst
> 
> Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>

Good catch!  However, we're not actually using ext4_kvzalloc()
anywhere, at least not any more.  And also, all the users of
ext4_kvmalloc() are using GFP_NOFS (otherwise, they would have been
converted over to use the generic kvmalloc() helper function).

So... a cleaner fix would be to (a) delete ext4_kvmazalloc(), (b)
rename ext4_kvmalloc() to ext4_kvmalloc_nofs(), and drop its flags
argument, and then the calls memalloc_nfs_save/restore() to
ext4_kvmalloc_nofs() --- and so we won't need the

	if (!(flags & __GFP_FS))

test.

Could you make those changes and resend the patch?

Thanks,

					- Ted

Re: [PATCH] ext4: Prevent ext4_kvmalloc re-entring the filesystem and deadlocking

[Naoto Kobayashi]

On 2019/12/26 23:39, Theodore Y. Ts'o wrote:
> On Thu, Dec 26, 2019 at 04:10:08PM +0900, Naoto Kobayashi wrote:
>> Although __vmalloc() doesn't support GFP_NOFS[1],
>> ext4_kvmalloc/kvzalloc caller may be under GFP_NOFS context
>> (e.g. fs/ext4/resize.c::add_new_gdb). In such cases, the memory
>> reclaim can re-entr the filesystem and potentially deadlock.
>>
>> To prevent the memory relcaim re-entring the filesystem,
>> use memalloc_nofs_save/restore that gets __vmalloc() to drop
>> __GFP_FS flag.
>>
>> [1] linux-tree/Documentation/core-api/gfp-mask-fs-io.rst
>>
>> Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
> 
> Good catch!  However, we're not actually using ext4_kvzalloc()
> anywhere, at least not any more.  And also, all the users of
> ext4_kvmalloc() are using GFP_NOFS (otherwise, they would have been
> converted over to use the generic kvmalloc() helper function).
> 
> So... a cleaner fix would be to (a) delete ext4_kvmazalloc(), (b)
> rename ext4_kvmalloc() to ext4_kvmalloc_nofs(), and drop its flags
> argument, and then the calls memalloc_nfs_save/restore() to
> ext4_kvmalloc_nofs() --- and so we won't need the
> 
> 	if (!(flags & __GFP_FS))
> 
> test.
> 
> Could you make those changes and resend the patch?
> 
> Thanks,
> 
> 					- Ted
> 

Thank you for the review! I made the changes you suggested and resent
the patch.

  [PATCH v2 0/3] ext4: Prevent memory reclaim from re-entering the filesystem and deadlocking
  https://marc.info/?l=linux-ext4&m=157743393000807&w=2

Would you review the patch?

Thanks,

                                       - Naoto