lkml.org 
[lkml]   [2012]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH 1/1] locks: prevent side-effects of locks_release_private before file_lock is initialized
From
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;

\
 
 \ /
  Last update: 2012-06-26 03:21    [W:1.482 / U:0.896 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site