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 PHC | |
Open Source and information security mailing list archives
| ||
|
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/
Powered by blists - more mailing lists