named-pipes.md

Problem

• Create named pipe in fakefs (turns out this creates a real named pipe!)
• Open it
• This blocks, while holding inodes_lock

Why does it hold the lock?

commit d57b6d26fad3103d2303efe0fa8d74c56a8e637b
Author: Theodore Dubois [email protected]
Date: Sun Nov 24 15:27:51 2019 -0800

Fix race between inode_release and generic_open

https://gist.github.com/tbodt/e92b86d8ddf0b79114703a24d7e99e68

It turns out this commit widened not one but two critical sections.

1. in generic_open

   • fd = mount->...->open()
   • stat = fd->...->fstat()
   • inode = inode_get(stat.inode)

   the inode_lock critical section was inside inode_get, and was expanded to be around all three steps.

   Why?

   • fd = mount->...->open()
     goes in because if inode_release happens between open() and fstat(), fstat will just crash because it tries to lookup an inode number that has been removed from the stats table.
   • stat = fd->...->fstat()
     goes in because if inode_release happens between fstat() and inode_get(), the fd would have (again) an inode number that has been removed from the stats table, which causes a crash not now but the next time something fstats this new fd.

2. in inode_release

   • --inode->refcount
   • list_remove(inode)
   • inode->mount->...->inode_orphaned(inode)

   the inodes_lock critical section was around the first two steps, and was expanded to be around all three.

   Why?

   • inode->mount->...->inode_orphaned(inode)
     goes in because if generic_open happens between list_remove(inode) and inode_orphaned, it could create an open FD for this inode, and then the inode_orphaned step would delete the stat for the inode number from the database, resulting in panics (since fakefs panics when anything tries to look up a nonexistent inode number.)

     open should either return ENOENT or return a valid FD that keeps the underlying file open, not this weird middle.

     well actually, on further thought, if all three steps above in generic_open are a unit, they wouldn't succeed if run between, because that's after the file on disk has been unlinked. but hm, (spoiler alert) we want to take the first open() step out of the unit, and then this logic wouldn't apply.

What to do

What needs to happen is for open() to not happen in inodes_lock. This seems doable as the only thing that could go wrong is fstat failing, and it would work to just convert its crash into an ENOENT.

But the fstat needs to stay inside, it looks like... maybe the critical section in this case could be pushed down the call stack, into fakefs_open itself, but without that refactor it seems best to just leave fstat in the lock.

One more thing

Make named pipes importable