netdev - Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

Open Source and information security mailing list archives

Date:   Mon, 26 Sep 2022 21:30:53 +0000
From:   Bobby Eshleman <bobbyeshleman@...il.com>
To:     Stefano Garzarella <sgarzare@...hat.com>
Cc:     Bobby Eshleman <bobby.eshleman@...il.com>,
        Wei Liu <wei.liu@...nel.org>,
        Cong Wang <cong.wang@...edance.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Bobby Eshleman <bobby.eshleman@...edance.com>,
        Jiang Wang <jiang.wang@...edance.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Dexuan Cui <decui@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
        Stefan Hajnoczi <stefanha@...hat.com>, kvm@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-hyperv@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 2/6] vsock: return errors other than -ENOMEM to socket

On Mon, Sep 26, 2022 at 03:21:45PM +0200, Stefano Garzarella wrote:
> On Mon, Aug 15, 2022 at 10:56:05AM -0700, Bobby Eshleman wrote:
> > This commit allows vsock implementations to return errors
> > to the socket layer other than -ENOMEM. One immediate effect
> > of this is that upon the sk_sndbuf threshold being reached -EAGAIN
> > will be returned and userspace may throttle appropriately.
> > 
> > Resultingly, a known issue with uperf is resolved[1].
> > 
> > Additionally, to preserve legacy behavior for non-virtio
> > implementations, hyperv/vmci force errors to be -ENOMEM so that behavior
> > is unchanged.
> > 
> > [1]: https://gitlab.com/vsock/vsock/-/issues/1
> > 
> > Signed-off-by: Bobby Eshleman <bobby.eshleman@...edance.com>
> > ---
> > include/linux/virtio_vsock.h            | 3 +++
> > net/vmw_vsock/af_vsock.c                | 3 ++-
> > net/vmw_vsock/hyperv_transport.c        | 2 +-
> > net/vmw_vsock/virtio_transport_common.c | 3 ---
> > net/vmw_vsock/vmci_transport.c          | 9 ++++++++-
> > 5 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > index 17ed01466875..9a37eddbb87a 100644
> > --- a/include/linux/virtio_vsock.h
> > +++ b/include/linux/virtio_vsock.h
> > @@ -8,6 +8,9 @@
> > #include <net/sock.h>
> > #include <net/af_vsock.h>
> > 
> > +/* Threshold for detecting small packets to copy */
> > +#define GOOD_COPY_LEN  128
> > +
> 
> This change seems unrelated.
> 
> Please move it in the patch where you need this.
> Maybe it's better to add a prefix if we move it in an header file (e.g.
> VIRTIO_VSOCK_...).
> 
> Thanks,
> Stefano
> 

Oh yes, definitely.

Thanks,
Bobby

> > enum virtio_vsock_metadata_flags {
> > 	VIRTIO_VSOCK_METADATA_FLAGS_REPLY		= BIT(0),
> > 	VIRTIO_VSOCK_METADATA_FLAGS_TAP_DELIVERED	= BIT(1),
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index e348b2d09eac..1893f8aafa48 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -1844,8 +1844,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg,
> > 			written = transport->stream_enqueue(vsk,
> > 					msg, len - total_written);
> > 		}
> > +
> > 		if (written < 0) {
> > -			err = -ENOMEM;
> > +			err = written;
> > 			goto out_err;
> > 		}
> > 
> > diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> > index fd98229e3db3..e99aea571f6f 100644
> > --- a/net/vmw_vsock/hyperv_transport.c
> > +++ b/net/vmw_vsock/hyperv_transport.c
> > @@ -687,7 +687,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
> > 	if (bytes_written)
> > 		ret = bytes_written;
> > 	kfree(send_buf);
> > -	return ret;
> > +	return ret < 0 ? -ENOMEM : ret;
> > }
> > 
> > static s64 hvs_stream_has_data(struct vsock_sock *vsk)
> > diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> > index 920578597bb9..d5780599fe93 100644
> > --- a/net/vmw_vsock/virtio_transport_common.c
> > +++ b/net/vmw_vsock/virtio_transport_common.c
> > @@ -23,9 +23,6 @@
> > /* How long to wait for graceful shutdown of a connection */
> > #define VSOCK_CLOSE_TIMEOUT (8 * HZ)
> > 
> > -/* Threshold for detecting small packets to copy */
> > -#define GOOD_COPY_LEN  128
> > -
> > static const struct virtio_transport *
> > virtio_transport_get_ops(struct vsock_sock *vsk)
> > {
> > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> > index b14f0ed7427b..c927a90dc859 100644
> > --- a/net/vmw_vsock/vmci_transport.c
> > +++ b/net/vmw_vsock/vmci_transport.c
> > @@ -1838,7 +1838,14 @@ static ssize_t vmci_transport_stream_enqueue(
> > 	struct msghdr *msg,
> > 	size_t len)
> > {
> > -	return vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> > +	int err;
> > +
> > +	err = vmci_qpair_enquev(vmci_trans(vsk)->qpair, msg, len, 0);
> > +
> > +	if (err < 0)
> > +		err = -ENOMEM;
> > +
> > +	return err;
> > }
> > 
> > static s64 vmci_transport_stream_has_data(struct vsock_sock *vsk)
> > -- 
> > 2.35.1
> > 
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@...ts.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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