[PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease

[PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease

[Joseph D. Wagner]

Gee, that seemed simple enough.

Keep in mind that I'm new to this whole 'kernel development' thing, and I offer no assurance that my patch actually works.  I only know that it compiles without errors or warnings.

But I THINK this is how a patch would fix the problem, in theory.

This is where the 'theory of a thousand eyes' is going to have to help me out.

TIA guys.

BTW, this patch should be applied to both the 2.4 and 2.6 series of kernels (if it works).

Joseph D. Wagner

--- /old/linux-2.4.22/fs/locks.c	2003-08-25 17:44:43.000000000 +0600
+++ /new/linux-2.4.22/fs/locks.c	2003-11-27 10:08:41.000000000 +0600
@@ -111,10 +111,16 @@
  *  Matthew Wilcox <willy@thepuffingroup.com>, March, 2000.
  *
  *  Leases and LOCK_MAND
  *  Matthew Wilcox <willy@linuxcare.com>, June, 2000.
  *  Stephen Rothwell <sfr@canb.auug.org.au>, June, 2000.
+ *
+ *  FIXED Leases Issue -- Function fcntl_setlease would only check if a
+ *  file had been opened before granting a F_WRLCK, a.k.a. a write lease.
+ *  It would not check if the file had be opened for writing before
+ *  granting a F_RDLCK, a.k.a. a read lease.  This issue is now resolved.
+ *  Joseph D. Wagner <wagnerjd@users.sourceforge.net> November 2003
  */
 
 #include <linux/slab.h>
 #include <linux/file.h>
 #include <linux/smp_lock.h>
@@ -1287,18 +1293,25 @@
 
 	lock_kernel();
 
 	time_out_leases(inode);
 
-	/*
-	 * FIXME: What about F_RDLCK and files open for writing?
-	 */
 	error = -EAGAIN;
 	if ((arg == F_WRLCK)
 	    && ((atomic_read(&dentry->d_count) > 1)
 		|| (atomic_read(&inode->i_count) > 1)))
 		goto out_unlock;
+	if ((arg == F_RDLCK)
+	    && ((dentry->d_flags & O_WRONLY)
+		|| (dentry->d_flags & O_RDWR)
+		|| (dentry->d_flags & O_CREAT)
+		|| (dentry->d_flags & O_TRUNC)
+		|| (inode->i_flags & O_WRONLY)
+		|| (inode->i_flags & O_RDWR)
+		|| (inode->i_flags & O_CREAT)
+		|| (inode->i_flags & O_TRUNC)))
+		goto out_unlock;
 
 	/*
 	 * At this point, we know that if there is an exclusive
 	 * lease on this file, then we hold it on this filp
 	 * (otherwise our open of this file would have blocked).

Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease

[Jamie Lokier]

Joseph D. Wagner wrote:
> But I THINK this is how a patch would fix the problem, in theory.

Sorry, it won't.

> +	if ((arg == F_RDLCK)
> +	    && ((dentry->d_flags & O_WRONLY)
> +		|| (dentry->d_flags & O_RDWR)
> +		|| (dentry->d_flags & O_CREAT)
> +		|| (dentry->d_flags & O_TRUNC)
> +		|| (inode->i_flags & O_WRONLY)
> +		|| (inode->i_flags & O_RDWR)
> +		|| (inode->i_flags & O_CREAT)
> +		|| (inode->i_flags & O_TRUNC)))
> +		goto out_unlock;

dentry->d_flags is a combination of the S_ flags, not O_ flags.
E.g. S_SYNC, S_NOATIME etc.

inode->i_flags is a combination of the DCACHE_ flags.
E.g. DCACHE_AUTOFS_PENDING, DCACHE_REFERENCED tc.

To detect if anyone has the file open for writing, you'll a new count
field which keeps track of writer references.  Something like this:

	if ((arg == F_RDLCK)
	    && ((atomic_read(&inode->i_writer_count) != 0)))

You'll also need to modify all the places where that needs to be
maintained.

Btw, I'm not sure why the F_WRLCK case needs to check d_count and
i_count.  Isn't it enough to check d_count?  Won't all opens reference
the inode through a dentry?:

>  	if ((arg == F_WRLCK)
>  	    && ((atomic_read(&dentry->d_count) > 1)
>  		|| (atomic_read(&inode->i_count) > 1)))

-- Jamie

Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease

[Nikita Danilov]

Jamie Lokier writes:
 > Joseph D. Wagner wrote:
 > > But I THINK this is how a patch would fix the problem, in theory.
 > 
 > Sorry, it won't.
 > 
 > > +	if ((arg == F_RDLCK)
 > > +	    && ((dentry->d_flags & O_WRONLY)
 > > +		|| (dentry->d_flags & O_RDWR)
 > > +		|| (dentry->d_flags & O_CREAT)
 > > +		|| (dentry->d_flags & O_TRUNC)
 > > +		|| (inode->i_flags & O_WRONLY)
 > > +		|| (inode->i_flags & O_RDWR)
 > > +		|| (inode->i_flags & O_CREAT)
 > > +		|| (inode->i_flags & O_TRUNC)))
 > > +		goto out_unlock;
 > 
 > dentry->d_flags is a combination of the S_ flags, not O_ flags.
 > E.g. S_SYNC, S_NOATIME etc.
 > 
 > inode->i_flags is a combination of the DCACHE_ flags.
 > E.g. DCACHE_AUTOFS_PENDING, DCACHE_REFERENCED tc.

I think it is other way around. ->i_flags are combined from S_SYNC (see,
include/linux/fs.h:IS_IMMUTABLE(), for example), and ->d_flags are
combined from DCACHE_*, latter is explicitly stated in
include/linux/dcache.h

 > 
 > To detect if anyone has the file open for writing, you'll a new count
 > field which keeps track of writer references.  Something like this:
 > 
 > 	if ((arg == F_RDLCK)
 > 	    && ((atomic_read(&inode->i_writer_count) != 0)))
 > 
 > You'll also need to modify all the places where that needs to be
 > maintained.
 > 
 > Btw, I'm not sure why the F_WRLCK case needs to check d_count and
 > i_count.  Isn't it enough to check d_count?  Won't all opens reference
 > the inode through a dentry?:
 > 
 > >  	if ((arg == F_WRLCK)
 > >  	    && ((atomic_read(&dentry->d_count) > 1)
 > >  		|| (atomic_read(&inode->i_count) > 1)))
 > 
 > -- Jamie

Nikita.

Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease

[Jamie Lokier]

Nikita Danilov wrote:
>  > dentry->d_flags is a combination of the S_ flags, not O_ flags.
>  > E.g. S_SYNC, S_NOATIME etc.
>  > 
>  > inode->i_flags is a combination of the DCACHE_ flags.
>  > E.g. DCACHE_AUTOFS_PENDING, DCACHE_REFERENCED tc.
> 
> I think it is other way around. ->i_flags are combined from S_SYNC (see,
> include/linux/fs.h:IS_IMMUTABLE(), for example), and ->d_flags are
> combined from DCACHE_*, latter is explicitly stated in
> include/linux/dcache.h

Oh, yes of course :)

-- Jamie

Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease

[Joseph D. Wagner]

>> But I THINK this is how a patch would fix the problem, in theory.

> Sorry, it won't.
...
> To detect if anyone has the file open for writing, you'll a new count
> field which keeps track of writer references.  Something like this:
>
>        if ((arg == F_RDLCK)
>            && ((atomic_read(&inode->i_writer_count) != 0)))
>
> You'll also need to modify all the places where that needs to be
> maintained.

Well, dang it all.  Why didn't they guy who implemented leasing in the first 
place bother to do it right the first time?

I don't have the time or technical expertise in kernel development to go 
through all that.  Somebody else is going to have to pick up his slack.

Joseph D. Wagner

Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease

[Jamie Lokier]

Joseph D. Wagner wrote:
> Well, dang it all.  Why didn't they guy who implemented leasing in the first 
> place bother to do it right the first time?
>
> I don't have the time or technical expertise in kernel development to go 
> through all that.  Somebody else is going to have to pick up his slack.

This isn't a commercial project with teams picking up "slack".  It's
mostly volunteers, using their private time as they see fit.  Progress
is made in pieces, nobody is obliged to "finish" anything, and people
contribute incomplete features so that someone else with time, energy
and inclination might complete them.

Being rude is inappropriate.  Words like "didn't bother" and "slack"
are offensive in this context, much like calling someone who works 100
hours a week "a lazy programmer" because they'd need 200 hours to do
what you think they should have done is offensive.

-- Jamie