lkml.org 
[lkml]   [2014]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>



\
 
 \ /
  Last update: 2014-07-16 10:01    [W:0.050 / U:1.248 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site