| Message ID | 20190320203338.53367-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND#2] mm/compaction: fix an undefined behaviour | expand |
On Wed, 20 Mar 2019 16:33:38 -0400 Qian Cai <cai@lca.pw> wrote: > In a low-memory situation, cc->fast_search_fail can keep increasing as > it is unable to find an available page to isolate in > fast_isolate_freepages(). As the result, it could trigger an error > below, so just compare with the maximum bits can be shifted first. > > UBSAN: Undefined behaviour in mm/compaction.c:1160:30 > shift exponent 64 is too large for 64-bit type 'unsigned long' > CPU: 131 PID: 1308 Comm: kcompactd1 Kdump: loaded Tainted: G > W L 5.0.0+ #17 > Call trace: > dump_backtrace+0x0/0x450 > show_stack+0x20/0x2c > dump_stack+0xc8/0x14c > __ubsan_handle_shift_out_of_bounds+0x7e8/0x8c4 > compaction_alloc+0x2344/0x2484 > unmap_and_move+0xdc/0x1dbc > migrate_pages+0x274/0x1310 > compact_zone+0x26ec/0x43bc > kcompactd+0x15b8/0x1a24 > kthread+0x374/0x390 > ret_from_fork+0x10/0x18 > > Fixes: 70b44595eafe ("mm, compaction: use free lists to quickly locate a migration source") > Acked-by: Vlastimil Babka <vbabka@suse.cz> > Acked-by: Mel Gorman <mgorman@techsingularity.net> > Signed-off-by: Qian Cai <cai@lca.pw> > --- > mm/compaction.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index e1a08fc92353..0d1156578114 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1157,7 +1157,9 @@ static bool suitable_migration_target(struct compact_control *cc, > static inline unsigned int > freelist_scan_limit(struct compact_control *cc) > { > - return (COMPACT_CLUSTER_MAX >> cc->fast_search_fail) + 1; > + return (COMPACT_CLUSTER_MAX >> > + min((unsigned short)(BITS_PER_LONG - 1), cc->fast_search_fail)) > + + 1; > } That's rather an eyesore. How about static inline unsigned int freelist_scan_limit(struct compact_control *cc) { unsigned short shift = BITS_PER_LONG - 1; return (COMPACT_CLUSTER_MAX >> min(shift, cc->fast_search_fail)) + 1; }
On 3/20/19 5:58 PM, Andrew Morton wrote: >> --- >> mm/compaction.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index e1a08fc92353..0d1156578114 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -1157,7 +1157,9 @@ static bool suitable_migration_target(struct compact_control *cc, >> static inline unsigned int >> freelist_scan_limit(struct compact_control *cc) >> { >> - return (COMPACT_CLUSTER_MAX >> cc->fast_search_fail) + 1; >> + return (COMPACT_CLUSTER_MAX >> >> + min((unsigned short)(BITS_PER_LONG - 1), cc->fast_search_fail)) >> + + 1; >> } > > That's rather an eyesore. How about > > static inline unsigned int > freelist_scan_limit(struct compact_control *cc) > { > unsigned short shift = BITS_PER_LONG - 1; > > return (COMPACT_CLUSTER_MAX >> min(shift, cc->fast_search_fail)) + 1; > } Agree. It looks better.
diff --git a/mm/compaction.c b/mm/compaction.c index e1a08fc92353..0d1156578114 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -1157,7 +1157,9 @@ static bool suitable_migration_target(struct compact_control *cc, static inline unsigned int freelist_scan_limit(struct compact_control *cc) { - return (COMPACT_CLUSTER_MAX >> cc->fast_search_fail) + 1; + return (COMPACT_CLUSTER_MAX >> + min((unsigned short)(BITS_PER_LONG - 1), cc->fast_search_fail)) + + 1; } /*
In a low-memory situation, cc->fast_search_fail can keep increasing as it is unable to find an available page to isolate in fast_isolate_freepages(). As the result, it could trigger an error below, so just compare with the maximum bits can be shifted first. UBSAN: Undefined behaviour in mm/compaction.c:1160:30 shift exponent 64 is too large for 64-bit type 'unsigned long' CPU: 131 PID: 1308 Comm: kcompactd1 Kdump: loaded Tainted: G W L 5.0.0+ #17 Call trace: dump_backtrace+0x0/0x450 show_stack+0x20/0x2c dump_stack+0xc8/0x14c __ubsan_handle_shift_out_of_bounds+0x7e8/0x8c4 compaction_alloc+0x2344/0x2484 unmap_and_move+0xdc/0x1dbc migrate_pages+0x274/0x1310 compact_zone+0x26ec/0x43bc kcompactd+0x15b8/0x1a24 kthread+0x374/0x390 ret_from_fork+0x10/0x18 Fixes: 70b44595eafe ("mm, compaction: use free lists to quickly locate a migration source") Acked-by: Vlastimil Babka <vbabka@suse.cz> Acked-by: Mel Gorman <mgorman@techsingularity.net> Signed-off-by: Qian Cai <cai@lca.pw> --- mm/compaction.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)