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: <20150216190254.3b66a9ba@tlielax.poochiereds.net>
Date:	Mon, 16 Feb 2015 19:02:54 -0500
From:	Jeff Layton <jlayton@...chiereds.net>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	"Kirill A. Shutemov" <kirill@...temov.name>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"J. Bruce Fields" <bfields@...ldses.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, 16 Feb 2015 11:24:03 -0800
Linus Torvalds <torvalds@...ux-foundation.org> 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?
> 
> Now, flock semantics are pretty much insane, so maybe all these bugs
> except for #2 aren't actually bugs, and are "features" of flock. But
> bug #2 can't be a semantic feature.
> 
> Is there something I'm missing here?
> 
> This was all just looking at a *single* function. Quite frankly, I
> hate how the code also just does
> 
>         if (filp->f_op->flock)
>                 filp->f_op->flock(filp, F_SETLKW, &fl);
>         else
>                 flock_lock_file(filp, &fl);
> 
> and blithely assumes that some random filesystem will get the flock
> semantics right, when even the code code screwed it up this badly.
> 
> And maybe I'm wrong, and there's some reason why none of the things
> above can actually happen, but it looks really bad to me.
> 
>                          Linus

Now that I look, it may be best to just revert this whole set for now.
Linus, are you amenable to doing that?

While I think this could be a nice cleanup, it obviously needs more
testing and scrutiny, and it won't hurt to wait another release or two
to make sure I get this right. There may be some merge conflicts with
Bruce's tree, but I'd rather deal with those than break the file
locking code.

Once we do that, then we could introduce some smaller patches to fix
up the bug you spotted, but at least we'll be proceeding from a spot
that is known to work.

I'll start preparing a pull request that does that...

Thanks,
-- 
Jeff Layton <jlayton@...chiereds.net>
--
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/