linux-kernel - Re: [PATCH] fix bug introduced in "mm: simplify find_vma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 4 Mar 2012 17:22:01 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Mikulas Patocka <mpatocka@...hat.com>
Cc:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Hugh Dickins <hughd@...gle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Shaohua Li <shaohua.li@...el.com>,
	Michal Hocko <mhocko@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fix bug introduced in "mm: simplify find_vma_prev()"

On Sun, Mar 4, 2012 at 4:52 PM, Mikulas Patocka <mpatocka@...hat.com> wrote:
>
> This patch fixes a bug introduced in "mm: simplify find_vma_prev()". You
> can apply this, or alternatively revert the original patch.

Hmm.

I realize the patch is a bug-fix, but I do wonder if we shouldn't look
at alternatives.

In particular, how about we just change the rule to be clearer, and
make the rule be:

 - no more find_vma_prev() AT ALL.

 - callers get turned into "find_vma()" and then we have a
"vma_prev()", which basically does your thing.

 - many of the callers seem to be interested in "prev" *only* if they
found a vma. For example, the do_mlock() kind of logic seems to be
common:

    vma = find_vma_prev(current->mm, start, &prev);
    if (!vma || vma->vm_start > start)
        return -ENOMEM;

   which implies that the current find_vma_prev() semantics are really
wrong, because they force us to do extra work in this case where we
really really don't care about the result.

So we could do an entirely mechanical conversion of

   vma = find_vma_prev(mm, address, &prev_vma);

into

   vma = find_vma(mm, address);
   prev_vma = vma_prev(vma);

and then after we've done that conversion, it looks like the bulk of
them will not care about "prev_vma" if vma is NULL, so they can then
be replaced with a pure

   prev_vma = vma->vm_prev;

instead. Leaving just the (few) cases that may care about the
"previous vma may be the last vma of the VM" case.

Hmm? And we'd get away from that horrible "find_vma_prev()" interface
that uses pointers to vma pointers for return values. Ugh. There only
seems to be nine callers in the whole kernel.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/