Filipe Brandenburger

When calling fcntl(fd, F_SETLEASE, lck) [with lck=F_WRLCK or F_RDLCK],
the custom signal or owner (if any were previously set using F_SETSIG
or F_SETOWN fcntls) would be reset when F_SETLEASE was called for the
second time on the same file descriptor.

This bug is a regression of 2.6.37 and is described here: https://bugzilla.kernel.org/show_bug.cgi?id=43336

This patch reverts a commit from Oct 2004 (with subject "nfs4 lease:
move the f_delown processing") which originally introduced the
lm_release_private callback.

Signed-off-by: Filipe Brandenburger <filbranden*******>
---
fs/locks.c |   17 ++++++-----------
1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 82c3533..6595882 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -427,18 +427,8 @@ static void lease_break_callback(struct file_lock *fl)
    kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG);
}

-static void lease_release_private_callback(struct file_lock *fl)
-{
-    if (!fl->fl_file)
-        return;
-
-    f_delown(fl->fl_file);
-    fl->fl_file->f_owner.signum = 0;
-}
-
static const struct lock_manager_operations lease_manager_ops = {
    .lm_break = lease_break_callback,
-    .lm_release_private = lease_release_private_callback,
    .lm_change = lease_modify,
};

@@ -1155,8 +1145,13 @@ int lease_modify(struct file_lock **before, int arg)
        return error;
    lease_clear_pending(fl, arg);
    locks_wake_up_blocks(fl);
-    if (arg == F_UNLCK)
+    if (arg == F_UNLCK) {
+        struct file *filp = fl->fl_file;
+
+        f_delown(filp);
+        filp->f_owner.signum = 0;
        locks_delete_lock(before);
+    }
    return 0;
}

--
1.7.7.6

When calling fcntl(F_SETLEASE) for a second time on the same file descriptor,
do_fcntl_add_lease will allocate and initialize a new file_lock to pass to
__vfs_setlease. If that function decides to reuse the existing file_lock, it
will free the newly allocated one to prevent leaking memory.

However, the newly allocate file_lock was initialized with a valid file
descriptor pointer and fl_lmops that contains a lm_release_private function,
so calling locks_free_lock will trigger a call to lease_release_private_callback
which will clear the fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file
descriptor even though the lease is not really being cleared at that point (as
only the temporary file_lock is being freed.)

This patch will fix this bug by calling kmem_cache_free directly instead of
locks_free_lock if the file_lock will not be used. This will end up avoiding
the call to lease_release_private_callback.

This bug was tracked in kernel.org Bugzilla database: https://bugzilla.kernel.org/show_bug.cgi?id=43336

Signed-off-by: Filipe Brandenburger <filbranden*******>
---
fs/locks.c |   22 +++++++++++++++++-----
1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 814c51d..2a751d8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -473,7 +473,10 @@ static struct file_lock *lease_alloc(struct file *filp, int type)

    error = lease_init(filp, type, fl);
    if (error) {
-        locks_free_lock(fl);
+        /* Free the lock by hand instead of calling locks_free_lock
+         * to prevent the call back to lease_release_private_callback
+         */
+        kmem_cache_free(filelock_cache, fl);
        return ERR_PTR(error);
    }
    return fl;
@@ -1538,19 +1541,28 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)

    new = fasync_alloc();
    if (!new) {
-        locks_free_lock(fl);
+        /* Free the lock by hand instead of calling locks_free_lock
+         * to prevent the call back to lease_release_private_callback
+         */
+        kmem_cache_free(filelock_cache, fl);
        return -ENOMEM;
    }
    ret = fl;
    lock_flocks();
    error = __vfs_setlease(filp, arg, &ret);
+    if (error || ret != fl)
+        /*
+         * Free the lock by hand instead of calling locks_free_lock
+         * to prevent the call back to lease_release_private_callback
+         * which will unset F_SETOWN and F_SETSIG for the file
+         * descriptor but that is not wanted as the lease was not
+         * really in use.
+         */
+        kmem_cache_free(filelock_cache, fl);
    if (error) {
        unlock_flocks();
-        locks_free_lock(fl);
        goto out_free_fasync;
    }
-    if (ret != fl)
-        locks_free_lock(fl);

    /*
     * fasync_insert_entry() returns the old entry if any.
--
1.7.7.6

When calling fcntl(F_SETLEASE) for a second time on the same fd, do_fcntl_add_lease
will allocate and initialize a new file_lock, then if __vfs_setlease decides to
reuse the existing file_lock it will free the newly allocated one to prevent leaking
memory.

However, the new file_lock was initialized to the point where it has a valid file
descriptor pointer and lmops, so calling locks_free_lock will trigger a call to
lease_release_private_callback which will have the side effect of clearing the
fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file descriptor even though
that was not supposed to happen at that point.

This patch will fix this by calling kmem_cache_free(filelock_cache, fl) instead of
locks_free_lock(fl) if the file_lock is not completely initialized and actually
associated to the file descriptor, avoiding the call to lease_release_private_callback
with the undesired side effects.

Signed-off-by: Filipe Brandenburger <filbranden*******>
---
fs/locks.c |    8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 814c51d..ce57c59 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -473,7 +473,7 @@ static struct file_lock *lease_alloc(struct file *filp, int type)

    error = lease_init(filp, type, fl);
    if (error) {
-        locks_free_lock(fl);
+        kmem_cache_free(filelock_cache, fl);
        return ERR_PTR(error);
    }
    return fl;
@@ -1538,7 +1538,7 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)

    new = fasync_alloc();
    if (!new) {
-        locks_free_lock(fl);
+        kmem_cache_free(filelock_cache, fl);
        return -ENOMEM;
    }
    ret = fl;
@@ -1546,11 +1546,11 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg)
    error = __vfs_setlease(filp, arg, &ret);
    if (error) {
        unlock_flocks();
-        locks_free_lock(fl);
+        kmem_cache_free(filelock_cache, fl);
        goto out_free_fasync;
    }
    if (ret != fl)
-        locks_free_lock(fl);
+        kmem_cache_free(filelock_cache, fl);

    /*
     * fasync_insert_entry() returns the old entry if any.
--
1.7.7.6

Read more »