tmpfs: fix use-after-free of mempolicy object

This patch fixes several mempolicy leaks in the tmpfs mount logic. These leaks are slow - on the order of one object leaked per mount attempt.

Leak 1 (umount doesn't free mpol allocated in mount):
while true; do
mount -t tmpfs -o mpol=interleave,size=100M nodev /mnt umount /mnt
done

Leak 2 (errors parsing remount options will leak mpol):
mount -t tmpfs -o size=100M nodev /mnt
while true; do
mount -o remount,mpol=interleave,size=x /mnt 2> /dev/null
done
umount /mnt

Leak 3 (multiple mpol per mount leak mpol):
while true; do
mount -t tmpfs -o mpol=interleave,mpol=interleave,size=100M nodev /mnt umount /mnt
done

This patch fixes all of the above. I could have broken the patch into three pieces but is seemed easier to review as one.

Signed-off-by Greg Thelen <[email protected]>

---
 mm/shmem.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

mm/shmem.c +2 -1

index efd0b3a..ed2cb26 100644
@@ -2386,6 +2386,7 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
 			       bool remount)
 {
 	char *this_char, *value, *rest;
+	struct mempolicy *mpol = NULL;
 	uid_t uid;
 	gid_t gid;
 

@@ -2414,7 +2415,7 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
 			printk(KERN_ERR
 			    "tmpfs: No value for mount option '%s'\n",
 			    this_char);
-			return 1;
+			goto error;
 		}
 
 		if (!strcmp(this_char,"size")) {

mm/shmem.c +6 -1

+			if (mpol_parse_str(value, &mpol))
 				goto bad_val;
 		} else {
 			printk(KERN_ERR "tmpfs: Bad mount option %s\n",
 			       this_char);
-			return 1;
+			goto error;
 		}
 	}
+	sbinfo->mpol = mpol;
 	return 0;
 
 bad_val:
 	printk(KERN_ERR "tmpfs: Bad value '%s' for mount option '%s'\n",
 	       value, this_char);
+error:
+	mpol_put(mpol);
 	return 1;
 
 }

@@ -2551,6 +2556,7 @@ static void shmem_put_super(struct super_block *sb)
 	struct shmem_sb_info *sbinfo = SHMEM_SB(sb);
 
 	percpu_counter_destroy(&sbinfo->used_blocks);
+	mpol_put(sbinfo->mpol);
 	kfree(sbinfo);
 	sb->s_fs_info = NULL;
 }

Signature · 1 line

1.8.1.3

mm/shmem.c static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo, · line 2467

Quoted 6 lines

mm/shmem.c +1 -1

@@ -2463,19 +2464,23 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
 			if (!gid_valid(sbinfo->gid))
 				goto bad_val;
 		} else if (!strcmp(this_char,"mpol")) {
-			if (mpol_parse_str(value, &sbinfo->mpol))
+			mpol_put(mpol);

I haven't tested to check, but don't we need

			mpol = NULL;

here, in case the new option turns out to be bad?

Quoted 23 lines · Greg Thelen

This patch fixes several mempolicy leaks in the tmpfs mount logic. These leaks are slow - on the order of one object leaked per mount attempt.

Leak 1 (umount doesn't free mpol allocated in mount):
while true; do
mount -t tmpfs -o mpol=interleave,size=100M nodev /mnt umount /mnt
done

Leak 2 (errors parsing remount options will leak mpol):
mount -t tmpfs -o size=100M nodev /mnt
while true; do
mount -o remount,mpol=interleave,size=x /mnt 2> /dev/null
done
umount /mnt

Leak 3 (multiple mpol per mount leak mpol):
while true; do
mount -t tmpfs -o mpol=interleave,mpol=interleave,size=100M nodev /mnt umount /mnt
done

Quoted 2 lines

This patch fixes all of the above. I could have broken the patch into three pieces but is seemed easier to review as one.

Yes, I agree, and nicely fixed - but one doubt below. If you resolve that, please add my Acked-by: Hugh Dickins <[email protected]>

Quoted 28 lines

Signed-off-by Greg Thelen <[email protected]>

---
  mm/shmem.c | 12 +++++++++---
  1 file changed, 9 insertions(+), 3 deletions(-)

mm/shmem.c +2 -1

index efd0b3a..ed2cb26 100644
@@ -2386,6 +2386,7 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
  			       bool remount)
  {
  	char *this_char, *value, *rest;
+	struct mempolicy *mpol = NULL;
  	uid_t uid;
  	gid_t gid;
 

@@ -2414,7 +2415,7 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
  			printk(KERN_ERR
  			    "tmpfs: No value for mount option '%s'\n",
  			    this_char);
-			return 1;
+			goto error;
  		}
 
  		if (!strcmp(this_char,"size")) {

mm/shmem.c static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo, · line 2467

Quoted 5 lines

mm/shmem.c +1 -1

@@ -2463,19 +2464,23 @@ static int shmem_parse_options(char *options, struct shmem_sb_info *sbinfo,
  			if (!gid_valid(sbinfo->gid))
  				goto bad_val;
  		} else if (!strcmp(this_char,"mpol")) {
-			if (mpol_parse_str(value, &sbinfo->mpol))
+			mpol_put(mpol);

I haven't tested to check, but don't we need

			mpol = NULL;

here, in case the new option turns out to be bad?

Quoted 23 lines

mm/shmem.c +5 -1

+			if (mpol_parse_str(value, &mpol))
  				goto bad_val;
  		} else {
  			printk(KERN_ERR "tmpfs: Bad mount option %s\n",
  			       this_char);
-			return 1;
+			goto error;
  		}
  	}
+	sbinfo->mpol = mpol;
  	return 0;
 
  bad_val:
  	printk(KERN_ERR "tmpfs: Bad value '%s' for mount option '%s'\n",
  	       value, this_char);
+error:
+	mpol_put(mpol);
  	return 1;
 
  }

@@ -2551,6 +2556,7 @@ static void shmem_put_super(struct super_block *sb)
  	struct shmem_sb_info *sbinfo = SHMEM_SB(sb);
 

Quoted 7 lines

mm/shmem.c +1 -0

  	percpu_counter_destroy(&sbinfo->used_blocks);
+	mpol_put(sbinfo->mpol);
  	kfree(sbinfo);
  	sb->s_fs_info = NULL;
  }

Signature · 1 line

1.8.1.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Could you explain me why shmem has more relationship with mempolicy? It seems that there are many codes in shmem handle mempolicy, but other components in mm subsystem just have little.

Quoted 3 lines · Will Huck

Could you explain me why shmem has more relationship with mempolicy? It seems that there are many codes in shmem handle mempolicy, but other components in mm subsystem just have little.

NUMA mempolicy is mostly handled in mm/mempolicy.c, which services the mbind, migrate_pages, set_mempolicy, get_mempolicy system calls: which govern how process memory is distributed across NUMA nodes.

mm/shmem.c is affected because it was also found useful to specify mempolicy on the shared memory objects which may back process memory:
that includes SysV SHM and POSIX shared memory and tmpfs. mm/hugetlb.c contains some mempolicy handling for hugetlbfs; fs/ramfs is kept minimal, so nothing in there.

Those are the memory-based filesystems, where NUMA mempolicy is most natural. The regular filesystems could support shared mempolicy too, but that would raise more awkward design questions.

Hugh

The tmpfs remount logic preserves filesystem mempolicy if the mpol=M option is not specified in the remount request. A new policy can be specified if mpol=M is given.

Before this patch remounting an mpol bound tmpfs without specifying mpol= mount option in the remount request would set the filesystem's mempolicy object to a freed mempolicy object.

To reproduce the problem boot a DEBUG_PAGEALLOC kernel and run:

    # mkdir /tmp/x

    # mount -t tmpfs -o size=100M,mpol=interleave nodev /tmp/x

    # grep /tmp/x /proc/mounts
    nodev /tmp/x tmpfs rw,relatime,size=102400k,mpol=interleave:0-3 0 0

    # mount -o remount,size=200M nodev /tmp/x

    # grep /tmp/x /proc/mounts
    nodev /tmp/x tmpfs rw,relatime,size=204800k,mpol=??? 0 0
        # note ? garbage in mpol=... output above

    # dd if=/dev/zero of=/tmp/x/f count=1
        # panic here

Panic:

    BUG: unable to handle kernel NULL pointer dereference at           (null)
    IP: [<          (null)>]           (null)
    [...]
    Oops: 0010 [#1] SMP DEBUG_PAGEALLOC
    Call Trace:
     [<ffffffff81186ead>] ? mpol_set_nodemask+0x8d/0x100
     [<ffffffff811895ef>] ? mpol_shared_policy_init+0x8f/0x160
     [<ffffffff81189605>] mpol_shared_policy_init+0xa5/0x160
     [<ffffffff811580e1>] ? shmem_get_inode+0x1e1/0x270
     [<ffffffff811580e1>] ? shmem_get_inode+0x1e1/0x270
     [<ffffffff810db15d>] ? trace_hardirqs_on+0xd/0x10
     [<ffffffff81158109>] shmem_get_inode+0x209/0x270
     [<ffffffff811581ae>] shmem_mknod+0x3e/0xf0
     [<ffffffff811582b8>] shmem_create+0x18/0x20
     [<ffffffff811af5d5>] vfs_create+0xb5/0x130
     [<ffffffff811afff1>] do_last+0x9a1/0xea0
     [<ffffffff811ac77a>] ? link_path_walk+0x7a/0x930
     [<ffffffff811b05a3>] path_openat+0xb3/0x4d0
     [<ffffffff811be831>] ? __alloc_fd+0x31/0x160
     [<ffffffff811b0de2>] do_filp_open+0x42/0xa0
     [<ffffffff811be8e0>] ? __alloc_fd+0xe0/0x160
     [<ffffffff811a066e>] do_sys_open+0xfe/0x1e0
     [<ffffffff811f0aeb>] compat_sys_open+0x1b/0x20
     [<ffffffff815d6055>] cstar_dispatch+0x7/0x1f

Non-debug kernels will not crash immediately because referencing the dangling mpol will not cause a fault. Instead the filesystem will reference a freed mempolicy object, which will cause unpredictable behavior.

The problem boils down to a dropped mpol reference below if shmem_parse_options() does not allocate a new mpol:
config = *sbinfo
shmem_parse_options(data, &config, true) mpol_put(sbinfo->mpol)
sbinfo->mpol = config.mpol /* BUG: saves unreferenced mpol */

This patch avoids the crash by not releasing the mempolicy if shmem_parse_options() doesn't create a new mpol.

How far back does this issue go? I see it in both 2.6.36 and 3.3. I did not look back further.

Signed-off-by Greg Thelen <[email protected]>

---
 mm/shmem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

mm/shmem.c +8 -2

index 5dd56f6..efd0b3a 100644
@@ -2487,6 +2487,7 @@ static int shmem_remount_fs(struct super_block *sb, int *flags, char *data)
 	unsigned long inodes;
 	int error = -EINVAL;
 
+	config.mpol = NULL;
 	if (shmem_parse_options(data, &config, true))
 		return error;
 

@@ -2511,8 +2512,13 @@ static int shmem_remount_fs(struct super_block *sb, int *flags, char *data)
 	sbinfo->max_inodes  = config.max_inodes;
 	sbinfo->free_inodes = config.max_inodes - inodes;
 
-	mpol_put(sbinfo->mpol);
-	sbinfo->mpol        = config.mpol;	/* transfers initial ref */
+	/*
+	 * Preserve previous mempolicy unless mpol remount option was specified.
+	 */
+	if (config.mpol) {
+		mpol_put(sbinfo->mpol);
+		sbinfo->mpol = config.mpol;	/* transfers initial ref */
+	}
 out:
 	spin_unlock(&sbinfo->stat_lock);
 	return error;

Signature · 1 line

1.8.1.3