[Fix] extra dput() in loop.c

Alexander Viro (viro@math.psu.edu)
Wed, 21 Oct 1998 14:38:05 -0400 (EDT)


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/