linux-kernel - Re: [PATCH 1/2] shmem: fix faulting into a hole, not taking i

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, 16 Jul 2014 09:18:47 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Hugh Dickins <hughd@...gle.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Sasha Levin <sasha.levin@...cle.com>,
	Konstantin Khlebnikov <koct9i@...il.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Michel Lespinasse <walken@...gle.com>,
	Lukas Czerner <lczerner@...hat.com>,
	Dave Jones <davej@...hat.com>, linux-mm@...ck.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] shmem: fix faulting into a hole, not taking i_mutex

On 07/15/2014 09:26 PM, Hugh Dickins wrote:
>> 
>> > @@ -760,7 +760,7 @@ static int shmem_writepage(struct page *
>> >   			spin_lock(&inode->i_lock);
>> >   			shmem_falloc = inode->i_private;
>> 
>> Without ACCESS_ONCE, can shmem_falloc potentially become an alias on
>> inode->i_private and later become re-read outside of the lock?
> 
> No, it could be re-read inside the locked section (which is okay since
> the locking ensures the same value would be re-read each time), but it
> cannot be re-read after the unlock.  The unlock guarantees that (whereas
> an assignment after the unlock might be moved up before the unlock).
> 
> I searched for a simple example (preferably not in code written by me!)
> to convince you.  I thought it would be easy to find an example of
> 
> 	spin_lock(&lock);
> 	thing_to_free = whatever;
> 	spin_unlock(&lock);
> 	if (thing_to_free)
> 		free(thing_to_free);
> 
> but everything I hit upon was actually a little more complicated than
> than that (e.g. involving whatever(), or setting whatever = NULL after),
> and therefore less convincing.  Please hunt around to convince yourself.

Yeah, I thought myself on the way home that this is probably the case. I guess
some recent bugs made me too paranoid. Sorry for the noise and time you spent
explaining this :/

>> 
>> > -		if (!shmem_falloc ||
>> > -		    shmem_falloc->mode != FALLOC_FL_PUNCH_HOLE ||
>> > -		    vmf->pgoff < shmem_falloc->start ||
>> > -		    vmf->pgoff >= shmem_falloc->next)
>> > -			shmem_falloc = NULL;
>> > -		spin_unlock(&inode->i_lock);
>> > -		/*
>> > -		 * i_lock has protected us from taking shmem_falloc seriously
>> > -		 * once return from shmem_fallocate() went back up that
>> > stack.
>> > -		 * i_lock does not serialize with i_mutex at all, but it does
>> > -		 * not matter if sometimes we wait unnecessarily, or
>> > sometimes
>> > -		 * miss out on waiting: we just need to make those cases
>> > rare.
>> > -		 */
>> > -		if (shmem_falloc) {
>> > +		if (shmem_falloc &&
>> > +		    shmem_falloc->waitq &&
>> 
>> Here it's operating outside of lock.
> 
> No, it's inside the lock: just easier to see from the patched source
> than from the patch itself.

Ah, right :/

> Hugh
> 

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