Re: [bug report] virtio_net: rework mergeable buffer handling
On Thu, Apr 06, 2017 at 08:29:49AM +0300, Dan Carpenter wrote:
> Hello Michael S. Tsirkin,
>
> The patch 6c8e5f3c41c8: "virtio_net: rework mergeable buffer
> handling" from Mar 6, 2017, leads to the following static checker
> warning:
>
> drivers/net/virtio_net.c:1042 virtnet_receive()
> error: uninitialized symbol 'ctx'.
>
> drivers/net/virtio_net.c
> 1030 static int virtnet_receive(struct receive_queue *rq, int budget)
> 1031 {
> 1032 struct virtnet_info *vi = rq->vq->vdev->priv;
> 1033 unsigned int len, received = 0, bytes = 0;
> 1034 void *buf;
> 1035 struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
> 1036
> 1037 if (vi->mergeable_rx_bufs) {
> 1038 void *ctx;
> ^^^
> 1039
> 1040 while (received < budget &&
> 1041 (buf = virtqueue_get_buf_ctx(rq->vq, ,
> ))) {
>
>
> 1042 bytes += receive_buf(vi, rq, buf, len, ctx);
>^^^
>
> It's possible that this code is correct, but I looked at it and wasn't
> immediately convinced. Returning non-NULL buf is not sufficient to
> show that "ctx" is initialized, because if it's vq->indirect then "buf"
> is still unintialized. Also it's possible that receive_buf() checks
> vq->indirect through some side effect way that I didn't see so it
> doesn't use the uninitialized value...
>
> I feel like if this is a false positive, that means the rules are too
> subtle... :/
Yes, false positive I think.
What happens is this: __vring_new_virtqueue does:
vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC) &&
!context;
so indirect is never set with context.
Adding code comments should help - could you take a stub at it?
> 1043 received++;
> 1044 }
> 1045 } else {
> 1046 while (received < budget &&
> 1047 (buf = virtqueue_get_buf(rq->vq, )) !=
> NULL) {
> 1048 bytes += receive_buf(vi, rq, buf, len, NULL);
> 1049 received++;
> 1050 }
> 1051 }
> 1052
>
> regards,
> dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [bug report] virtio_net: rework mergeable buffer handling
On Thu, Apr 06, 2017 at 08:29:49AM +0300, Dan Carpenter wrote:
> Hello Michael S. Tsirkin,
>
> The patch 6c8e5f3c41c8: "virtio_net: rework mergeable buffer
> handling" from Mar 6, 2017, leads to the following static checker
> warning:
>
> drivers/net/virtio_net.c:1042 virtnet_receive()
> error: uninitialized symbol 'ctx'.
Thanks will fix asap.
> drivers/net/virtio_net.c
> 1030 static int virtnet_receive(struct receive_queue *rq, int budget)
> 1031 {
> 1032 struct virtnet_info *vi = rq->vq->vdev->priv;
> 1033 unsigned int len, received = 0, bytes = 0;
> 1034 void *buf;
> 1035 struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
> 1036
> 1037 if (vi->mergeable_rx_bufs) {
> 1038 void *ctx;
> ^^^
> 1039
> 1040 while (received < budget &&
> 1041 (buf = virtqueue_get_buf_ctx(rq->vq, ,
> ))) {
>
>
> 1042 bytes += receive_buf(vi, rq, buf, len, ctx);
>^^^
>
> It's possible that this code is correct, but I looked at it and wasn't
> immediately convinced. Returning non-NULL buf is not sufficient to
> show that "ctx" is initialized, because if it's vq->indirect then "buf"
> is still unintialized. Also it's possible that receive_buf() checks
> vq->indirect through some side effect way that I didn't see so it
> doesn't use the uninitialized value...
>
> I feel like if this is a false positive, that means the rules are too
> subtle... :/
>
> 1043 received++;
> 1044 }
> 1045 } else {
> 1046 while (received < budget &&
> 1047 (buf = virtqueue_get_buf(rq->vq, )) !=
> NULL) {
> 1048 bytes += receive_buf(vi, rq, buf, len, NULL);
> 1049 received++;
> 1050 }
> 1051 }
> 1052
>
> regards,
> dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[bug report] virtio_net: rework mergeable buffer handling
Hello Michael S. Tsirkin,
The patch 6c8e5f3c41c8: "virtio_net: rework mergeable buffer
handling" from Mar 6, 2017, leads to the following static checker
warning:
drivers/net/virtio_net.c:1042 virtnet_receive()
error: uninitialized symbol 'ctx'.
drivers/net/virtio_net.c
1030 static int virtnet_receive(struct receive_queue *rq, int budget)
1031 {
1032 struct virtnet_info *vi = rq->vq->vdev->priv;
1033 unsigned int len, received = 0, bytes = 0;
1034 void *buf;
1035 struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
1036
1037 if (vi->mergeable_rx_bufs) {
1038 void *ctx;
^^^
1039
1040 while (received < budget &&
1041 (buf = virtqueue_get_buf_ctx(rq->vq, ,
))) {
1042 bytes += receive_buf(vi, rq, buf, len, ctx);
^^^
It's possible that this code is correct, but I looked at it and wasn't
immediately convinced. Returning non-NULL buf is not sufficient to
show that "ctx" is initialized, because if it's vq->indirect then "buf"
is still unintialized. Also it's possible that receive_buf() checks
vq->indirect through some side effect way that I didn't see so it
doesn't use the uninitialized value...
I feel like if this is a false positive, that means the rules are too
subtle... :/
1043 received++;
1044 }
1045 } else {
1046 while (received < budget &&
1047 (buf = virtqueue_get_buf(rq->vq, )) != NULL)
{
1048 bytes += receive_buf(vi, rq, buf, len, NULL);
1049 received++;
1050 }
1051 }
1052
regards,
dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
