[PATCH] fix posix lock on NFS, #2

[PATCH] fix posix lock on NFS, #2

[ASANO Masahiro]

Hi,

This is another problem report on posix lock.

The nfsd may handle the lock information incorrectly if new lock is
contiguous to old one.  I think both 2.6 and 2.4 kernel have this bug.

The following is a sample program to produce the circumstance.
        fd = open("file_on_nfs_2", O_RDWR | O_CREAT, 0644);
        memset(&lck, 0, sizeof(lck));
        lck.l_type = F_WRLCK;
        switch (child = fork()) {
        case 0:
                lck.l_start = 0; lck.l_len = 1; fcntl(fd, F_SETLK, &lck);
                sleep(1);
                lck.l_start = 1; lck.l_len = 1; fcntl(fd, F_SETLKW, &lck);
                _exit(0);
        default:
                lck.l_start = 1; lck.l_len = 1; fcntl(fd, F_SETLK, &lck);
                sleep(2); close(fd);
                printf("waiting...\n");
                // sleep(1); kill(chiled, SIGINT);
                wait(NULL);
                break;
        }

All version of linux requires time to grant the lock, but on older
kernel the lock is leaked if it is signaled while waiting the grant.

I think it is caused by a miss-calling of posix_lock_file() in nfsd.
posix_lock_file() updates the second argument `request' as you know,
but it seems that nfsd gives no care about that.

Here is a patch.  It changes nfsd to keep the range of file_lock for
later use.  Any comments and feedback are welcome.

Signed-off-by: ASANO Masahiro <masano@tnes.nec.co.jp>

--- linux-2.6.15-rc6/fs/lockd/svclock.c.orig	2005-12-23 20:16:33.000000000 +0900
+++ linux-2.6.15-rc6/fs/lockd/svclock.c	2005-12-23 20:24:13.000000000 +0900
@@ -510,6 +510,7 @@ nlmsvc_grant_blocked(struct nlm_block *b
 	struct nlm_file		*file = block->b_file;
 	struct nlm_lock		*lock = &block->b_call.a_args.lock;
 	struct file_lock	*conflock;
+	struct file_lock	tmplck;
 	int			error;
 
 	dprintk("lockd: grant blocked lock %p\n", block);
@@ -542,7 +543,8 @@ nlmsvc_grant_blocked(struct nlm_block *b
 	 * following yields an error, this is most probably due to low
 	 * memory. Retry the lock in a few seconds.
 	 */
-	if ((error = posix_lock_file(file->f_file, &lock->fl)) < 0) {
+	tmplck = lock->fl;	/* keep the range for later use */
+	if ((error = posix_lock_file(file->f_file, &tmplck)) < 0) {
 		printk(KERN_WARNING "lockd: unexpected error %d in %s!\n",
 				-error, __FUNCTION__);
 		nlmsvc_insert_block(block, 10 * HZ);

Re: [PATCH] fix posix lock on NFS, #2

[Trond Myklebust]

On Fri, 2005-12-23 at 23:38 +0900, ASANO Masahiro wrote:
> Here is a patch.  It changes nfsd to keep the range of file_lock for
> later use.  Any comments and feedback are welcome.
> 
> Signed-off-by: ASANO Masahiro <masano@tnes.nec.co.jp>
> 
> --- linux-2.6.15-rc6/fs/lockd/svclock.c.orig	2005-12-23 20:16:33.000000000 +0900
> +++ linux-2.6.15-rc6/fs/lockd/svclock.c	2005-12-23 20:24:13.000000000 +0900
> @@ -510,6 +510,7 @@ nlmsvc_grant_blocked(struct nlm_block *b
>  	struct nlm_file		*file = block->b_file;
>  	struct nlm_lock		*lock = &block->b_call.a_args.lock;
>  	struct file_lock	*conflock;
> +	struct file_lock	tmplck;
>  	int			error;
>  
>  	dprintk("lockd: grant blocked lock %p\n", block);
> @@ -542,7 +543,8 @@ nlmsvc_grant_blocked(struct nlm_block *b
>  	 * following yields an error, this is most probably due to low
>  	 * memory. Retry the lock in a few seconds.
>  	 */
> -	if ((error = posix_lock_file(file->f_file, &lock->fl)) < 0) {
> +	tmplck = lock->fl;	/* keep the range for later use */
> +	if ((error = posix_lock_file(file->f_file, &tmplck)) < 0) {
>  		printk(KERN_WARNING "lockd: unexpected error %d in %s!\n",
>  				-error, __FUNCTION__);
>  		nlmsvc_insert_block(block, 10 * HZ);

NACK. You cannot copy locks like this. See locks_copy_lock().

Cheers,
  Trond

Re: [PATCH] fix posix lock on NFS, #2

[ASANO Masahiro]

From: Trond Myklebust <trond.myklebust@fys.uio.no>
Subject: Re: [PATCH] fix posix lock on NFS, #2
Date: Fri, 23 Dec 2005 16:02:31 +0100

> On Fri, 2005-12-23 at 23:38 +0900, ASANO Masahiro wrote:
> > Here is a patch.  It changes nfsd to keep the range of file_lock for
> > later use.  Any comments and feedback are welcome.

> NACK. You cannot copy locks like this. See locks_copy_lock().

Thank you for pointing it out.  I forgot all that point.  I'm trying
to update my patch, but things are complicated.
Do you think it is safe to copy the file_lock table simply with
locks_copy_lock() at that point, or it may break nlm_blocked list in
nfsd?
	
--
masano