Re: DoS with POSIX file locks?

Miklos Szeredi
2006-03-22T12:17:30Z

> > > 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