Re: [ 10/41] CIFS: Do not kmalloc under the flocks spinlock

[Pavel Shilovsky]

17 марта 2012 г. 6:37 пользователь Ben Hutchings <ben@xxxxxxxxxxxxxxx> написал:
> 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@xxxxxxxxxxx>
>>
>> commit d5751469f210d2149cc2159ffff66cbeef6da3f2 upstream.
>>
>> Reorganize the code to make the memory already allocated before
>> spinlock'ed loop.
>>
>> Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>> Signed-off-by: Steve French <sfrench@xxxxxxxxxx>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>
>> ---
>>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/