netdev - Re: [PATCH v1 12/15] io_uring/zcrx: add io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7cee82f7-188f-438a-9fe1-086aeda66caf@gmail.com>
Date: Wed, 9 Oct 2024 20:27:15 +0100
From: Pavel Begunkov <asml.silence@...il.com>
To: Jens Axboe <axboe@...nel.dk>, David Wei <dw@...idwei.uk>,
 io-uring@...r.kernel.org, netdev@...r.kernel.org
Cc: Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jesper Dangaard Brouer <hawk@...nel.org>, David Ahern <dsahern@...nel.org>,
 Mina Almasry <almasrymina@...gle.com>
Subject: Re: [PATCH v1 12/15] io_uring/zcrx: add io_recvzc request

On 10/9/24 20:01, Jens Axboe wrote:
> On 10/9/24 12:51 PM, Pavel Begunkov wrote:
>> On 10/9/24 19:28, Jens Axboe wrote:
>>>> diff --git a/io_uring/net.c b/io_uring/net.c
>>>> index d08abcca89cc..482e138d2994 100644
>>>> --- a/io_uring/net.c
>>>> +++ b/io_uring/net.c
>>>> @@ -1193,6 +1201,76 @@ int io_recv(struct io_kiocb *req, unsigned int issue_flags)
>>>>        return ret;
>>>>    }
>>>>    +int io_recvzc_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>>> +{
>>>> +    struct io_recvzc *zc = io_kiocb_to_cmd(req, struct io_recvzc);
>>>> +    unsigned ifq_idx;
>>>> +
>>>> +    if (unlikely(sqe->file_index || sqe->addr2 || sqe->addr ||
>>>> +             sqe->len || sqe->addr3))
>>>> +        return -EINVAL;
>>>> +
>>>> +    ifq_idx = READ_ONCE(sqe->zcrx_ifq_idx);
>>>> +    if (ifq_idx != 0)
>>>> +        return -EINVAL;
>>>> +    zc->ifq = req->ctx->ifq;
>>>> +    if (!zc->ifq)
>>>> +        return -EINVAL;
>>>
>>> This is read and assigned to 'zc' here, but then the issue handler does
>>> it again? I'm assuming that at some point we'll have ifq selection here,
>>> and then the issue handler will just use zc->ifq. So this part should
>>> probably remain, and the issue side just use zc->ifq?
>>
>> Yep, fairly overlooked. It's not a real problem, but should
>> only be fetched and checked here.
> 
> Right
> 
>>>> +    /* All data completions are posted as aux CQEs. */
>>>> +    req->flags |= REQ_F_APOLL_MULTISHOT;
>>>
>>> This puzzles me a bit...
>>
>> Well, it's a multishot request. And that flag protects from cq
>> locking rules violations, i.e. avoiding multishot reqs from
>> posting from io-wq.
> 
> Maybe make it more like the others and require that
> IORING_RECV_MULTISHOT is set then, and set it based on that?

if (IORING_RECV_MULTISHOT)
	return -EINVAL;
req->flags |= REQ_F_APOLL_MULTISHOT;

It can be this if that's the preference. It's a bit more consistent,
but might be harder to use. Though I can just hide the flag behind
liburing helpers, would spare from neverending GH issues asking
why it's -EINVAL'ed


>>>> +    zc->flags = READ_ONCE(sqe->ioprio);
>>>> +    zc->msg_flags = READ_ONCE(sqe->msg_flags);
>>>> +    if (zc->msg_flags)
>>>> +        return -EINVAL;
>>>
>>> Maybe allow MSG_DONTWAIT at least? You already pass that in anyway.
>>
>> What would the semantics be? The io_uring nowait has always
>> been a pure mess because it's not even clear what it supposed
>> to mean for async requests.
> 
> Yeah can't disagree with that. Not a big deal, doesn't really matter,
> can stay as-is.

I went through the MSG_* flags before looking which ones might
even make sense here and be useful... Let's better enable if
it'd be needed.

>>>> +    if (zc->flags & ~(IORING_RECVSEND_POLL_FIRST | IORING_RECV_MULTISHOT))
>>>> +        return -EINVAL;
>>>> +
>>>> +
>>>> +#ifdef CONFIG_COMPAT
>>>> +    if (req->ctx->compat)
>>>> +        zc->msg_flags |= MSG_CMSG_COMPAT;
>>>> +#endif
>>>> +    return 0;
>>>> +}
>>>
>>> Heh, we could probably just return -EINVAL for that case, but since this
>>> is all we need, fine.
>>
>> Well, there is no msghdr, cmsg nor iovec there, so doesn't even
>> make sense to set it. Can fail as well, I don't anyone would care.
> 
> Then let's please just kill it, should not need a check for that then.
> 

-- 
Pavel Begunkov