[PATCH 4/9] staging/lustre: clean up and remove libcfs/linux/linux-fs.c

Peng Tao bergwolf at gmail.com
Tue Jun 4 10:23:40 UTC 2013


On Tue, Jun 4, 2013 at 4:32 PM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> On Mon, Jun 03, 2013 at 09:58:17PM +0800, Peng Tao wrote:
>>  int libcfs_kkuc_msg_put(struct file *filp, void *payload)
>>  {
>>       struct kuc_hdr *kuch = (struct kuc_hdr *)payload;
>> +     ssize_t count = kuch->kuc_msglen;
>> +     loff_t offset = 0;
>> +     mm_segment_t fs;
>>       int rc = -ENOSYS;
>>
>>       if (filp == NULL || IS_ERR(filp))
>> @@ -165,11 +168,18 @@ int libcfs_kkuc_msg_put(struct file *filp, void *payload)
>>               return -ENOSYS;
>>       }
>>
>> -     {
>> -             loff_t offset = 0;
>> -             rc = filp_user_write(filp, payload, kuch->kuc_msglen,
>> -                                  &offset);
>> +     fs = get_fs();
>> +     set_fs(KERNEL_DS);
>> +     while ((ssize_t)count > 0) {
>> +             rc = vfs_write(filp, (const void __user *)payload,
>> +                            count, &offset);
>> +             if (rc < 0)
>> +                     break;
>> +             count -= rc;
>> +             payload += rc;
>> +             rc = 0;
>>       }
>> +     set_fs(fs);
>>
>>       if (rc < 0)
>>               CWARN("message send failed (%d)\n", rc);
>
> This was all there in the original code, it has just been shifted.
> Still, I had some questions about it.  "payload" is a pointer to
> kernel stack memory, wouldn't the access_ok() check in vfs_write()
> fail every time on x86?
>
Thanks for reviewing. I am not familiar with access_ok() but I think
you are right and I'll test to see if it really breaks. In the
meantime, I took a look at other kernel callers of vfs_write() and
btrfs is also calling vfs_write() to write kernel stack memory
(fs/btrfs/send.c: send_header->write_buf->vfs_write). So I CC'ed btrfs
list in case someone knows better than I do.

> Some of the casting is not needed.  No need to cast "count" because
> it is already ssize_t.  I haven't tested but I think Sparse will
> complain about the __user cast until __force is added.  No need for
> the cast to const.
>
Thanks. I will fix it.

> I worry about partial writes and that this could be a forever loop
> but I don't know enough about anything to say if that's possible.
> Probably not.
>
For partial writes, both payload and count are advanced properly. So
it won't loop forever.

Thanks,
Tao



More information about the devel mailing list