netdev - Re: [PATCH net-next v5 4/4] vsock/virtio: MSG

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: <00f2b7bdb18e0eaa42f0cca542a9530564615475.camel@redhat.com>
Date: Tue, 01 Aug 2023 16:04:55 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Arseniy Krasnov <avkrasnov@...rdevices.ru>, Stefan Hajnoczi
 <stefanha@...hat.com>, Stefano Garzarella <sgarzare@...hat.com>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>,  "Michael S. Tsirkin" <mst@...hat.com>, Jason
 Wang <jasowang@...hat.com>, Bobby Eshleman <bobby.eshleman@...edance.com>
Cc: 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 v5 4/4] vsock/virtio: MSG_ZEROCOPY flag support

On Tue, 2023-08-01 at 16:36 +0300, Arseniy Krasnov wrote:
> 
> On 01.08.2023 16:34, Paolo Abeni wrote:
> > On Sun, 2023-07-30 at 11:59 +0300, Arseniy Krasnov wrote:
> > > +static int virtio_transport_fill_skb(struct sk_buff *skb,
> > > +				     struct virtio_vsock_pkt_info *info,
> > > +				     size_t len,
> > > +				     bool zcopy)
> > > +{
> > > +	if (zcopy) {
> > > +		return __zerocopy_sg_from_iter(info->msg, NULL, skb,
> > > +					      &info->msg->msg_iter,
> > > +					      len);
> > > +	} else {
> > 
> > 
> > No need for an else statement after 'return'
> > 
> > > +		void *payload;
> > > +		int err;
> > > +
> > > +		payload = skb_put(skb, len);
> > > +		err = memcpy_from_msg(payload, info->msg, len);
> > > +		if (err)
> > > +			return -1;
> > > +
> > > +		if (msg_data_left(info->msg))
> > > +			return 0;
> > > +
> > 
> > This path does not update truesize, evem if it increases the skb len...
> 
> Thanks, I'll fix it.
> 
> > 
> > > +		return 0;
> > > +	}
> > > +}
> > 
> > [...]
> > 
> > > @@ -214,6 +251,70 @@ static u16 virtio_transport_get_type(struct sock *sk)
> > >  		return VIRTIO_VSOCK_TYPE_SEQPACKET;
> > >  }
> > >  
> > > +static struct sk_buff *virtio_transport_alloc_skb(struct vsock_sock *vsk,
> > > +						  struct virtio_vsock_pkt_info *info,
> > > +						  size_t payload_len,
> > > +						  bool zcopy,
> > > +						  u32 src_cid,
> > > +						  u32 src_port,
> > > +						  u32 dst_cid,
> > > +						  u32 dst_port)
> > > +{
> > > +	struct sk_buff *skb;
> > > +	size_t skb_len;
> > > +
> > > +	skb_len = VIRTIO_VSOCK_SKB_HEADROOM;
> > > +
> > > +	if (!zcopy)
> > > +		skb_len += payload_len;
> > > +
> > > +	skb = virtio_vsock_alloc_skb(skb_len, GFP_KERNEL);
> > > +	if (!skb)
> > > +		return NULL;
> > > +
> > > +	virtio_transport_init_hdr(skb, info, src_cid, src_port,
> > > +				  dst_cid, dst_port,
> > > +				  payload_len);
> > > +
> > > +	/* Set owner here, because '__zerocopy_sg_from_iter()' uses
> > > +	 * owner of skb without check to update 'sk_wmem_alloc'.
> > > +	 */
> > > +	if (vsk)
> > > +		skb_set_owner_w(skb, sk_vsock(vsk));
> > 
> > ... which can lead to bad things(TM) if the skb goes trough some later
> > non trivial processing, due to the above skb_set_owner_w().
> > 
> > Additionally can be the following condition be true:
> > 
> > 	vsk == NULL && (info->msg && payload_len > 0) && zcopy
> > 
> > ???
> 
> No, vsk == NULL only when we reset connection, in that case both info->msg == NULL and payload_len == 0,
> as this is control message without any data.

Perhaps a comment with possibly even a WARN_ON_ONCE(!<the above>) could
help ;)

Thanks!

Paolo