On Tue, Jun 4, 2013 at 6:23 PM, Peng Tao <bergwolf@xxxxxxxxx> wrote:
> On Tue, Jun 4, 2013 at 4:32 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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?
>>
I've run some tests and it turns out that access_ok() doesn't deny
vfs_write() when passed in allocated kernel memory (e.g.,
mdc_wr_kuc()). There is indeed a bug in mdc_wr_kuc() for which I will
send a fix later. However, I cannot test the specific stack memory
write path because of a known issue of Lustre
(https://jira.hpdd.intel.com/browse/LU-2489). And FYI, the Lustre hsm
feature is still under development.
Thanks,
Tao
> 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
