lkml.org 
[lkml]   [2012]   [Mar]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [ 10/41] CIFS: Do not kmalloc under the flocks spinlock
    From
    17 марта 2012 г. 6:37 пользователь Ben Hutchings <ben@decadent.org.uk> написал:
    > On Fri, Mar 16, 2012 at 04:38:20PM -0700, Greg KH wrote:
    >> 3.2-stable review patch.  If anyone has any objections, please let me know.
    >>
    >> ------------------
    >>
    >> From: Pavel Shilovsky <piastry@etersoft.ru>
    >>
    >> commit d5751469f210d2149cc2159ffff66cbeef6da3f2 upstream.
    >>
    >> Reorganize the code to make the memory already allocated before
    >> spinlock'ed loop.
    >>
    >> Reviewed-by: Jeff Layton <jlayton@redhat.com>
    >> Signed-off-by: Pavel Shilovsky <piastry@etersoft.ru>
    >> Signed-off-by: Steve French <sfrench@us.ibm.com>
    >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    >>
    >> ---
    >>  fs/cifs/file.c |   69 ++++++++++++++++++++++++++++++++++++++++++++++-----------
    >>  1 file changed, 56 insertions(+), 13 deletions(-)
    >>
    >> --- a/fs/cifs/file.c
    >> +++ b/fs/cifs/file.c
    > [....]
    >> @@ -940,29 +950,55 @@ cifs_push_posix_locks(struct cifsFileInf
    >>               return rc;
    >>       }
    >>
    >> +     lock_flocks();
    >> +     cifs_for_each_lock(cfile->dentry->d_inode, before) {
    >> +             if ((*before)->fl_flags & FL_POSIX)
    >> +                     count++;
    >> +     }
    >> +     unlock_flocks();
    >> +
    >>       INIT_LIST_HEAD(&locks_to_send);
    >>
    >> +     /*
    >> +      * Allocating count locks is enough because no locks can be added to
    >> +      * the list while we are holding cinode->lock_mutex that protects
    >> +      * locking operations of this inode.
    >> +      */
    >> +     for (; i < count; i++) {
    >> +             lck = kmalloc(sizeof(struct lock_to_push), GFP_KERNEL);
    >> +             if (!lck) {
    >> +                     rc = -ENOMEM;
    >> +                     goto err_out;
    >> +             }
    >> +             list_add_tail(&lck->llist, &locks_to_send);
    >> +     }
    >> +
    >> +     i = 0;
    >> +     el = locks_to_send.next;
    >>       lock_flocks();
    >>       cifs_for_each_lock(cfile->dentry->d_inode, before) {
    >> +             if (el == &locks_to_send) {
    >> +                     /* something is really wrong */
    >> +                     cERROR(1, "Can't push all brlocks!");
    >> +                     break;
    >> +             }
    >>               flock = *before;
    >> +             if ((flock->fl_flags & FL_POSIX) == 0)
    >> +                     continue;
    > [...]
    >
    > If I understand the logic correctly, el == &locks_to_send means we
    > already used all the lock_to_push structures.  (It should also be
    > equivalent to testing i == count.  Why is i incremented but not
    > otherwise used in the loop?)
    >
    > But we test this before flock->fl_flags & FL_POSIX, which means we
    > don't know whether this lock actually needs to be assigned one of
    > those structures.  So it appears that we might report a spurious error
    > if the lock list ends with a mandatory lock.  If so, this is
    > relatively harmless but does need to be fixed.
    >

    You are right here, thanks for the catch! I will repost the patch asap.

    --
    Best regards,
    Pavel Shilovsky.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-03-17 07:17    [W:2.890 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site