| 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
| ||
|
Message-ID: <acd54194-d397-e721-28e4-73a69257a2a9@gmail.com>
Date: Thu, 27 Jul 2023 11:00:55 +0300
From: Arseniy Krasnov <oxffffaa@...il.com>
To: Bobby Eshleman <bobbyeshleman@...il.com>
Cc: Bobby Eshleman <bobby.eshleman@...edance.com>,
Stefan Hajnoczi <stefanha@...hat.com>,
Stefano Garzarella <sgarzare@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
"K. Y. Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
Bryan Tan <bryantan@...are.com>,
Vishnu Dasa <vdasa@...are.com>,
VMware PV-Drivers Reviewers <pv-drivers@...are.com>,
Dan Carpenter <dan.carpenter@...aro.org>,
Simon Horman <simon.horman@...igine.com>, kvm@...r.kernel.org,
virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org,
bpf@...r.kernel.org
Subject: Re: [PATCH RFC net-next v5 11/14] vhost/vsock: implement datagram
support
On 26.07.2023 20:55, Bobby Eshleman wrote:
> On Sat, Jul 22, 2023 at 11:42:38AM +0300, Arseniy Krasnov wrote:
>>
>>
>> On 19.07.2023 03:50, Bobby Eshleman wrote:
>>> This commit implements datagram support for vhost/vsock by teaching
>>> vhost to use the common virtio transport datagram functions.
>>>
>>> If the virtio RX buffer is too small, then the transmission is
>>> abandoned, the packet dropped, and EHOSTUNREACH is added to the socket's
>>> error queue.
>>>
>>> Signed-off-by: Bobby Eshleman <bobby.eshleman@...edance.com>
>>> ---
>>> drivers/vhost/vsock.c | 62 +++++++++++++++++++++++++++++++++++++++++++++---
>>> net/vmw_vsock/af_vsock.c | 5 +++-
>>> 2 files changed, 63 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>>> index d5d6a3c3f273..da14260c6654 100644
>>> --- a/drivers/vhost/vsock.c
>>> +++ b/drivers/vhost/vsock.c
>>> @@ -8,6 +8,7 @@
>>> */
>>> #include <linux/miscdevice.h>
>>> #include <linux/atomic.h>
>>> +#include <linux/errqueue.h>
>>> #include <linux/module.h>
>>> #include <linux/mutex.h>
>>> #include <linux/vmalloc.h>
>>> @@ -32,7 +33,8 @@
>>> enum {
>>> VHOST_VSOCK_FEATURES = VHOST_FEATURES |
>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
>>> - (1ULL << VIRTIO_VSOCK_F_SEQPACKET)
>>> + (1ULL << VIRTIO_VSOCK_F_SEQPACKET) |
>>> + (1ULL << VIRTIO_VSOCK_F_DGRAM)
>>> };
>>>
>>> enum {
>>> @@ -56,6 +58,7 @@ struct vhost_vsock {
>>> atomic_t queued_replies;
>>>
>>> u32 guest_cid;
>>> + bool dgram_allow;
>>> bool seqpacket_allow;
>>> };
>>>
>>> @@ -86,6 +89,32 @@ static struct vhost_vsock *vhost_vsock_get(u32 guest_cid)
>>> return NULL;
>>> }
>>>
>>> +/* Claims ownership of the skb, do not free the skb after calling! */
>>> +static void
>>> +vhost_transport_error(struct sk_buff *skb, int err)
>>> +{
>>> + struct sock_exterr_skb *serr;
>>> + struct sock *sk = skb->sk;
>>> + struct sk_buff *clone;
>>> +
>>> + serr = SKB_EXT_ERR(skb);
>>> + memset(serr, 0, sizeof(*serr));
>>> + serr->ee.ee_errno = err;
>>> + serr->ee.ee_origin = SO_EE_ORIGIN_NONE;
>>> +
>>> + clone = skb_clone(skb, GFP_KERNEL);
>>
>> May for skb which is error carrier we can use 'sock_omalloc()', not 'skb_clone()' ? TCP uses skb
>> allocated by this function as carriers of error structure. I guess 'skb_clone()' also clones data of origin,
>> but i think that there is no need in data as we insert it to error queue of the socket.
>>
>> What do You think?
>
> IIUC skb_clone() is often used in this scenario so that the user can
> retrieve the error-causing packet from the error queue. Is there some
> reason we shouldn't do this?
>
> I'm seeing that the serr bits need to occur on the clone here, not the
> original. I didn't realize the SKB_EXT_ERR() is a skb->cb cast. I'm not
> actually sure how this passes the test case since ->cb isn't cloned.
Ah yes, sorry, You are right, I just confused this case with zerocopy completion
handling - there we allocate "empty" skb which carries completion metadata in its
'cb' field.
Hm, but can't we just reinsert current skb (update it's 'cb' as 'sock_exterr_skb')
to error queue of the socket without cloning it ?
Thanks, Arseniy
>
>>
>>> + if (!clone)
>>> + return;
>>
>> What will happen here 'if (!clone)' ? skb will leak as it was removed from queue?
>>
>
> Ah yes, true.
>
>>> +
>>> + if (sock_queue_err_skb(sk, clone))
>>> + kfree_skb(clone);
>>> +
>>> + sk->sk_err = err;
>>> + sk_error_report(sk);
>>> +
>>> + kfree_skb(skb);
>>> +}
>>> +
>>> static void
>>> vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
>>> struct vhost_virtqueue *vq)
>>> @@ -160,9 +189,15 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
>>> hdr = virtio_vsock_hdr(skb);
>>>
>>> /* If the packet is greater than the space available in the
>>> - * buffer, we split it using multiple buffers.
>>> + * buffer, we split it using multiple buffers for connectible
>>> + * sockets and drop the packet for datagram sockets.
>>> */
>>> if (payload_len > iov_len - sizeof(*hdr)) {
>>> + if (le16_to_cpu(hdr->type) == VIRTIO_VSOCK_TYPE_DGRAM) {
>>> + vhost_transport_error(skb, EHOSTUNREACH);
>>> + continue;
>>> + }
>>> +
>>> payload_len = iov_len - sizeof(*hdr);
>>>
>>> /* As we are copying pieces of large packet's buffer to
>>> @@ -394,6 +429,7 @@ static bool vhost_vsock_more_replies(struct vhost_vsock *vsock)
>>> return val < vq->num;
>>> }
>>>
>>> +static bool vhost_transport_dgram_allow(u32 cid, u32 port);
>>> static bool vhost_transport_seqpacket_allow(u32 remote_cid);
>>>
>>> static struct virtio_transport vhost_transport = {
>>> @@ -410,7 +446,8 @@ static struct virtio_transport vhost_transport = {
>>> .cancel_pkt = vhost_transport_cancel_pkt,
>>>
>>> .dgram_enqueue = virtio_transport_dgram_enqueue,
>>> - .dgram_allow = virtio_transport_dgram_allow,
>>> + .dgram_allow = vhost_transport_dgram_allow,
>>> + .dgram_addr_init = virtio_transport_dgram_addr_init,
>>>
>>> .stream_enqueue = virtio_transport_stream_enqueue,
>>> .stream_dequeue = virtio_transport_stream_dequeue,
>>> @@ -443,6 +480,22 @@ static struct virtio_transport vhost_transport = {
>>> .send_pkt = vhost_transport_send_pkt,
>>> };
>>>
>>> +static bool vhost_transport_dgram_allow(u32 cid, u32 port)
>>> +{
>>> + struct vhost_vsock *vsock;
>>> + bool dgram_allow = false;
>>> +
>>> + rcu_read_lock();
>>> + vsock = vhost_vsock_get(cid);
>>> +
>>> + if (vsock)
>>> + dgram_allow = vsock->dgram_allow;
>>> +
>>> + rcu_read_unlock();
>>> +
>>> + return dgram_allow;
>>> +}
>>> +
>>> static bool vhost_transport_seqpacket_allow(u32 remote_cid)
>>> {
>>> struct vhost_vsock *vsock;
>>> @@ -799,6 +852,9 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>>> if (features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET))
>>> vsock->seqpacket_allow = true;
>>>
>>> + if (features & (1ULL << VIRTIO_VSOCK_F_DGRAM))
>>> + vsock->dgram_allow = true;
>>> +
>>> for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
>>> vq = &vsock->vqs[i];
>>> mutex_lock(&vq->mutex);
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index e73f3b2c52f1..449ed63ac2b0 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -1427,9 +1427,12 @@ int vsock_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
>>> return prot->recvmsg(sk, msg, len, flags, NULL);
>>> #endif
>>>
>>> - if (flags & MSG_OOB || flags & MSG_ERRQUEUE)
>>> + if (unlikely(flags & MSG_OOB))
>>> return -EOPNOTSUPP;
>>>
>>> + if (unlikely(flags & MSG_ERRQUEUE))
>>> + return sock_recv_errqueue(sk, msg, len, SOL_VSOCK, 0);
>>> +
>>
>> Sorry, but I get build error here, because SOL_VSOCK in undefined. I think it should be added to
>> include/linux/socket.h and to uapi files also for future use in userspace.
>>
>
> Strange, I built each patch individually without issue. My base is
> netdev/main with your SOL_VSOCK patch applied. I will look today and see
> if I'm missing something.
>
>> Also Stefano Garzarella <sgarzare@...hat.com> suggested to add define something like VSOCK_RECVERR,
>> in the same way as IP_RECVERR, and use it as last parameter of 'sock_recv_errqueue()'.
>>
>
> Got it, thanks.
>
>>> transport = vsk->transport;
>>>
>>> /* Retrieve the head sk_buff from the socket's receive queue. */
>>>
>>
>> Thanks, Arseniy
>
> Thanks,
> Bobby
Powered by blists - more mailing lists