Skip to contentSkip to conversation

mm: Fix the uninitialized use in overcommit_policy_handler

PATCH Superseded Chen Jun 3 participants 4 messages Sep 22, 2021 – Sep 23, 2021

Subsystems (2)
Memory management MM core

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Chen Jun

author
#0 lore raw

PATCH v2 1/1 mm: Fix the uninitialized use in overcommit_policy_handler

An unexpected value of /proc/sys/vm/panic_on_oom we will get, after running the following program.

int main()

{
    int fd = open("/proc/sys/vm/panic_on_oom", O_RDWR)
    write(fd, "1", 1);
    write(fd, "2", 1);
    close(fd);
}

write(fd, "2", 1) will pass *ppos = 1 to proc_dointvec_minmax. proc_dointvec_minmax will return 0 without setting new_policy.

t.data = &new_policy;
ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos)

      -->do_proc_dointvec
         -->__do_proc_dointvec
              if (write) {
                if (proc_first_pos_non_zero_ignore(ppos, table))
                  goto out;

sysctl_overcommit_memory = new_policy;

so sysctl_overcommit_memory will be set to an uninitialized value.

Check whether new_policy has been changed by proc_dointvec_minmax.

Fixes 56f3547bfa4d ("mm: adjust vm_committed_as_batch according to vm overcommit policy"
Signed-off-by Chen Jun <[email protected]>

---

v2:
* Check whether new_policy has been changed by proc_dointvec_minmax.

 mm/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mm/util.c +2 -2
index 4ddb6e186dd5..d5be67771850 100644
@@ -756,7 +756,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
 		size_t *lenp, loff_t *ppos)
 {
 	struct ctl_table t;
-	int new_policy;
+	int new_policy = -1;
 	int ret;
 
 	/*

@@ -774,7 +774,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
 		t = *table;
 		t.data = &new_policy;
 		ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
-		if (ret)
+		if (ret || new_policy == -1)
 			return ret;
 
 		mm_compute_batch(new_policy);
Signature · 1 line
2.17.1
reviewer Acked-by on v2 1/1
#1 lore raw
Quoted 22 lines · Chen Jun

An unexpected value of /proc/sys/vm/panic_on_oom we will get, after running the following program.

int main()

{
    int fd = open("/proc/sys/vm/panic_on_oom", O_RDWR)
    write(fd, "1", 1);
    write(fd, "2", 1);
    close(fd);
}

write(fd, "2", 1) will pass *ppos = 1 to proc_dointvec_minmax. proc_dointvec_minmax will return 0 without setting new_policy.

t.data = &new_policy;
ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos)

      -->do_proc_dointvec
         -->__do_proc_dointvec
              if (write) {
                if (proc_first_pos_non_zero_ignore(ppos, table))
                  goto out;
Quoted 8 lines

sysctl_overcommit_memory = new_policy;

so sysctl_overcommit_memory will be set to an uninitialized value.

Check whether new_policy has been changed by proc_dointvec_minmax.

Fixes 56f3547bfa4d ("mm: adjust vm_committed_as_batch according to vm overcommit policy"
Signed-off-by Chen Jun <[email protected]>
Acked-by Michal Hocko <[email protected]>

Btw. you could also check for new_policy == sysctl_overcommit_memory so that the sync of pcp counters is not fired without the policy change. Something for a separate patch I guess.

Quoted 32 lines

---

v2:
* Check whether new_policy has been changed by proc_dointvec_minmax.

 mm/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mm/util.c +2 -2
index 4ddb6e186dd5..d5be67771850 100644
@@ -756,7 +756,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
 		size_t *lenp, loff_t *ppos)
 {
 	struct ctl_table t;
-	int new_policy;
+	int new_policy = -1;
 	int ret;
 
 	/*

@@ -774,7 +774,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
 		t = *table;
 		t.data = &new_policy;
 		ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
-		if (ret)
+		if (ret || new_policy == -1)
 			return ret;
 
 		mm_compute_batch(new_policy);
Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko
SUSE Labs
Reviewed-by on v2 1/1
#2 lore raw
In reply to Chen Jun

Hi Jun,

Quoted 6 lines · Chen Jun

An unexpected value of /proc/sys/vm/panic_on_oom we will get, after running the following program.

int main()

{
    int fd = open("/proc/sys/vm/panic_on_oom", O_RDWR)

should this be "/proc/sys/vm/overcommit_memory"

Quoted 16 lines
    write(fd, "1", 1);
    write(fd, "2", 1);
    close(fd);
}

write(fd, "2", 1) will pass *ppos = 1 to proc_dointvec_minmax. proc_dointvec_minmax will return 0 without setting new_policy.

t.data = &new_policy;
ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos)

      -->do_proc_dointvec
         -->__do_proc_dointvec
              if (write) {
                if (proc_first_pos_non_zero_ignore(ppos, table))
                  goto out;
Quoted 5 lines

sysctl_overcommit_memory = new_policy;

so sysctl_overcommit_memory will be set to an uninitialized value.

Check whether new_policy has been changed by proc_dointvec_minmax.

Other than the nit above, it looks good to me, thanks!

Reviewed-by Feng Tang <[email protected]>
Quoted 35 lines
Fixes 56f3547bfa4d ("mm: adjust vm_committed_as_batch according to vm overcommit policy"
Signed-off-by Chen Jun <[email protected]>

---

v2:
* Check whether new_policy has been changed by proc_dointvec_minmax.

 mm/util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
mm/util.c +2 -2
index 4ddb6e186dd5..d5be67771850 100644
@@ -756,7 +756,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
 		size_t *lenp, loff_t *ppos)
 {
 	struct ctl_table t;
-	int new_policy;
+	int new_policy = -1;
 	int ret;
 
 	/*

@@ -774,7 +774,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
 		t = *table;
 		t.data = &new_policy;
 		ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
-		if (ret)
+		if (ret || new_policy == -1)
 			return ret;
 
 		mm_compute_batch(new_policy);
Signature · 1 line
2.17.1
author
#3 lore raw

Hi Tang,

Quoted 12 lines · Feng Tang

Hi Jun,

Quoted 6 lines · Chen Jun

An unexpected value of /proc/sys/vm/panic_on_oom we will get, after running the following program.

int main()

{
     int fd = open("/proc/sys/vm/panic_on_oom", O_RDWR)

should this be "/proc/sys/vm/overcommit_memory"

Sorry, that is my mistake, I will correct it in v3. Thanks.

Quoted 62 lines
Quoted 16 lines
     write(fd, "1", 1);
     write(fd, "2", 1);
     close(fd);
}

write(fd, "2", 1) will pass *ppos = 1 to proc_dointvec_minmax. proc_dointvec_minmax will return 0 without setting new_policy.

t.data = &new_policy;
ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos)

       -->do_proc_dointvec
          -->__do_proc_dointvec
               if (write) {
                 if (proc_first_pos_non_zero_ignore(ppos, table))
                   goto out;
Quoted 5 lines

sysctl_overcommit_memory = new_policy;

so sysctl_overcommit_memory will be set to an uninitialized value.

Check whether new_policy has been changed by proc_dointvec_minmax.

Other than the nit above, it looks good to me, thanks!

Reviewed-by Feng Tang <[email protected]>
Quoted 35 lines
Fixes 56f3547bfa4d ("mm: adjust vm_committed_as_batch according to vm overcommit policy"
Signed-off-by Chen Jun <[email protected]>

---

v2:
* Check whether new_policy has been changed by proc_dointvec_minmax.

  mm/util.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
mm/util.c +2 -2
index 4ddb6e186dd5..d5be67771850 100644
@@ -756,7 +756,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
  		size_t *lenp, loff_t *ppos)
  {
  	struct ctl_table t;
-	int new_policy;
+	int new_policy = -1;
  	int ret;
 
  	/*

@@ -774,7 +774,7 @@ int overcommit_policy_handler(struct ctl_table *table, int write, void *buffer,
  		t = *table;
  		t.data = &new_policy;
  		ret = proc_dointvec_minmax(&t, write, buffer, lenp, ppos);
-		if (ret)
+		if (ret || new_policy == -1)
  			return ret;
 
  		mm_compute_batch(new_policy);
Signature · 1 line
2.17.1
Signature · 2 lines
Regards
Chen Jun