Re: [PATCH 1/2] shmem: fix faulting into a hole, not taking i_mutex

[Vlastimil Babka]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/