On Fri, 2006-03-31 at 13:27 -0500, Trond Myklebust wrote:
> On Fri, 2006-03-31 at 19:30 +0200, Miklos Szeredi wrote:
> > posix_lock_file() was too cautious, failing operations on OOM, even if
> > they didn't actually require an allocation.
> >
> > This has the disadvantage, that a failing unlock on process exit could
> > lead to a memory leak. There are two possibilites for this:
> >
> > - filesystem implements .lock() and calls back to posix_lock_file().
> > On cleanup of files_struct locks_remove_posix() is called which should
> > remove all locks belonging to files_struct. However if filesystem
> > calls posix_lock_file() which fails, then those locks will never be
> > freed.
> >
> > - if a file is closed while a lock is blocked, then after acquiring
> > fcntl_setlk() will undo the lock. But this unlock itself might fail
> > on OOM, again possibly leaking the lock.
> >
> > The solution is to move the checking of the allocations until after it
> > is sure that they will be needed. This will solve the above problem
> > since unlock will always succeed unless it splits an existing region.
> >
> > Signed-off-by: Miklos Szeredi <miklos@xxxxxxxxxx>
> >
> > Index: linux/fs/locks.c
> > ===================================================================
> > --- linux.orig/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
> > +++ linux/fs/locks.c 2006-03-31 18:55:33.000000000 +0200
> > @@ -835,14 +835,7 @@ int __posix_lock_file(struct inode *inod
> > if (request->fl_flags & FL_ACCESS)
> > goto out;
> >
> > - error = -ENOLCK; /* "no luck" */
> > - if (!(new_fl && new_fl2))
> > - goto out;
> > -
> > /*
> > - * We've allocated the new locks in advance, so there are no
> > - * errors possible (and no blocking operations) from here on.
> > - *
> > * Find the first old lock with the same owner as the new lock.
> > */
> >
> > @@ -939,6 +932,18 @@ int __posix_lock_file(struct inode *inod
> > before = &fl->fl_next;
> > }
> >
> > + /*
> > + * The above code only modifies existing locks in case of
> > + * merging or replacing. If new lock(s) need to be inserted
> > + * all modifications are done bellow this, so it's safe yet to
> > + * bail out.
> > + */
> > + error = -ENOLCK; /* "no luck" */
> > + if (!added && request->fl_type != F_UNLCK && !new_fl)
> > + goto out;
> > + if (right && left == right && !new_fl2)
> > + goto out;
> > +
> > error = 0;
> > if (!added) {
> > if (request->fl_type == F_UNLCK)
>
> NACK.
>
> This changes the behaviour of F_UNLCK. Currently, if the allocation
> fails, the inode locking state remains unchanged. With your change, an
> unlock request may end up unlocking part of the inode, but not the rest.
>
> Furthermore, AFAICS you are now able to Oops if (!added && !new_fl).
Sorry: the Oops turned out to be a mirage.
However you are also changing the behaviour of F_SETLK for the case
where the user is trying to up/downgrade a set of existing READ/WRITE
locks. Again you may end up with a situation where some of the existing
locks get up/downgraded, and yet the lock request fails.
Cheers,
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
