Dan Carpenter
2017-Apr-06 05:29 UTC
[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, &len,
&ctx))) {
^^^^
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, &len)) !=
NULL) {
1048 bytes += receive_buf(vi, rq, buf, len, NULL);
1049 received++;
1050 }
1051 }
1052
regards,
dan carpenter
Michael S. Tsirkin
2017-Apr-06 11:43 UTC
[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, &len, &ctx))) { > ^^^^ > 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, &len)) != NULL) { > 1048 bytes += receive_buf(vi, rq, buf, len, NULL); > 1049 received++; > 1050 } > 1051 } > 1052 > > regards, > dan carpenter
Michael S. Tsirkin
2017-Jun-02 15:35 UTC
[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, &len, &ctx))) { > ^^^^ > 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, &len)) != NULL) { > 1048 bytes += receive_buf(vi, rq, buf, len, NULL); > 1049 received++; > 1050 } > 1051 } > 1052 > > regards, > dan carpenter
Possibly Parallel Threads
- [bug report] virtio_net: rework mergeable buffer handling
- [PATCH net-next 1/3] virtio-net: remove unnecessary parameter of virtnet_xdp_xmit()
- [PATCH net-next 1/3] virtio-net: remove unnecessary parameter of virtnet_xdp_xmit()
- [PATCH 5/6] virtio_net: rework mergeable buffer handling
- [PATCH 5/6] virtio_net: rework mergeable buffer handling