Re: DoS with POSIX file locks?

i concur with Trond, there's no sane way to get rid of it w/out
formalizing CLONE_FILES and locks on exec

Probably there is. It would involve allocating a separate
lock-owner-ID stored in files_struct but separate from it. But it's
more complicated than simply not propagating locks on exec in the
CLONE_FILES case.

That doesn't solve the fundamental problem.

You would still have to be able to tell a remote server that some locks
which previously belonged to one owner are being reallocated to several
owners.

No changing of lock owner is involved, that's the whole point.

Instead of trying to explain, here's a untested/unfinished patch
showing the idea.

Miklos

Index: linux/fs/locks.c
===================================================================
--- linux.orig/fs/locks.c 2006-03-22 11:20:15.000000000 +0100
+++ linux/fs/locks.c 2006-03-22 13:03:47.000000000 +0100
@@ -333,7 +333,7 @@ static int flock_to_posix_lock(struct fi
if (fl->fl_end < fl->fl_start)
return -EOVERFLOW;

- fl->fl_owner = current->files;
+ fl->fl_owner = current->files->lock_owner;
fl->fl_pid = current->tgid;
fl->fl_file = filp;
fl->fl_flags = FL_POSIX;
@@ -379,7 +379,7 @@ static int flock64_to_posix_lock(struct
if (fl->fl_end < fl->fl_start)
return -EOVERFLOW;

- fl->fl_owner = current->files;
+ fl->fl_owner = current->files->lock_owner;
fl->fl_pid = current->tgid;
fl->fl_file = filp;
fl->fl_flags = FL_POSIX;
@@ -432,7 +432,7 @@ static struct lock_manager_operations le
*/
static int lease_init(struct file *filp, int type, struct file_lock *fl)
{
- fl->fl_owner = current->files;
+ fl->fl_owner = current->files->lock_owner;
fl->fl_pid = current->tgid;

fl->fl_file = filp;
@@ -1003,7 +1003,7 @@ EXPORT_SYMBOL(posix_lock_file_wait);
*/
int locks_mandatory_locked(struct inode *inode)
{
- fl_owner_t owner = current->files;
+ fl_owner_t owner = current->files->lock_owner;
struct file_lock *fl;

/*
@@ -1041,7 +1041,7 @@ int locks_mandatory_area(int read_write,
int error;

locks_init_lock(&fl);
- fl.fl_owner = current->files;
+ fl.fl_owner = current->files->lock_owner;
fl.fl_pid = current->tgid;
fl.fl_file = filp;
fl.fl_flags = FL_POSIX | FL_ACCESS;
@@ -1141,7 +1141,7 @@ int __break_lease(struct inode *inode, u
goto out;

for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next)
- if (fl->fl_owner == current->files)
+ if (fl->fl_owner == current->files->lock_owner)
i_have_this_lease = 1;

if (mode & FMODE_WRITE) {
@@ -2183,55 +2183,23 @@ int lock_may_write(struct inode *inode,

EXPORT_SYMBOL(lock_may_write);

-static inline void __steal_locks(struct file *file, fl_owner_t from)
-{
- struct inode *inode = file->f_dentry->d_inode;
- struct file_lock *fl = inode->i_flock;
-
- while (fl) {
- if (fl->fl_file == file && fl->fl_owner == from)
- fl->fl_owner = current->files;
- fl = fl->fl_next;
- }
-}
-
/* When getting ready for executing a binary, we make sure that current
* has a files_struct on its own. Before dropping the old files_struct,
* we take over ownership of all locks for all file descriptors we own.
* Note that we may accidentally steal a lock for a file that a sibling
* has created since the unshare_files() call.
*/
-void steal_locks(fl_owner_t from)
+void steal_locks(struct files_struct *from)
{
struct files_struct *files = current->files;
- int i, j;
- struct fdtable *fdt;
+ struct lock_owner *tmp;

if (from == files)
return;

- lock_kernel();
- j = 0;
- rcu_read_lock();
- fdt = files_fdtable(files);
- for (;;) {
- unsigned long set;
- i = j * __NFDBITS;
- if (i >= fdt->max_fdset || i >= fdt->max_fds)
- break;
- set = fdt->open_fds->fds_bits[j++];
- while (set) {
- if (set & 1) {
- struct file *file = fdt->fd[i];
- if (file)
- __steal_locks(file, from);
- }
- i++;
- set >>= 1;
- }
- }
- rcu_read_unlock();
- unlock_kernel();
+ tmp = from->lock_owner;
+ from->lock_owner = files->lock_owner;
+ files->lock_owner = tmp;
}
EXPORT_SYMBOL(steal_locks);

Index: linux/include/linux/fs.h
===================================================================
--- linux.orig/include/linux/fs.h 2006-03-22 11:20:15.000000000 +0100
+++ linux/include/linux/fs.h 2006-03-22 12:59:09.000000000 +0100
@@ -678,7 +678,7 @@ extern spinlock_t files_lock;
*
* Lockd stuffs a "host" pointer into this.
*/
-typedef struct files_struct *fl_owner_t;
+typedef struct lock_owner *fl_owner_t;

struct file_lock_operations {
void (*fl_insert)(struct file_lock *); /* lock insertion callback */
@@ -767,7 +767,7 @@ extern int setlease(struct file *, long,
extern int lease_modify(struct file_lock **, int);
extern int lock_may_read(struct inode *, loff_t start, unsigned long count);
extern int lock_may_write(struct inode *, loff_t start, unsigned long count);
-extern void steal_locks(fl_owner_t from);
+extern void steal_locks(struct files_struct *from);

struct fasync_struct {
int magic;
Index: linux/fs/fcntl.c
===================================================================
--- linux.orig/fs/fcntl.c 2006-03-20 06:53:29.000000000 +0100
+++ linux/fs/fcntl.c 2006-03-22 13:02:36.000000000 +0100
@@ -177,7 +177,7 @@ asmlinkage long sys_dup2(unsigned int ol
spin_unlock(&files->file_lock);

if (tofree)
- filp_close(tofree, files);
+ filp_close(tofree, files->lock_owner);
err = newfd;
out:
return err;
Index: linux/fs/open.c
===================================================================
--- linux.orig/fs/open.c 2006-03-20 06:53:29.000000000 +0100
+++ linux/fs/open.c 2006-03-22 13:01:39.000000000 +0100
@@ -1159,7 +1159,7 @@ asmlinkage long sys_close(unsigned int f
FD_CLR(fd, fdt->close_on_exec);
__put_unused_fd(files, fd);
spin_unlock(&files->file_lock);
- return filp_close(filp, files);
+ return filp_close(filp, files->lock_owner);

out_unlock:
spin_unlock(&files->file_lock);
Index: linux/include/linux/file.h
===================================================================
--- linux.orig/include/linux/file.h 2006-03-22 11:20:15.000000000 +0100
+++ linux/include/linux/file.h 2006-03-22 12:28:21.000000000 +0100
@@ -29,6 +29,11 @@ struct fdtable {
struct fdtable *next;
};

+struct lock_owner {
+ /* lock accounting will go in here */
+ int dummy;
+};
+
/*
* Open file table structure
*/
@@ -40,6 +45,7 @@ struct files_struct {
fd_set open_fds_init;
struct file * fd_array[NR_OPEN_DEFAULT];
spinlock_t file_lock; /* Protects concurrent writers. Nests inside tsk->alloc_lock */
+ struct lock_owner *lock_owner;
};

#define files_fdtable(files) (rcu_dereference((files)->fdt))
Index: linux/kernel/fork.c
===================================================================
--- linux.orig/kernel/fork.c 2006-03-22 11:20:15.000000000 +0100
+++ linux/kernel/fork.c 2006-03-22 12:48:51.000000000 +0100
@@ -605,6 +605,7 @@ static struct files_struct *alloc_files(
goto out;

atomic_set(&newf->count, 1);
+ newf->lock_owner = kmalloc(sizeof(struct lock_owner), SLAB_KERNEL);

spin_lock_init(&newf->file_lock);
fdt = &newf->fdtab;
Index: linux/kernel/exit.c
===================================================================
--- linux.orig/kernel/exit.c 2006-03-22 13:12:20.000000000 +0100
+++ linux/kernel/exit.c 2006-03-22 13:12:50.000000000 +0100
@@ -395,7 +395,7 @@ static void close_files(struct files_str
if (set & 1) {
struct file * file = xchg(&fdt->fd[i], NULL);
if (file)
- filp_close(file, files);
+ filp_close(file, files->lock_owner);
}
i++;
set >>= 1;
@@ -422,6 +422,7 @@ void fastcall put_files_struct(struct fi

if (atomic_dec_and_test(&files->count)) {
close_files(files);
+ kfree(files->lock_owner);
/*
* Free the fd and fdset arrays if we expanded them.
* If the fdtable was embedded, pass files for freeing






-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages