Skip to content

Commit

Permalink
locks: fix possible infinite loop in fcntl(F_SETLKW) over nfs
Browse files Browse the repository at this point in the history
Miklos Szeredi found the bug:

	"Basically what happens is that on the server nlm_fopen() calls
	nfsd_open() which returns -EACCES, to which nlm_fopen() returns
	NLM_LCK_DENIED.

	"On the client this will turn into a -EAGAIN (nlm_stat_to_errno()),
	which in will cause fcntl_setlk() to retry forever."

So, for example, opening a file on an nfs filesystem, changing
permissions to forbid further access, then trying to lock the file,
could result in an infinite loop.

And Trond Myklebust identified the culprit, from Marc Eshel and I:

	7723ec9 "locks: factor out
	generic/filesystem switch from setlock code"

That commit claimed to just be reshuffling code, but actually introduced
a behavioral change by calling the lock method repeatedly as long as it
returned -EAGAIN.

We assumed this would be safe, since we assumed a lock of type SETLKW
would only return with either success or an error other than -EAGAIN.
However, nfs does can in fact return -EAGAIN in this situation, and
independently of whether that behavior is correct or not, we don't
actually need this change, and it seems far safer not to depend on such
assumptions about the filesystem's ->lock method.

Therefore, revert the problematic part of the original commit.  This
leaves vfs_lock_file() and its other callers unchanged, while returning
fcntl_setlk and fcntl_setlk64 to their former behavior.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Tested-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>
Cc: Marc Eshel <eshel@almaden.ibm.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
  • Loading branch information
J. Bruce Fields authored and Linus Torvalds committed Apr 14, 2008
1 parent a985aab commit 19e729a
Showing 1 changed file with 28 additions and 20 deletions.
48 changes: 28 additions & 20 deletions fs/locks.c
Original file line number Diff line number Diff line change
Expand Up @@ -1801,17 +1801,21 @@ int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd,
if (error)
goto out;

for (;;) {
error = vfs_lock_file(filp, cmd, file_lock, NULL);
if (error != -EAGAIN || cmd == F_SETLK)
break;
error = wait_event_interruptible(file_lock->fl_wait,
!file_lock->fl_next);
if (!error)
continue;
if (filp->f_op && filp->f_op->lock != NULL)
error = filp->f_op->lock(filp, cmd, file_lock);
else {
for (;;) {
error = posix_lock_file(filp, file_lock, NULL);
if (error != -EAGAIN || cmd == F_SETLK)
break;
error = wait_event_interruptible(file_lock->fl_wait,
!file_lock->fl_next);
if (!error)
continue;

locks_delete_block(file_lock);
break;
locks_delete_block(file_lock);
break;
}
}

/*
Expand Down Expand Up @@ -1925,17 +1929,21 @@ int fcntl_setlk64(unsigned int fd, struct file *filp, unsigned int cmd,
if (error)
goto out;

for (;;) {
error = vfs_lock_file(filp, cmd, file_lock, NULL);
if (error != -EAGAIN || cmd == F_SETLK64)
break;
error = wait_event_interruptible(file_lock->fl_wait,
!file_lock->fl_next);
if (!error)
continue;
if (filp->f_op && filp->f_op->lock != NULL)
error = filp->f_op->lock(filp, cmd, file_lock);
else {
for (;;) {
error = posix_lock_file(filp, file_lock, NULL);
if (error != -EAGAIN || cmd == F_SETLK64)
break;
error = wait_event_interruptible(file_lock->fl_wait,
!file_lock->fl_next);
if (!error)
continue;

locks_delete_block(file_lock);
break;
locks_delete_block(file_lock);
break;
}
}

/*
Expand Down

0 comments on commit 19e729a

Please sign in to comment.