| 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: <vqocs2hgezf77nvaj3wb7qjrtkanxjp6ethk3jw5cnkwdwmgqv@wfbqx3xi2s27>
Date: Fri, 23 Jun 2023 10:14:03 +0200
From: Stefano Garzarella <sgarzare@...hat.com>
To: Bobby Eshleman <bobbyeshleman@...il.com>
Cc: Arseniy Krasnov <oxffffaa@...il.com>, linux-hyperv@...r.kernel.org,
Bobby Eshleman <bobby.eshleman@...edance.com>, kvm@...r.kernel.org, "Michael S. Tsirkin" <mst@...hat.com>,
VMware PV-Drivers Reviewers <pv-drivers@...are.com>, Simon Horman <simon.horman@...igine.com>,
virtualization@...ts.linux-foundation.org, Eric Dumazet <edumazet@...gle.com>,
Dan Carpenter <dan.carpenter@...aro.org>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
Bryan Tan <bryantan@...are.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Haiyang Zhang <haiyangz@...rosoft.com>,
Stefan Hajnoczi <stefanha@...hat.com>, Vishnu Dasa <vdasa@...are.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, bpf@...r.kernel.org, "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH RFC net-next v4 1/8] vsock/dgram: generalize recvmsg and
drop transport->dgram_dequeue
On Thu, Jun 22, 2023 at 11:37:42PM +0000, Bobby Eshleman wrote:
>On Thu, Jun 22, 2023 at 04:51:41PM +0200, Stefano Garzarella wrote:
>> On Sun, Jun 11, 2023 at 11:43:15PM +0300, Arseniy Krasnov wrote:
>> > Hello Bobby! Thanks for this patchset! Small comment below:
>> >
>> > On 10.06.2023 03:58, Bobby Eshleman wrote:
>> > > This commit drops the transport->dgram_dequeue callback and makes
>> > > vsock_dgram_recvmsg() generic. It also adds additional transport
>> > > callbacks for use by the generic vsock_dgram_recvmsg(), such as for
>> > > parsing skbs for CID/port which vary in format per transport.
>> > >
>> > > Signed-off-by: Bobby Eshleman <bobby.eshleman@...edance.com>
>> > > ---
>> > > drivers/vhost/vsock.c | 4 +-
>> > > include/linux/virtio_vsock.h | 3 ++
>> > > include/net/af_vsock.h | 13 ++++++-
>> > > net/vmw_vsock/af_vsock.c | 51 ++++++++++++++++++++++++-
>> > > net/vmw_vsock/hyperv_transport.c | 17 +++++++--
>> > > net/vmw_vsock/virtio_transport.c | 4 +-
>> > > net/vmw_vsock/virtio_transport_common.c | 18 +++++++++
>> > > net/vmw_vsock/vmci_transport.c | 68 +++++++++++++--------------------
>> > > net/vmw_vsock/vsock_loopback.c | 4 +-
>> > > 9 files changed, 132 insertions(+), 50 deletions(-)
>> > >
>> > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>> > > index 6578db78f0ae..c8201c070b4b 100644
>> > > --- a/drivers/vhost/vsock.c
>> > > +++ b/drivers/vhost/vsock.c
>> > > @@ -410,9 +410,11 @@ static struct virtio_transport vhost_transport = {
>> > > .cancel_pkt = vhost_transport_cancel_pkt,
>> > >
>> > > .dgram_enqueue = virtio_transport_dgram_enqueue,
>> > > - .dgram_dequeue = virtio_transport_dgram_dequeue,
>> > > .dgram_bind = virtio_transport_dgram_bind,
>> > > .dgram_allow = virtio_transport_dgram_allow,
>> > > + .dgram_get_cid = virtio_transport_dgram_get_cid,
>> > > + .dgram_get_port = virtio_transport_dgram_get_port,
>> > > + .dgram_get_length = virtio_transport_dgram_get_length,
>> > >
>> > > .stream_enqueue = virtio_transport_stream_enqueue,
>> > > .stream_dequeue = virtio_transport_stream_dequeue,
>> > > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> > > index c58453699ee9..23521a318cf0 100644
>> > > --- a/include/linux/virtio_vsock.h
>> > > +++ b/include/linux/virtio_vsock.h
>> > > @@ -219,6 +219,9 @@ bool virtio_transport_stream_allow(u32 cid, u32 port);
>> > > int virtio_transport_dgram_bind(struct vsock_sock *vsk,
>> > > struct sockaddr_vm *addr);
>> > > bool virtio_transport_dgram_allow(u32 cid, u32 port);
>> > > +int virtio_transport_dgram_get_cid(struct sk_buff *skb, unsigned int *cid);
>> > > +int virtio_transport_dgram_get_port(struct sk_buff *skb, unsigned int *port);
>> > > +int virtio_transport_dgram_get_length(struct sk_buff *skb, size_t *len);
>> > >
>> > > int virtio_transport_connect(struct vsock_sock *vsk);
>> > >
>> > > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
>> > > index 0e7504a42925..7bedb9ee7e3e 100644
>> > > --- a/include/net/af_vsock.h
>> > > +++ b/include/net/af_vsock.h
>> > > @@ -120,11 +120,20 @@ struct vsock_transport {
>> > >
>> > > /* DGRAM. */
>> > > int (*dgram_bind)(struct vsock_sock *, struct sockaddr_vm *);
>> > > - int (*dgram_dequeue)(struct vsock_sock *vsk, struct msghdr *msg,
>> > > - size_t len, int flags);
>> > > int (*dgram_enqueue)(struct vsock_sock *, struct sockaddr_vm *,
>> > > struct msghdr *, size_t len);
>> > > bool (*dgram_allow)(u32 cid, u32 port);
>> > > + int (*dgram_get_cid)(struct sk_buff *skb, unsigned int *cid);
>> > > + int (*dgram_get_port)(struct sk_buff *skb, unsigned int *port);
>> > > + int (*dgram_get_length)(struct sk_buff *skb, size_t *length);
>> > > +
>> > > + /* The number of bytes into the buffer at which the payload starts, as
>> > > + * first seen by the receiving socket layer. For example, if the
>> > > + * transport presets the skb pointers using skb_pull(sizeof(header))
>> > > + * than this would be zero, otherwise it would be the size of the
>> > > + * header.
>> > > + */
>> > > + const size_t dgram_payload_offset;
>> > >
>> > > /* STREAM. */
>> > > /* TODO: stream_bind() */
>> > > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> > > index efb8a0937a13..ffb4dd8b6ea7 100644
>> > > --- a/net/vmw_vsock/af_vsock.c
>> > > +++ b/net/vmw_vsock/af_vsock.c
>> > > @@ -1271,11 +1271,15 @@ static int vsock_dgram_connect(struct socket *sock,
>> > > int vsock_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
>> > > size_t len, int flags)
>> > > {
>> > > + const struct vsock_transport *transport;
>> > > #ifdef CONFIG_BPF_SYSCALL
>> > > const struct proto *prot;
>> > > #endif
>> > > struct vsock_sock *vsk;
>> > > + struct sk_buff *skb;
>> > > + size_t payload_len;
>> > > struct sock *sk;
>> > > + int err;
>> > >
>> > > sk = sock->sk;
>> > > vsk = vsock_sk(sk);
>> > > @@ -1286,7 +1290,52 @@ int vsock_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
>> > > return prot->recvmsg(sk, msg, len, flags, NULL);
>> > > #endif
>> > >
>> > > - return vsk->transport->dgram_dequeue(vsk, msg, len, flags);
>> > > + if (flags & MSG_OOB || flags & MSG_ERRQUEUE)
>> > > + return -EOPNOTSUPP;
>> > > +
>> > > + transport = vsk->transport;
>> > > +
>> > > + /* Retrieve the head sk_buff from the socket's receive queue. */
>> > > + err = 0;
>> > > + skb = skb_recv_datagram(sk_vsock(vsk), flags, &err);
>> > > + if (!skb)
>> > > + return err;
>> > > +
>> > > + err = transport->dgram_get_length(skb, &payload_len);
>>
>> What about ssize_t return value here?
>>
>> Or maybe a single callback that return both length and offset?
>>
>> .dgram_get_payload_info(skb, &payload_len, &payload_off)
>>
>
>What are your thoughts on Arseniy's idea of using skb->len and adding a
>skb_pull() just before vmci adds the skb to the sk receive queue?
Yep, I agree on that!
Thanks,
Stefano
Powered by blists - more mailing lists