[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

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

[Sean Neakums]

Jamie Lokier <jamie@shareable.org> writes:

> 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.

Looking at 2.6.0-test11, it seems that its struct inode has such a
field, albeit named i_writecount.  Commentary from fs/namei.c:

/*
 * get_write_access() gets write permission for a file.
 * put_write_access() releases this write permission.
 * This is used for regular files.
 * We cannot support write (and maybe mmap read-write shared) accesses and
 * MAP_DENYWRITE mmappings simultaneously. The i_writecount field of an inode
 * can have the following values:
 * 0: no writers, no VM_DENYWRITE mappings
 * < 0: (-i_writecount) vm_area_structs with VM_DENYWRITE set exist
 * > 0: (i_writecount) users are writing to the file.
 *
 * Normally we operate on that counter with atomic_{inc,dec} and it's safe
 * except for the cases where we don't hold i_writecount yet. Then we need to
 * use {get,deny}_write_access() - these functions check the sign and refuse
 * to do the change if sign is wrong. Exclusion between them is provided by
 * spinlock (arbitration_lock) and I'll rip the second arsehole to the first
 * who will try to move it in struct inode - just leave it here.
 */

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

[Jamie Lokier]

Sean Neakums wrote:
> > 	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.
> 
> Looking at 2.6.0-test11, it seems that its struct inode has such a
> field, albeit named i_writecount.  Commentary from fs/namei.c:

Cool.  At a glance, it looks like deny_write_access() can be used to
handle the F_RDLCK case.

(It would also be nice to have some way of blocking until the lease can
be taken, but even the F_WRLCK case doesn't offer that at the moment).

-- Jamie