lkml.org 
[lkml]   [2008]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    Subject[PATCH] locks: fix possible infinite loop in fcntl(F_SETLKW) over nfs
    From
    From: J. Bruce Fields <bfields@citi.umich.edu>

    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:

    7723ec9777d9832849b76475b1a21a2872a40d20 "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>
    ---
    fs/locks.c | 48 ++++++++++++++++++++++++++++--------------------
    1 files changed, 28 insertions(+), 20 deletions(-)

    This is appropriate for 2.6.25 (as well as older stable kernels--the bug
    was introduced in 2.6.22).

    --b.

    diff --git a/fs/locks.c b/fs/locks.c
    index d83fab1..43c0af2 100644
    --- a/fs/locks.c
    +++ b/fs/locks.c
    @@ -1801,17 +1801,21 @@ again:
    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;
    + }
    }

    /*
    @@ -1925,17 +1929,21 @@ again:
    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;
    + }
    }

    /*
    --
    1.5.5.rc1


    \
     
     \ /
      Last update: 2008-04-14 21:07    [W:0.026 / U:0.108 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site