Coverity: do_migrate_range(): Error handling issues
Hello!
This is an experimental semi-automated report about issues detected by Coverity from a scan of next-20210608 as part of the linux-next 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 commits:
Tue Dec 15 12:13:39 2020 -0800
013339df116c ("mm/rmap: always do TTU_IGNORE_ACCESS")Coverity reported the following:
*** CID 1505146: Error handling issues (CHECKED_RETURN) /mm/memory_hotplug.c: 1422 in do_migrate_range()
1416 * the unmap as the catch all safety net).
1417 */
1418 if (PageHWPoison(page)) {
1419 if (WARN_ON(PageLRU(page)))
1420 isolate_lru_page(page);
1421 if (page_mapped(page))
vvv CID 1505146: Error handling issues (CHECKED_RETURN)
vvv Calling "try_to_unmap" without checking return value (as is done elsewhere 4 out of 5 times).
1422 try_to_unmap(page, TTU_IGNORE_MLOCK);
1423 continue;
1424 }1425
1426 if (!get_page_unless_zero(page))
1427 continue;
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):
Addresses-Coverity-ID: 1505146 ("Error handling issues")
Thanks for your attention!
Signature · 1 line
Coverity-bot
Quoted 6 lines · coverity-bot
You're getting this email because you were associated with the identified lines of code (noted below) that were touched by commits:
Tue Dec 15 12:13:39 2020 -0800
013339df116c ("mm/rmap: always do TTU_IGNORE_ACCESS")
Heh. Entirely independent change, but whatever..
That does mean that the suggested
Quoted 1 line
Fixes 013339df116c ("mm/rmap: always do TTU_IGNORE_ACCESS")
tag would have been entirely bogus. That commit didn't actually change the coverity issue in question anyway, it just happened to change that line in other ways.
Maybe the coverity-bot logic could actually try to see if a coverity report changes, which it wouldn't have done here.
The report itself is:
Quoted 4 lines
1421 if (
page_mapped(page))
vvv CID 1505146: Error handling issues (CHECKED_RETURN)
vvv Calling "try_to_unmap" without checking return value (as is done elsewhere 4 out of 5 times).
1422try_to_unmap(page, TTU_IGNORE_MLOCK);
Hmm. That "4 out of 5 times" seems misleading. It implies that 80% of callers check the return value.
But when I actually grep for it, I see 4 call-sites that don't check the return value, and 5 that do. So it's more like 55% of callers that do check.
Anyway, this is a false positive. You don't have to check the return value. The main example of doing so is the page-out code in mm/vmscan.c, and it does so just to keep track of success/fail statistics.
Linus
Quoted 1 line · Linus Torvalds
[...]
Quoted 5 lines
Anyway, this is a false positive. You don't have to check the return value. The main example of doing so is the page-out code in
mm/vmscan.c, and it does so just to keep track of success/fail statistics.
BTW there is an in-fly patch to make the try_to_umap() void function.
Quoted 8 lines · Linus Torvalds
But when I actually grep for it, I see 4 call-sites that don't check the return value, and 5 that do. So it's more like 55% of callers that do check.
Anyway, this is a false positive. You don't have to check the return value. The main example of doing so is the page-out code in
mm/vmscan.c, and it does so just to keep track of success/fail statistics.
Okay, cool; thanks for taking a closer look at it.
Signature · 1 line
Kees Cook