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]

Date:	Tue, 17 Feb 2015 14:08:44 -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 Mon, Feb 16, 2015 at 11:24:03AM -0800, Linus Torvalds wrote:
> On Mon, Feb 16, 2015 at 10:46 AM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
> >
> > This code is so broken that my initial reaction is "We need to just
> > revert the crap".
> 
> How the hell is flock_lock_file() supposed to work at all, btw?
> 
> Say we have an existing flock, and now do a new one that conflicts. I
> see what looks like three separate bugs.
> 
>  - We go through the first loop, find a lock of another type, and
> delete it in preparation for replacing it
> 
>  - we *drop* the lock context spinlock.
> 
>  - BUG #1? So now there is no lock at all, and somebody can come in
> and see that unlocked state. Is that really valid?
> 
>  - another thread comes in while the first thread dropped the lock
> context lock, and wants to add its own lock. It doesn't see the
> deleted or pending locks, so it just adds it
> 
>  - the first thread gets the context spinlock again, and adds the lock
> that replaced the original
> 
>  - BUG #2? So now there are *two* locks on the thing, and the next
> time you do an unlock (or when you close the file), it will only
> remove/replace the first one.
> 
> Both of those bugs are due to the whole "drop the lock in the middle",
> which is pretty much always a mistake.  BUG#2 could easily explain the
> warning Kirill reports, afaik.
> 
> BUG#3 seems to be independent, and is about somebody replacing an
> existing lock, but the new lock conflicts. Again, the first loop will
> remove the old lock, and then the second loop will see the conflict,
> and return an error (and we may then end up waiting for it for the
> FILE_LOCK_DEFERRED case). Now the original lock is gone. Is that
> really right? That sounds bogus. *Failing* to insert a flock causing
> the old flock to go away?

>From flock(2):

	Converting a lock (shared to exclusive, or vice versa) is  not
	guaranteed  to  be atomic: the existing lock is first removed,
	and then a new lock is established.  Between these two steps, a
	pending  lock  request by  another process may be granted, with
	the result that the conversion either blocks, or fails if
	LOCK_NB was specified.

I also checked Michael Kerrisk's book quickly and see similar language
plus "... the conversion will fail and the process will lose its
original lock".

I don't have a quick way to check BSD, but it looks to me like this is
the way Linux has always behaved.

I agree that it's weird, but I think it's what we're stuck with.

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