linux-kernel - Re: [GIT PULL] please pull file-locking related changes for v3.20

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: <20150217194511.GE27900@fieldses.org>
Date:	Tue, 17 Feb 2015 14:45:11 -0500
From:	"J. Bruce Fields" <bfields@...ldses.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jeff Layton <jlayton@...chiereds.net>,
	"Kirill A. Shutemov" <kirill@...temov.name>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Christoph Hellwig <hch@....de>,
	Dave Chinner <david@...morbit.com>,
	Sasha Levin <sasha.levin@...cle.com>
Subject: Re: [GIT PULL] please pull file-locking related changes for v3.20

On Tue, Feb 17, 2015 at 11:41:40AM -0800, Linus Torvalds wrote:
> On Tue, Feb 17, 2015 at 11:27 AM, Jeff Layton <jlayton@...chiereds.net> wrote:
> >
> > What about this instead then?
> 
> No. Really.
> 
> > - leave the "drop the spinlock" thing in place in flock_lock_file for
> >   v3.20
> 
> No. The whole concept of "drop the lock in the middle" is *BROKEN*.
> It's seriously crap. It's not just a bug, it's a really fundamentally
> wrong thing to do.
> 
> > - change locks_remove_flock to just walk the list and delete any locks
> >   associated with the filp being closed
> 
> No. That's still wrong. You can have two people holding a write-lock.
> Seriously. That's *shit*.
> 
> The "drop the spinlock in the middle" must go. There's not even any
> reason for it. Just get rid of it. There can be no deadlock if you get
> rid of it, because
> 
>  - we hold the flc_lock over the whole event, so we can never see any
> half-way state
> 
>  - if we actually decide to sleep (due to conflicting locks) and
> return FILE_LOCK_DEFERRED, we will drop the lock before actually
> sleeping, so nobody else will be deadlocking on this file lock. So any
> *other* person who tries to do an upgrade will not sleep, because the
> pending upgrade will have moved to the blocking list (that whole
> "locks_insert_block" part.

Whoops, you're right, I was forgetting that wait happens up in
flock_lock_file_wait(), OK.

--b.

> Ergo, either we'll upgrade the lock (atomically, within flc_lock), or
> we will drop the lock (possibly moving it to the blocking list). I
> don't see a deadlock.
> 
> I think your (and mine - but mine had the more fundamental problem of
> never setting "old_fl" correctly at all) patch had a deadlock because
> you didn't actually remove the old lock when you returned
> FILE_LOCK_DEFERRED.
> 
> But I think the correct minimal patch is actually to just remove the
> "if (found)" statement.
> 
>                        Linus
--
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/