linux-kernel - Re: [PATCH 5/8] vrange: Add new vrange(2) system call

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]

Message-ID: <20130612164848.10b93db2@notabene.brown>
Date:	Wed, 12 Jun 2013 16:48:48 +1000
From:	NeilBrown <neilb@...e.de>
To:	John Stultz <john.stultz@...aro.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Minchan Kim <minchan@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	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>,
	Dmitry Adamushko <dmitry.adamushko@...il.com>,
	Dave Chinner <david@...morbit.com>,
	Andrea Righi <andrea@...terlinux.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	Mike Hommey <mh@...ndium.org>, Taras Glek <tglek@...illa.com>,
	Dhaval Giani <dgiani@...illa.com>, Jan Kara <jack@...e.cz>,
	KOSAKI Motohiro <kosaki.motohiro@...il.com>,
	Michel Lespinasse <walken@...gle.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>
Subject: Re: [PATCH 5/8] vrange: Add new vrange(2) system call

On Tue, 11 Jun 2013 21:22:48 -0700 John Stultz <john.stultz@...aro.org> wrote:

> From: Minchan Kim <minchan@...nel.org>
> 
> This patch adds new system call sys_vrange.
> 
> NAME
> 	vrange - Mark or unmark range of memory as volatile
> 
> SYNOPSIS
> 	int vrange(unsigned_long start, size_t length, int mode,
> 			 int *purged);
> 
...
> 
> 	purged: Pointer to an integer which will return 1 if
> 	mode == VRANGE_NONVOLATILE and any page in the affected range
> 	was purged. If purged returns zero during a mode ==
> 	VRANGE_NONVOLATILE call, it means all of the pages in the range
> 	are intact.

This seems a bit ambiguous.
It is clear that the pointed-to location will be set to '1' if any part of
the range was purged, but it is not clear what will happen if it wasn't
purged.
The mention of 'returns zero' seems to suggest that it might set the location
to '0' in that case, but that isn't obvious to me.  The code appear to always
set it - that should be explicit.

Also, should the location be a fixed number of bytes to reduce possible
issues with N-bit userspace on M-bit kernels?

May I suggest:

        purge:  If not NULL, a pointer to a 32bit location which will be set
        to 1 if mode == VRANGE_NONVOLATILE and any page in the affected range
        was purged, and will be set to 0 in all other cases (including
        if mode == VRANGE_VOLATILE).


I don't think any further explanation is needed.


> +	if (purged) {
> +		/* Test pointer is valid before making any changes */
> +		if (put_user(p, purged))
> +			return -EFAULT;
> +	}
> +
> +	ret = do_vrange(mm, start, end - 1, mode, &p);
> +
> +	if (purged) {
> +		if (put_user(p, purged)) {
> +			/*
> +			 * This would be bad, since we've modified volatilty
> +			 * and the change in purged state would be lost.
> +			 */
> +			BUG();
> +		}
> +	}

I agree that would be bad, but I don't think a BUG() is called for.  Maybe a
WARN, and certainly a "return -EFAULT;"

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)