Re: [PATCH RFC net-next v5 07/14] virtio/vsock: add common datagram send path

[Bobby Eshleman]

On Sat, Jul 22, 2023 at 11:16:05AM +0300, Arseniy Krasnov wrote:
> 
> 
> On 19.07.2023 03:50, Bobby Eshleman wrote:
> > This commit implements the common function
> > virtio_transport_dgram_enqueue for enqueueing datagrams. It does not add
> > usage in either vhost or virtio yet.
> > 
> > Signed-off-by: Bobby Eshleman <bobby.eshleman@xxxxxxxxxxxxx>
> > ---
> >  net/vmw_vsock/virtio_transport_common.c | 76 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 75 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index ffcbdd77feaa..3bfaff758433 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -819,7 +819,81 @@ virtio_transport_dgram_enqueue(struct vsock_sock *vsk,
> >  			       struct msghdr *msg,
> >  			       size_t dgram_len)
> >  {
> > -	return -EOPNOTSUPP;
> > +	/* Here we are only using the info struct to retain style uniformity
> > +	 * and to ease future refactoring and merging.
> > +	 */
> > +	struct virtio_vsock_pkt_info info_stack = {
> > +		.op = VIRTIO_VSOCK_OP_RW,
> > +		.msg = msg,
> > +		.vsk = vsk,
> > +		.type = VIRTIO_VSOCK_TYPE_DGRAM,
> > +	};
> > +	const struct virtio_transport *t_ops;
> > +	struct virtio_vsock_pkt_info *info;
> > +	struct sock *sk = sk_vsock(vsk);
> > +	struct virtio_vsock_hdr *hdr;
> > +	u32 src_cid, src_port;
> > +	struct sk_buff *skb;
> > +	void *payload;
> > +	int noblock;
> > +	int err;
> > +
> > +	info = &info_stack;
> 
> I think 'info' assignment could be moved below, to the place where it is used
> first time.
> 
> > +
> > +	if (dgram_len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
> > +		return -EMSGSIZE;
> > +
> > +	t_ops = virtio_transport_get_ops(vsk);
> > +	if (unlikely(!t_ops))
> > +		return -EFAULT;
> > +
> > +	/* Unlike some of our other sending functions, this function is not
> > +	 * intended for use without a msghdr.
> > +	 */
> > +	if (WARN_ONCE(!msg, "vsock dgram bug: no msghdr found for dgram enqueue\n"))
> > +		return -EFAULT;
> 
> Sorry, but is that possible? I thought 'msg' is always provided by general socket layer (e.g. before
> af_vsock.c code) and can't be NULL for DGRAM. Please correct me if i'm wrong.
> 
> Also I see, that in af_vsock.c , 'vsock_dgram_sendmsg()' dereferences 'msg' for checking MSG_OOB without any
> checks (before calling transport callback - this function in case of virtio). So I think if we want to keep
> this type of check - such check must be placed in af_vsock.c or somewhere before first dereference of this pointer.
> 

There is some talk about dgram sockets adding additional messages types
in the future that help with congestion control. Those messages won't
come from the socket layer, so msghdr will be null. Since there is no
other function for sending datagrams, it seemed likely that this
function would be reworked for that purpose. I felt that adding this
check was a direct way to make it explicit that this function is
currently designed only for the socket-layer caller.

Perhaps a comment would suffice?

> > +
> > +	noblock = msg->msg_flags & MSG_DONTWAIT;
> > +
> > +	/* Use sock_alloc_send_skb to throttle by sk_sndbuf. This helps avoid
> > +	 * triggering the OOM.
> > +	 */
> > +	skb = sock_alloc_send_skb(sk, dgram_len + VIRTIO_VSOCK_SKB_HEADROOM,
> > +				  noblock, &err);
> > +	if (!skb)
> > +		return err;
> > +
> > +	skb_reserve(skb, VIRTIO_VSOCK_SKB_HEADROOM);
> > +
> > +	src_cid = t_ops->transport.get_local_cid();
> > +	src_port = vsk->local_addr.svm_port;
> > +
> > +	hdr = virtio_vsock_hdr(skb);
> > +	hdr->type	= cpu_to_le16(info->type);
> > +	hdr->op		= cpu_to_le16(info->op);
> > +	hdr->src_cid	= cpu_to_le64(src_cid);
> > +	hdr->dst_cid	= cpu_to_le64(remote_addr->svm_cid);
> > +	hdr->src_port	= cpu_to_le32(src_port);
> > +	hdr->dst_port	= cpu_to_le32(remote_addr->svm_port);
> > +	hdr->flags	= cpu_to_le32(info->flags);
> > +	hdr->len	= cpu_to_le32(dgram_len);
> > +
> > +	skb_set_owner_w(skb, sk);
> > +
> > +	payload = skb_put(skb, dgram_len);
> > +	err = memcpy_from_msg(payload, msg, dgram_len);
> > +	if (err)
> > +		return err;
> 
> Do we need free allocated skb here ?
> 

Yep, thanks.

> > +
> > +	trace_virtio_transport_alloc_pkt(src_cid, src_port,
> > +					 remote_addr->svm_cid,
> > +					 remote_addr->svm_port,
> > +					 dgram_len,
> > +					 info->type,
> > +					 info->op,
> > +					 0);
> > +
> > +	return t_ops->send_pkt(skb);
> >  }
> >  EXPORT_SYMBOL_GPL(virtio_transport_dgram_enqueue);
> >  
> > 
> 
> Thanks, Arseniy

Thanks for the review!

Best,
Bobby