Coverity: shrink_node_memcgs(): Null pointer dereferences

DISCUSSION coverity-bot 2 participants 5 messages Nov 12, 2019 Linux-next tree

Share & tools

coverity-bot

author

2019-11-12 01:35 UTC #0 lore raw

Hello!

This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:

c34aa3085f94 ("mm-vmscan-split-shrink_node-into-node-part-and-memcgs-part-fix")

Coverity reported the following:

*** CID 1487844: Null pointer dereferences (NULL_RETURNS) /mm/vmscan.c: 2695 in shrink_node_memcgs()

2689     	memcg = mem_cgroup_iter(target_memcg, NULL, NULL);
2690     	do {
2691     		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
2692     		unsigned long reclaimed;
2693     		unsigned long scanned;
2694
vvv     CID 1487844:  Null pointer dereferences  (NULL_RETURNS)
vvv     Dereferencing a pointer that might be "NULL" "memcg" when calling "mem_cgroup_protected".
2695     		switch (mem_cgroup_protected(target_memcg, memcg)) {
2696     		case MEMCG_PROT_MIN:
2697     			/*
2698     			 * Hard protection.
2699     			 * If there is no reclaimable memory, OOM.
2700     			 */

If this is a false positive, please let us know so we can mark it as such, or teach the Coverity rules to be smarter. If not, please make sure fixes get into linux-next. :) For patches fixing this, please include these lines (but double-check the "Fixes" first):

Reported-by coverity-bot <[email protected]>

Addresses-Coverity-ID: 1487844 ("Null pointer dereferences")

Fixes c34aa3085f94 ("mm-vmscan-split-shrink_node-into-node-part-and-memcgs-part-fix")

Thanks for your attention!

Signature · 1 line

Coverity-bot

Johannes Weiner

2019-11-12 15:21 UTC #1 lore raw

Quoted 20 lines · coverity-bot

Hello!
This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan
You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:
c34aa3085f94 ("mm-vmscan-split-shrink_node-into-node-part-and-memcgs-part-fix")
Coverity reported the following:
*** CID 1487844: Null pointer dereferences (NULL_RETURNS) /mm/vmscan.c: 2695 in shrink_node_memcgs()
2689     	memcg = mem_cgroup_iter(target_memcg, NULL, NULL);
2690     	do {
2691     		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
2692     		unsigned long reclaimed;
2693     		unsigned long scanned;

Quoted 4 lines

2694
vvv CID 1487844: Null pointer dereferences (NULL_RETURNS)
vvv Dereferencing a pointer that might be "NULL" "memcg" when calling "mem_cgroup_protected".
2695 switch (mem_cgroup_protected(target_memcg, memcg)) {

This appears to be a false alarm.

All the "culprit" patch did was rename the local variable "target_memcg".

And while it's correct that memcg can be NULL (befor and after this patch), it's the case only when mem_cgroup_disabled(), and mem_cgroup_protected() checks for this case.

Kees Cook

author

2019-11-12 21:04 UTC #2 lore raw

Quoted 26 lines · Johannes Weiner

Quoted 20 lines · coverity-bot
Hello!
This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan
You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:
c34aa3085f94 ("mm-vmscan-split-shrink_node-into-node-part-and-memcgs-part-fix")
Coverity reported the following:
*** CID 1487844: Null pointer dereferences (NULL_RETURNS) /mm/vmscan.c: 2695 in shrink_node_memcgs()
2689     	memcg = mem_cgroup_iter(target_memcg, NULL, NULL);
2690     	do {
2691     		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
2692     		unsigned long reclaimed;
2693     		unsigned long scanned;
Quoted 4 lines
2694
vvv CID 1487844: Null pointer dereferences (NULL_RETURNS)
vvv Dereferencing a pointer that might be "NULL" "memcg" when calling "mem_cgroup_protected".
2695 switch (mem_cgroup_protected(target_memcg, memcg)) {

Quoted 1 line

This appears to be a false alarm.

Okay, thanks!

Quoted 6 lines

All the "culprit" patch did was rename the local variable "target_memcg".
And while it's correct that memcg can be NULL (befor and after this patch), it's the case only when mem_cgroup_disabled(), and mem_cgroup_protected() checks for this case.

Right, that's certainly the design. I wonder if in the interests of defensively asserting requirements, it would be worth adding something like this to mem_cgroup_protected():

if (WARN_ON_ONCE(!memcg))

    return MEMCG_PROT_NONE;

Signature · 1 line

Kees Cook

Johannes Weiner

2019-11-12 21:56 UTC #3 lore raw

Quoted 38 lines · Kees Cook

Quoted 26 lines · Johannes Weiner
Quoted 20 lines · coverity-bot
Hello!
This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan
You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:
c34aa3085f94 ("mm-vmscan-split-shrink_node-into-node-part-and-memcgs-part-fix")
Coverity reported the following:
*** CID 1487844: Null pointer dereferences (NULL_RETURNS) /mm/vmscan.c: 2695 in shrink_node_memcgs()
2689     	memcg = mem_cgroup_iter(target_memcg, NULL, NULL);
2690     	do {
2691     		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
2692     		unsigned long reclaimed;
2693     		unsigned long scanned;
Quoted 4 lines
2694
vvv CID 1487844: Null pointer dereferences (NULL_RETURNS)
vvv Dereferencing a pointer that might be "NULL" "memcg" when calling "mem_cgroup_protected".
2695 switch (mem_cgroup_protected(target_memcg, memcg)) {
Quoted 1 line
This appears to be a false alarm.
Okay, thanks!
Quoted 6 lines
All the "culprit" patch did was rename the local variable "target_memcg".
And while it's correct that memcg can be NULL (befor and after this patch), it's the case only when mem_cgroup_disabled(), and mem_cgroup_protected() checks for this case.

Quoted 6 lines

Right, that's certainly the design. I wonder if in the interests of defensively asserting requirements, it would be worth adding something like this to mem_cgroup_protected():
if (WARN_ON_ONCE(!memcg))
    return MEMCG_PROT_NONE;

I'm having trouble enumerating the number of places where we would crash in reclaim if memcg were zero while the mem controller is on.

And even if we annotated all of them and dreamed up more or less sensical exit values for all of these places, we'd quickly panic due to failing page reclaim.

There is no graceful exit strategy here. We may as well take the crash right away, without having to clutter up the code.

Kees Cook

author

2019-11-12 22:19 UTC #4 lore raw

Quoted 49 lines · Johannes Weiner

Quoted 38 lines · Kees Cook
Quoted 26 lines · Johannes Weiner
Quoted 20 lines · coverity-bot
Hello!
This is an experimental automated report about issues detected by Coverity from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan
You're getting this email because you were associated with the identified lines of code (noted below) that were touched by recent commits:
c34aa3085f94 ("mm-vmscan-split-shrink_node-into-node-part-and-memcgs-part-fix")
Coverity reported the following:
*** CID 1487844: Null pointer dereferences (NULL_RETURNS) /mm/vmscan.c: 2695 in shrink_node_memcgs()
2689     	memcg = mem_cgroup_iter(target_memcg, NULL, NULL);
2690     	do {
2691     		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, pgdat);
2692     		unsigned long reclaimed;
2693     		unsigned long scanned;
Quoted 4 lines
2694
vvv CID 1487844: Null pointer dereferences (NULL_RETURNS)
vvv Dereferencing a pointer that might be "NULL" "memcg" when calling "mem_cgroup_protected".
2695 switch (mem_cgroup_protected(target_memcg, memcg)) {
Quoted 1 line
This appears to be a false alarm.
Okay, thanks!
Quoted 6 lines
All the "culprit" patch did was rename the local variable "target_memcg".
And while it's correct that memcg can be NULL (befor and after this patch), it's the case only when mem_cgroup_disabled(), and mem_cgroup_protected() checks for this case.
Quoted 6 lines
Right, that's certainly the design. I wonder if in the interests of defensively asserting requirements, it would be worth adding something like this to mem_cgroup_protected():
if (WARN_ON_ONCE(!memcg))
    return MEMCG_PROT_NONE;
I'm having trouble enumerating the number of places where we would crash in reclaim if memcg were zero while the mem controller is on.

Quoted 6 lines

And even if we annotated all of them and dreamed up more or less sensical exit values for all of these places, we'd quickly panic due to failing page reclaim.
There is no graceful exit strategy here. We may as well take the crash right away, without having to clutter up the code.

Okay, cool. I was just thinking mem_cgroup_protected() would be central enough since it's already tested in tons of places.

Thanks for looking at it!

Signature · 1 line

Kees Cook