On Fri, Sep 27, 2019 at 01:26:57PM +0200, Stefano Garzarella wrote:
> @@ -140,18 +145,11 @@ struct vsock_transport {
> struct vsock_transport_send_notify_data *);
> int (*notify_send_post_enqueue)(struct vsock_sock *, ssize_t,
> struct vsock_transport_send_notify_data *);
> + int (*notify_buffer_size)(struct vsock_sock *, u64 *);
Is ->notify_buffer_size() called under lock_sock(sk)? If yes, please
document it.
> +static void vsock_update_buffer_size(struct vsock_sock *vsk,
> + const struct vsock_transport *transport,
> + u64 val)
> +{
> + if (val > vsk->buffer_max_size)
> + val = vsk->buffer_max_size;
> +
> + if (val < vsk->buffer_min_size)
> + val = vsk->buffer_min_size;
> +
> + if (val != vsk->buffer_size &&
> + transport && transport->notify_buffer_size)
> + transport->notify_buffer_size(vsk, &val);
Why does this function return an int if we don't check the return value?
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index fc046c071178..bac9e7430a2e 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -403,17 +403,13 @@ int virtio_transport_do_socket_init(struct vsock_sock *vsk,
> if (psk) {
> struct virtio_vsock_sock *ptrans = psk->trans;
>
> - vvs->buf_size = ptrans->buf_size;
> - vvs->buf_size_min = ptrans->buf_size_min;
> - vvs->buf_size_max = ptrans->buf_size_max;
> vvs->peer_buf_alloc = ptrans->peer_buf_alloc;
> - } else {
> - vvs->buf_size = VIRTIO_VSOCK_DEFAULT_BUF_SIZE;
> - vvs->buf_size_min = VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE;
> - vvs->buf_size_max = VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE;
> }
>
> - vvs->buf_alloc = vvs->buf_size;
> + if (vsk->buffer_size > VIRTIO_VSOCK_MAX_BUF_SIZE)
> + vsk->buffer_size = VIRTIO_VSOCK_MAX_BUF_SIZE;
Hmm...this could be outside the [min, max] range. I'm not sure how much
it matters.
Another issue is that this patch drops the VIRTIO_VSOCK_MAX_BUF_SIZE
limit that used to be enforced by virtio_transport_set_buffer_size().
Now the limit is only applied at socket init time. If the buffer size
is changed later then VIRTIO_VSOCK_MAX_BUF_SIZE can be exceeded. If
that doesn't matter, why even bother with VIRTIO_VSOCK_MAX_BUF_SIZE
here?
Attachment:
signature.asc
Description: PGP signature
