>
> About suggestion, well, no. Not only this would be not simpler, it
> would be just wrong.
> All the checks and all the use must be done after fget before fput, otherwise
> user space can close the descriptor and even open something
> else in another thread.
>
Yeah, I forgot about this concern when I suggested to do fdget/fdput in each
loop,.
But how about the idea that we do fdget on every file in the first loop, do
all necessary
checks, and then store returned fd if all checks pass or jump to out path in
case of
any failures. Then in the second loop, we can just use stored fds from the
first loop
and do data copy. In the out path, we can just do fdput on all stored fds.
However I’m not sure if there is any risk of holding multiple fds at the same
time, or holding
one fd longer than necessary.
> On Thu, Jan 18, 2024 at 8:26 PM Kui Liu <kui....@acronis.com> wrote:
>>
>>
>>
>>> On 18 Jan 2024, at 5:08 PM, Alexey Kuznetsov <kuz...@virtuozzo.com> wrote:
>>>
>>>> Is it difficult to separate the verification of inputs, such as fds, size
>>>> of src buffs against dest buffs etc, from the process of copying data?
>>>> Similar to what’s been done in ‘copy_out_args()’.
>>>
>>> I am not sure I understand what you mean here. Show, plz.
>>>
>>> Do you really think copy_out_args is even "readable"? :-)
>>> From my pov it is awful. Actually, I hold 4d patch in series
>>> which replaces copy_out_args with normal human intellibible
>>> function which copies data 10% faster. And I do not think
>>> it was by accident, ugly code is usually a bad code.
>>
>> OK, what I meant was can we break the big for loop in two smaller ones. In
>> the first one, just verify whether inputs are valid, something like
>>
>> + for (i = 0; i < nsplices; i++) {
>> + void *src, *dst;
>> + struct fd f = fdget(fdarr[i]);
>> +
>> + if (f.file) {
>> + unsigned int head, tail, mask;
>> +
>> + if (unlikely(f.file->f_op != fuse_g_splice_ops)) {
>> + if (fuse_g_splice_ops == NULL) {
>> + struct file *probe_files[2];
>> +
>> + if (create_pipe_files(probe_files,
>> 0)) {
>> + err = -EBADF;
>> + goto out;
>> + }
>> + fuse_g_splice_ops =
>> probe_files[0]->f_op;
>> + fput(probe_files[0]);
>> + fput(probe_files[1]);
>> + }
>> + if (unlikely(f.file->f_op !=
>> fuse_g_splice_ops)) {
>> + err = -EBADF;
>> + goto out;
>> + }
>> + }
>> +
>> + pipe = f.file->private_data;
>> +
>> + if (pipe == NULL) {
>> + err = -EBADF;
>> + goto out;
>> + }
>> + } else {
>> + err = -EBADF;
>> + goto out;
>> + }
>>
>> /* count total size of inputs */
>>
>> fdput(f);
>> + }
>> /* check whether size of inputs exceeds available buff in req->ap
>> here */
>>
>>
>> after all checks pass, do data copying in the second loop. Something like:
>>
>> for (i = 0; i < nsplices; i++) {
>> + void *src, *dst;
>> + struct fd f = fdget(fdarr[i]);
>>
>> + pipe = f.file->private_data;
>> + pipe_lock(pipe);
>> +
>> + head = pipe->head;
>> + mask = pipe->ring_size - 1;
>> +
>> + for (tail = pipe->tail; tail != head; tail++) {
>> + struct page *ipage = pipe->bufs[tail & mask].page;
>> + int ioff = pipe->bufs[tail & mask].offset;
>> + int ilen = pipe->bufs[tail & mask].len;
>>
>> + src = kmap_atomic(ipage);
>> + while (ilen > 0) {
>> + int copy = ilen;
>>
>> + if (doff >= dend) {
>> + didx++;
>> + dpage = ap->pages[didx];
>> + doff = ap->descs[didx].offset;
>> + dend = doff + ap->descs[didx].length;
>> + }
>> +
>> + if (copy > dend - doff)
>> + copy = dend - doff;
>> + dst = kmap_atomic(dpage);
>> + memcpy(dst + doff, src + ioff, copy);
>> + kunmap_atomic(dst);
>> +
>> + doff += copy;
>> + ioff += copy;
>> + ilen -= copy;
>> + }
>> }
>> + kunmap_atomic(src);
>> + put_page(ipage);
>> + pipe->bufs[tail & mask].ops = NULL;
>> + pipe->bufs[tail & mask].page = NULL;
>> + pipe->tail = tail + 1;
>> + }
>> + pipe_unlock(pipe);
>> + fdput(f);
>> }
>>
>> This way we can fail before starting to copy data if there’s something wrong
>> in inputs, however it does fdget/ fdput twice, I’m not sure how expensive
>> fdget/fdput can be.
>> If it’s expensive, maybe we can cache fdget() from the first loop and do
>> fdput() after second loop.
>>
>> BTW I just found out there’s no fdput() in the error path.
>>
>> Lastly I was definitely not suggesting to do something similar in
>> fuse_copy_args(), which I agree is not that readable.
>>
>>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
