Messages in this thread Patches in this message | | | Date | Wed, 21 Oct 1998 14:38:05 -0400 (EDT) | From | Alexander Viro <> | Subject | [Fix] extra dput() in loop.c |
| |
loop.c::loop_set_fd() assumes that get_write_access can't fail. It can - just feed it a binary that is executing right now. Result: extra dput().
Minimal patch: --- drivers/block/loop.c.orig Wed Oct 21 03:37:48 1998 +++ drivers/block/loop.c Wed Oct 21 03:42:27 1998 @@ -385,8 +385,10 @@ lo->lo_backing_file->private_data = file->private_data; error = get_write_access(inode); /* cannot fail */ + /* yes, it can */ if (error) { - fput(lo->lo_backing_file); + /* We didn't increase d_count yet */ + put_filp(lo->lo_backing_file); lo->lo_backing_file = NULL; } } Alternative variant: since loop_set_fd is the only modular user of get_empty_filp() and all it actually needs is to clone the existing struct file (for regular file)... IMHO it deserves adding such primitive to fs/file_table.c, including it into exported symbols and dropping get_empty_filp from there. Following patch only extracts that into separate function (clone_filp) and leaves it in drivers/block/loop.c. If you think that it's worth changing the public interface - please, do it. IMHO it would be much cleaner.
--- drivers/block/loop.c.orig Wed Oct 21 03:37:48 1998 +++ drivers/block/loop.c Wed Oct 21 03:56:40 1998 @@ -328,6 +328,24 @@ } } +struct file *clone_filp(struct file *file) { + struct file *res; + + if (!S_ISREG(file->f_dentry->d_inode->i_mode)) + return NULL; + res = get_empty_filp(); + if (res) { + res->f_mode = file->f_mode; + res->f_pos = file->f_pos; + res->f_flags = file->f_flags; + res->f_owner = file->f_owner; + res->f_dentry = dget(file->f_dentry); + res->f_op = file->f_op; + res->private_data = file->private_data; + } + return res; +} + static int loop_set_fd(struct loop_device *lo, kdev_t dev, unsigned int arg) { struct file *file; @@ -360,6 +378,7 @@ /* Backed by a block device - don't need to hold onto a file structure */ lo->lo_backing_file = NULL; + dget(lo->lo_dentry); } else if (S_ISREG(inode->i_mode)) { /* Backed by a regular file - we need to hold onto @@ -374,21 +393,12 @@ lo->lo_flags = LO_FLAGS_DO_BMAP; error = -ENFILE; - lo->lo_backing_file = get_empty_filp(); - if (lo->lo_backing_file) { - lo->lo_backing_file->f_mode = file->f_mode; - lo->lo_backing_file->f_pos = file->f_pos; - lo->lo_backing_file->f_flags = file->f_flags; - lo->lo_backing_file->f_owner = file->f_owner; - lo->lo_backing_file->f_dentry = file->f_dentry; - lo->lo_backing_file->f_op = file->f_op; - lo->lo_backing_file->private_data = file->private_data; - - error = get_write_access(inode); /* cannot fail */ - if (error) { - fput(lo->lo_backing_file); - lo->lo_backing_file = NULL; - } + if (!(lo->lo_backing_file = clone_filp(file)) + goto out_putf; + error = get_write_access(inode); + if (error) { + fput(lo->lo_backing_file); + lo->lo_backing_file = NULL; } } if (error) HTH. HAND, Al
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
| |