linux-kernel - Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 25 Jun 2012 20:48:49 -0400
From:	Filipe Brandenburger <filbranden@...il.com>
To:	"J. Bruce Fields" <bfields@...ldses.org>
Cc:	Al Viro <viro@...iv.linux.org.uk>, Matthew Wilcox <matthew@....cx>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] locks: prevent side-effects of locks_release_private
 before file_lock is initialized

Hi Bruce,

I was just reviewing this set of patches today... I think if the idea
is not to call fl->fl_lmops->lm_release_private(fl) when the file_lock
struct was not used, then I'd prefer to introduce an exported
__locks_free_lock() function that would do it in order not to expose
the kmem_cache implementation and allow other implementations to do
it.

But I was reading the code and thinking a little more about it and now
I think the correct behavior should be to always call
fl->fl_lmops->lm_release_private(fl) (if the pointers are not NULL)
and have that function behave appropriately if the file_lock struct
was not used.

What made me think of that was a use of fl_ops (it's not directly
fl_lmops, but still I think it would be nice to keep a similar
interface) where nfs4_fl_lock_ops.fl_release_private =
nfs4_fl_release_lock and nfs4_fl_release_lock calls
nfs4_put_lock_state which frees some lists and decrements the usage
counter... Not calling fl->fl_ops->fl_release_private(fl) in that
particular case would be clearly wrong...

So I was thinking of tracking whether the lease was assigned, probably
setting the flag in vfs_setlease(), and then changing
lease_release_private_callback() to check whether the flag was set and
only resetting the F_SETOWN and F_SETSIG information if the flag was
set...

What do you think of that idea?

I just got a quick diff which outlines what I'm thinking of, I haven't
tested it yet, I'll try to build it and run it to see if it passes the
testcase. But please let me know what you think of it.

Cheers,
Fil

------- >8 cut here -------

diff --git a/fs/locks.c b/fs/locks.c
index 814c51d..242ac84 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -429,7 +429,7 @@ static void lease_break_callback(struct file_lock *fl)

 static void lease_release_private_callback(struct file_lock *fl)
 {
-       if (!fl->fl_file)
+       if (!fl->fl_file || !fl->fl_lease_inuse)
                return;

        f_delown(fl->fl_file);
@@ -1513,6 +1513,9 @@ int vfs_setlease(struct file *filp, long arg,
struct file_lock **lease)
        error = __vfs_setlease(filp, arg, lease);
        unlock_flocks();

+       if (!error)
+               lease->fl_lease_inuse = 1;
+
        return error;
 }
 EXPORT_SYMBOL_GPL(vfs_setlease);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 17fd887..2c577a9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1176,6 +1176,7 @@ struct file_lock {
        struct list_head fl_block;      /* circular list of blocked processes */
        fl_owner_t fl_owner;
        unsigned int fl_flags;
+       unsigned char fl_lease_inuse;
        unsigned char fl_type;
        unsigned int fl_pid;
        struct pid *fl_nspid;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/