| 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
| ||
|
Date: Wed, 16 Jan 2013 17:48:37 -0800
From: John Stultz <john.stultz@...aro.org>
To: Minchan Kim <minchan@...nel.org>
CC: Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org,
Michael Kerrisk <mtk.manpages@...il.com>,
Arun Sharma <asharma@...com>, sanjay@...gle.com,
Paul Turner <pjt@...gle.com>,
David Rientjes <rientjes@...gle.com>,
Christoph Lameter <cl@...ux.com>,
Android Kernel Team <kernel-team@...roid.com>,
Robert Love <rlove@...gle.com>, Mel Gorman <mel@....ul.ie>,
Hugh Dickins <hughd@...gle.com>,
Dave Hansen <dave@...ux.vnet.ibm.com>,
Rik van Riel <riel@...hat.com>,
Dave Chinner <david@...morbit.com>, Neil Brown <neilb@...e.de>,
Mike Hommey <mh@...ndium.org>, Taras Glek <tglek@...illa.com>,
KOSAKI Motohiro <kosaki.motohiro@...il.com>,
KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [RFC 1/8] Introduce new system call mvolatile
On 01/02/2013 08:27 PM, Minchan Kim wrote:
> This patch adds new system call m[no]volatile.
> If someone asks is_volatile system call, it could be added, too.
So some nits below from my initial playing around with this patchset.
> +/*
> + * Return -EINVAL if range doesn't include a right vma at all.
> + * Return -ENOMEM with interrupting range opeartion if memory is not enough to
> + * merge/split vmas.
> + * Return 0 if range consists of only proper vmas.
> + * Return 1 if part of range includes inavlid area(ex, hole/huge/ksm/mlock/
> + * special area)
> + */
> +SYSCALL_DEFINE2(mvolatile, unsigned long, start, size_t, len)
> +{
> + unsigned long end, tmp;
> + struct vm_area_struct *vma, *prev;
> + bool invalid = false;
> + int error = -EINVAL;
> +
> + down_write(¤t->mm->mmap_sem);
> + if (start & ~PAGE_MASK)
> + goto out;
> +
> + len &= PAGE_MASK;
> + if (!len)
> + goto out;
> +
> + end = start + len;
> + if (end < start)
> + goto out;
> +
> + vma = find_vma_prev(current->mm, start, &prev);
> + if (!vma)
> + goto out;
> +
> + if (start > vma->vm_start)
> + prev = vma;
> +
> + for (;;) {
> + /* Here start < (end|vma->vm_end). */
> + if (start < vma->vm_start) {
> + start = vma->vm_start;
> + if (start >= end)
> + goto out;
> + invalid = true;
> + }
> +
> + /* Here vma->vm_start <= start < (end|vma->vm_end) */
> + tmp = vma->vm_end;
> + if (end < tmp)
> + tmp = end;
> +
> + /* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> + error = do_mvolatile(vma, &prev, start, tmp);
> + if (error == -ENOMEM) {
> + up_write(¤t->mm->mmap_sem);
> + return error;
> + }
> + if (error == -EINVAL)
> + invalid = true;
> + else
> + error = 0;
> + start = tmp;
> + if (prev && start < prev->vm_end)
> + start = prev->vm_end;
> + if (start >= end)
> + break;
> +
> + vma = prev->vm_next;
> + if (!vma)
> + break;
> + }
> +out:
> + up_write(¤t->mm->mmap_sem);
> + return invalid ? 1 : 0;
> +}
The error logic here is really strange. If any of the early error cases
are triggered (ie: (start & ~PAGE_MASK), etc), then we jump to out and
return 0 (instead of EINVAL). I don't think that's what you intended.
> +/*
> + * Return -ENOMEM with interrupting range opeartion if memory is not enough
> + * to merge/split vmas.
> + * Return 1 if part of range includes purged's one, otherwise, return 0
> + */
> +SYSCALL_DEFINE2(mnovolatile, unsigned long, start, size_t, len)
> +{
> + unsigned long end, tmp;
> + struct vm_area_struct *vma, *prev;
> + int ret, error = -EINVAL;
> + bool is_purged = false;
> +
> + down_write(¤t->mm->mmap_sem);
> + if (start & ~PAGE_MASK)
> + goto out;
> +
> + len &= PAGE_MASK;
> + if (!len)
> + goto out;
> +
> + end = start + len;
> + if (end < start)
> + goto out;
> +
> + vma = find_vma_prev(current->mm, start, &prev);
> + if (!vma)
> + goto out;
> +
> + if (start > vma->vm_start)
> + prev = vma;
> +
> + for (;;) {
> + /* Here start < (end|vma->vm_end). */
> + if (start < vma->vm_start) {
> + start = vma->vm_start;
> + if (start >= end)
> + goto out;
> + }
> +
> + /* Here vma->vm_start <= start < (end|vma->vm_end) */
> + tmp = vma->vm_end;
> + if (end < tmp)
> + tmp = end;
> +
> + /* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> + error = do_mnovolatile(vma, &prev, start, tmp, &is_purged);
> + if (error) {
> + WARN_ON(error != -ENOMEM);
> + goto out;
> + }
> + start = tmp;
> + if (prev && start < prev->vm_end)
> + start = prev->vm_end;
> + if (start >= end)
> + break;
> +
> + vma = prev->vm_next;
> + if (!vma)
> + break;
> + }
I'm still not sure how this logic improves over the madvise case. If we
catch an error mid-way through setting a series of vmas to non-volatile,
we end up exiting and losing state (ie: if only the first vma was
purged, but half way through 10 vmas we get a ENOMEM error. So the first
vma is now non-volatile, but we do not return the purged flag ).
If we're going to have a new syscall for this (which I'm not sure is the
right approach), we should make use of multiple arguments so we can
return if data was purged, even if we hit an error midway).
Alternatively, if we can find a way to allocate any necessary memory
before we do any vma volatility state changes, then we can return ENOMEM
then and be confident we won't end up with failed partial state change
(this is the approach I used in my fallocate-volatile patches).
thanks
-john
--
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/
Powered by blists - more mailing lists