netdev - Re: [PATCH net-next v1 08/12] vsock: enable setting SO

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]

Date: Wed, 27 Sep 2023 09:35:31 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Arseniy Krasnov <avkrasnov@...utedevices.com>
Cc: Stefan Hajnoczi <stefanha@...hat.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	"Michael S. Tsirkin" <mst@...hat.com>, Jason Wang <jasowang@...hat.com>, 
	Bobby Eshleman <bobby.eshleman@...edance.com>, kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, kernel@...rdevices.ru, 
	oxffffaa@...il.com
Subject: Re: [PATCH net-next v1 08/12] vsock: enable setting SO_ZEROCOPY

On Tue, Sep 26, 2023 at 10:38:06PM +0300, Arseniy Krasnov wrote:
>
>
>On 26.09.2023 15:56, Stefano Garzarella wrote:
>> On Fri, Sep 22, 2023 at 08:24:24AM +0300, Arseniy Krasnov wrote:
>>> For AF_VSOCK, zerocopy tx mode depends on transport, so this option must
>>> be set in AF_VSOCK implementation where transport is accessible (if
>>> transport is not set during setting SO_ZEROCOPY: for example socket is
>>> not connected, then SO_ZEROCOPY will be enabled, but once transport will
>>> be assigned, support of this type of transmission will be checked).
>>>
>>> To handle SO_ZEROCOPY, AF_VSOCK implementation uses SOCK_CUSTOM_SOCKOPT
>>> bit, thus handling SOL_SOCKET option operations, but all of them except
>>> SO_ZEROCOPY will be forwarded to the generic handler by calling
>>> 'sock_setsockopt()'.
>>>
>>> Signed-off-by: Arseniy Krasnov <avkrasnov@...utedevices.com>
>>> ---
>>> Changelog:
>>> v5(big patchset) -> v1:
>>>  * Compact 'if' conditions.
>>>  * Rename 'zc_val' to 'zerocopy'.
>>>  * Use 'zerocopy' value directly in 'sock_valbool_flag()', without
>>>    ?: operator.
>>>  * Set 'SOCK_CUSTOM_SOCKOPT' bit for connectible sockets only, as
>>>    suggested by Bobby Eshleman <bobbyeshleman@...il.com>.
>>>
>>> net/vmw_vsock/af_vsock.c | 46 ++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index 482300eb88e0..c05a42e02a17 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -1406,8 +1406,16 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
>>>             goto out;
>>>         }
>>>
>>> -        if (vsock_msgzerocopy_allow(transport))
>>> +        if (vsock_msgzerocopy_allow(transport)) {
>>>             set_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags);
>>> +        } else if (sock_flag(sk, SOCK_ZEROCOPY)) {
>>> +            /* If this option was set before 'connect()',
>>> +             * when transport was unknown, check that this
>>> +             * feature is supported here.
>>> +             */
>>> +            err = -EOPNOTSUPP;
>>> +            goto out;
>>> +        }
>>>
>>>         err = vsock_auto_bind(vsk);
>>>         if (err)
>>> @@ -1643,7 +1651,7 @@ static int vsock_connectible_setsockopt(struct socket *sock,
>>>     const struct vsock_transport *transport;
>>>     u64 val;
>>>
>>> -    if (level != AF_VSOCK)
>>> +    if (level != AF_VSOCK && level != SOL_SOCKET)
>>>         return -ENOPROTOOPT;
>>>
>>> #define COPY_IN(_v)                                       \
>>> @@ -1666,6 +1674,34 @@ static int vsock_connectible_setsockopt(struct socket *sock,
>>>
>>>     transport = vsk->transport;
>>>
>>> +    if (level == SOL_SOCKET) {
>>> +        int zerocopy;
>>> +
>>> +        if (optname != SO_ZEROCOPY) {
>>> +            release_sock(sk);
>>> +            return sock_setsockopt(sock, level, optname, optval, optlen);
>>> +        }
>>> +
>>> +        /* Use 'int' type here, because variable to
>>> +         * set this option usually has this type.
>>> +         */
>>> +        COPY_IN(zerocopy);
>>> +
>>> +        if (zerocopy < 0 || zerocopy > 1) {
>>> +            err = -EINVAL;
>>> +            goto exit;
>>> +        }
>>> +
>>> +        if (transport && !vsock_msgzerocopy_allow(transport)) {
>>> +            err = -EOPNOTSUPP;
>>> +            goto exit;
>>> +        }
>>> +
>>> +        sock_valbool_flag(sk, SOCK_ZEROCOPY,
>>> +                  zerocopy);
>>
>> it's not necessary to wrap this call.
>
>Sorry, what do you mean ?

I mean that can be on the same line:

	sock_valbool_flag(sk, SOCK_ZEROCOPY, zerocopy);

Stefano