linux-kernel - Re: [PATCH RFC] mm: account VMA before forced-COW via /proc/pid/mem

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:	Wed, 4 Apr 2012 17:41:48 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Konstantin Khlebnikov <khlebnikov@...nvz.org>
Cc:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Hugh Dickins <hughd@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH RFC] mm: account VMA before forced-COW via /proc/pid/mem

On 04/04, Konstantin Khlebnikov wrote:
>
> Oleg Nesterov wrote:
>> On 04/02, Konstantin Khlebnikov wrote:
>>>
>>> Currently kernel does not account read-only private mappings into memory commitment.
>>> But these mappings can be force-COW-ed in get_user_pages().
>>
>> Heh. tail -n3 Documentation/vm/overcommit-accounting
>> may be you should update it then.
>
> I just wonder how fragile this accounting...

I meant, this patch could also remove this "TODO" from the docs.

>> Can't really comment the patch, this is not my area. Still,
>>
>>> +	down_write(&mm->mmap_sem);
>>> +	*pvma = vma = find_vma(mm, addr);
>>> +	if (vma&&  vma->vm_start<= addr) {
>>> +		ret = vma->vm_end - addr;
>>> +		if ((vma->vm_flags&  (VM_ACCOUNT | VM_NORESERVE | VM_SHARED |
>>> +				VM_HUGETLB | VM_MAYWRITE)) == VM_MAYWRITE) {
>>> +			if (!security_vm_enough_memory_mm(mm, vma_pages(vma)))
>>
>> Oooooh, the whole vma. Say, gdb installs the single breakpoint into
>> the huge .text mapping...
>
> We cannot split vma right there, this will be really weird. =)

Sure, I understand why you did it this way.

>> I am not sure, but probably you want to check at least VM_IO/PFNMAP
>> as well. We do not want to charge this memory and retry with FOLL_FORCE
>> before vm_ops->access(). Say, /dev/mem
>
> No, VM_IO/PFNMAP aren't affect accounting, there is VM_NORESERVE for this.

You misunderstood. Again, I can be wrong, but.

Suppose the task mmmaps /dev/mem (for example). This vma doesn't have
VM_NORESERVE (but it has VM_IO).

gup() fails correctly with or without FOLL_FORCE, we should fallback
to vma_ops->access().

However. With your patch __access_remote_vm() tries gup() without
FOLL_FORCE first and wrongly assumes that it fails because it neeeds
FOLL_FORCE and we are going to force-cow.

So __account_vma() adds VM_ACCOUNT before (unnecessary) retry, and
this is unnecessary too and wrong.

>> Hmm. OTOH, if I am right then mprotect_fixup() should be fixed??
>
> mprotect_fixup() does not account area if it already accounted, so all ok.

No, I meant another thing. But yes, I think I was wrong, mprotect_fixup()
is fine.

>> We drop ->mmap_sem... Say, the task does mremap() in between and
>> len == 2 * PAGE_SIZE. Then, for example, copy_to_user_page() can
>> write to the same page twice. Perhaps not a problem in practice,
>> I dunno.
>
> I have an old unfinished patch which implements upgrade_read() for rw-semaphore =)

Interesting ;)

Oleg.

--
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/