Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rcore-fs-sfs: possible deadlock bugs #18

Open
BurtonQin opened this issue Jun 27, 2020 · 1 comment
Open

rcore-fs-sfs: possible deadlock bugs #18

BurtonQin opened this issue Jun 27, 2020 · 1 comment

Comments

@BurtonQin
Copy link

There are two kinds of possible deadlock bugs in rcore-fs/rcore-fs-sfs/src/lib.rs:

  1. Double-Lock:

    let disk_inode = self.disk_inode.write();
    let old_size = disk_inode.size as usize;
    self._resize(old_size + BLKSIZE)?;
    self._write_at(old_size, entry.as_buf()).unwrap();

    The first lock self.disk_inode.write() is on L394.
    fn _resize is called on L396. The read and write locks of self.disk_inode are heavily used in fn _resize and its callees.
    fn _write_at is called L397, which calls fn _io_at on L352. The second lock is on L324 in fn _io_at.
    fn _write_at(&self, offset: usize, buf: &[u8]) -> vfs::Result<usize> {
    self._io_at(offset, offset + buf.len(), |device, range, offset| {

    let size = self.disk_inode.read().size as usize;

    A simple drop(disk_inode) before L396 may fix the bugs. But we need to be careful about possible atomicity violations.

  2. Locks in conflicting orders:
    self.super_block.write() is called before self.free_map.write() in fn sync.

    fn sync(&self) -> vfs::Result<()> {
    let mut super_block = self.super_block.write();
    if super_block.dirty() {
    self.device
    .write_at(BLKSIZE * BLKN_SUPER, super_block.as_buf())?;
    super_block.sync();
    }
    let mut free_map = self.free_map.write();

    self.super_block.write() is called after self.free_map.write() in fn alloc_block and fn free_block.
    fn alloc_block(&self) -> Option<usize> {
    let mut free_map = self.free_map.write();
    let id = free_map.alloc();
    if let Some(block_id) = id {
    let mut super_block = self.super_block.write();

    fn free_block(&self, block_id: usize) {
    let mut free_map = self.free_map.write();
    assert!(!free_map[block_id]);
    free_map.set(block_id, true);
    self.super_block.write().unused_blocks += 1;

    A possible fix is to move self.free_map.write() before self.super_block.write() in fn sync.

@jiegec
Copy link
Member

jiegec commented Jun 28, 2020

Yes, we didn't carefully handle locks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants