Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH -next] mm/vmscan: fix an undefined behavior for zone id
@ 2019-11-08 20:44 Qian Cai
  2019-11-08 21:26 ` Qian Cai
  0 siblings, 1 reply; 6+ messages in thread
From: Qian Cai @ 2019-11-08 20:44 UTC (permalink / raw)
  To: akpm; +Cc: mhocko, hannes, guro, linux-mm, cgroups, linux-kernel, Qian Cai

The -next commit "mm: vmscan: simplify lruvec_lru_size()" [1] introduced
an undefined behavior as zone_idx could equal to MAX_NR_ZONES, and then
zid is then out of range.

[ 5399.483257] LTP: starting mtest01w (mtest01 -p80 -w)
[ 5400.245051] ================================================================================
[ 5400.255784] UBSAN: Undefined behaviour in ./include/linux/memcontrol.h:536:26
[ 5400.265235] index 5 is out of range for type 'long unsigned int [5][5]'
[ 5400.273925] CPU: 28 PID: 455 Comm: kswapd7 Tainted: G        W         5.4.0-rc6-next-20191108 #3
[ 5400.285461] Hardware name: HPE ProLiant DL385 Gen10/ProLiant DL385 Gen10, BIOS A40 07/10/2019
[ 5400.295784] Call Trace:
[ 5400.299483]  dump_stack+0x7a/0xaa
[ 5400.304052]  ubsan_epilogue+0x9/0x26
[ 5400.309180]  __ubsan_handle_out_of_bounds.cold.13+0x2b/0x36
[ 5400.316192]  inactive_list_is_low+0x8bb/0x9f0
[ 5400.321952]  balance_pgdat+0x252/0x7d0
[ 5400.327006]  kswapd+0x251/0x590
[ 5400.331725]  ? finish_wait+0x90/0x90
[ 5400.336574]  kthread+0x12a/0x140
[ 5400.341102]  ? balance_pgdat+0x7d0/0x7d0
[ 5400.346330]  ? kthread_create_worker_on_cpu+0x70/0x70
[ 5400.352810]  ret_from_fork+0x27/0x50

[1] https://lore.kernel.org/linux-mm/20191022144803.302233-2-hannes@cmpxchg.org/

Signed-off-by: Qian Cai <cai@lca.pw>
---
 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index d97985262dda..9485b80d6b5b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -317,7 +317,7 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone
 	unsigned long size = 0;
 	int zid;
 
-	for (zid = 0; zid <= zone_idx; zid++) {
+	for (zid = 0; zid < zone_idx; zid++) {
 		struct zone *zone = &lruvec_pgdat(lruvec)->node_zones[zid];
 
 		if (!managed_zone(zone))
-- 
2.21.0 (Apple Git-122.2)



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -next] mm/vmscan: fix an undefined behavior for zone id
  2019-11-08 20:44 [PATCH -next] mm/vmscan: fix an undefined behavior for zone id Qian Cai
@ 2019-11-08 21:26 ` Qian Cai
  2019-11-11 10:12   ` Michal Hocko
  2019-11-11 13:05   ` Chris Down
  0 siblings, 2 replies; 6+ messages in thread
From: Qian Cai @ 2019-11-08 21:26 UTC (permalink / raw)
  To: akpm; +Cc: mhocko, hannes, guro, linux-mm, cgroups, linux-kernel



> On Nov 8, 2019, at 3:44 PM, Qian Cai <cai@lca.pw> wrote:
> 
> -    for (zid = 0; zid <= zone_idx; zid++) {
> +    for (zid = 0; zid < zone_idx; zid++) {
>        struct zone *zone =

Oops, I think here needs to be,

for (zid = 0; zid <= zone_idx && zid < MAX_NR_ZONES; zid++) {

to deal with this MAX_NR_ZONES special case.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -next] mm/vmscan: fix an undefined behavior for zone id
  2019-11-08 21:26 ` Qian Cai
@ 2019-11-11 10:12   ` Michal Hocko
  2019-11-11 13:05   ` Chris Down
  1 sibling, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2019-11-11 10:12 UTC (permalink / raw)
  To: Qian Cai; +Cc: akpm, hannes, guro, linux-mm, cgroups, linux-kernel

On Fri 08-11-19 16:26:52, Qian Cai wrote:
> 
> 
> > On Nov 8, 2019, at 3:44 PM, Qian Cai <cai@lca.pw> wrote:
> > 
> > -    for (zid = 0; zid <= zone_idx; zid++) {
> > +    for (zid = 0; zid < zone_idx; zid++) {
> >        struct zone *zone =
> 
> Oops, I think here needs to be,
> 
> for (zid = 0; zid <= zone_idx && zid < MAX_NR_ZONES; zid++) {
> 
> to deal with this MAX_NR_ZONES special case.

Yep this looks correct.

Thanks!

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -next] mm/vmscan: fix an undefined behavior for zone id
  2019-11-08 21:26 ` Qian Cai
  2019-11-11 10:12   ` Michal Hocko
@ 2019-11-11 13:05   ` Chris Down
  2019-11-11 13:14     ` Chris Down
  1 sibling, 1 reply; 6+ messages in thread
From: Chris Down @ 2019-11-11 13:05 UTC (permalink / raw)
  To: Qian Cai; +Cc: akpm, mhocko, hannes, guro, linux-mm, cgroups, linux-kernel

Qian Cai writes:
>> On Nov 8, 2019, at 3:44 PM, Qian Cai <cai@lca.pw> wrote:
>>
>> -    for (zid = 0; zid <= zone_idx; zid++) {
>> +    for (zid = 0; zid < zone_idx; zid++) {
>>        struct zone *zone =
>
>Oops, I think here needs to be,
>
>for (zid = 0; zid <= zone_idx && zid < MAX_NR_ZONES; zid++) {
>
>to deal with this MAX_NR_ZONES special case.

Ah, I just saw this in my local checkout and thought it was from my changes, 
until I saw it's also on clean mmots checkout. Thanks for the fixup!

Acked-by: Chris Down <chris@chrisdown.name>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -next] mm/vmscan: fix an undefined behavior for zone id
  2019-11-11 13:05   ` Chris Down
@ 2019-11-11 13:14     ` Chris Down
  2019-11-11 13:28       ` Michal Hocko
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Down @ 2019-11-11 13:14 UTC (permalink / raw)
  To: Qian Cai; +Cc: akpm, mhocko, hannes, guro, linux-mm, cgroups, linux-kernel

Chris Down writes:
>Ah, I just saw this in my local checkout and thought it was from my 
>changes, until I saw it's also on clean mmots checkout. Thanks for the 
>fixup!

Also, does this mean we should change callers that may pass through 
zone_idx=MAX_NR_ZONES to become MAX_NR_ZONES-1 in a separate commit, then 
remove this interim fixup? I'm worried otherwise we might paper over real 
issues in future.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH -next] mm/vmscan: fix an undefined behavior for zone id
  2019-11-11 13:14     ` Chris Down
@ 2019-11-11 13:28       ` Michal Hocko
  0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2019-11-11 13:28 UTC (permalink / raw)
  To: Chris Down; +Cc: Qian Cai, akpm, hannes, guro, linux-mm, cgroups, linux-kernel

On Mon 11-11-19 13:14:27, Chris Down wrote:
> Chris Down writes:
> > Ah, I just saw this in my local checkout and thought it was from my
> > changes, until I saw it's also on clean mmots checkout. Thanks for the
> > fixup!
> 
> Also, does this mean we should change callers that may pass through
> zone_idx=MAX_NR_ZONES to become MAX_NR_ZONES-1 in a separate commit, then
> remove this interim fixup? I'm worried otherwise we might paper over real
> issues in future.

Yes, removing this special casing is reasonable. I am not sure
MAX_NR_ZONES - 1 is a better choice though. It is error prone and
zone_idx is the highest zone we should consider and MAX_NR_ZONES - 1
be ZONE_DEVICE if it is configured. But ZONE_DEVICE is really standing
outside of MM reclaim code AFAIK. It would be probably better to have
MAX_LRU_ZONE (equal to MOVABLE) and use it instead.

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 20:44 [PATCH -next] mm/vmscan: fix an undefined behavior for zone id Qian Cai
2019-11-08 21:26 ` Qian Cai
2019-11-11 10:12   ` Michal Hocko
2019-11-11 13:05   ` Chris Down
2019-11-11 13:14     ` Chris Down
2019-11-11 13:28       ` Michal Hocko

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git