* [bug report] virtio_net: rework mergeable buffer handling
@ 2017-04-06 5:29 Dan Carpenter
2017-04-06 11:43 ` Michael S. Tsirkin
2017-06-02 15:35 ` Michael S. Tsirkin
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2017-04-06 5:29 UTC (permalink / raw)
To: mst; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] virtio_net: rework mergeable buffer handling
2017-04-06 5:29 [bug report] virtio_net: rework mergeable buffer handling Dan Carpenter
@ 2017-04-06 11:43 ` Michael S. Tsirkin
2017-06-02 15:35 ` Michael S. Tsirkin
1 sibling, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2017-04-06 11:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] virtio_net: rework mergeable buffer handling
2017-04-06 5:29 [bug report] virtio_net: rework mergeable buffer handling Dan Carpenter
2017-04-06 11:43 ` Michael S. Tsirkin
@ 2017-06-02 15:35 ` Michael S. Tsirkin
1 sibling, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2017-06-02 15:35 UTC (permalink / raw)
To: Dan Carpenter; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-06-02 15:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-06 5:29 [bug report] virtio_net: rework mergeable buffer handling Dan Carpenter
2017-04-06 11:43 ` Michael S. Tsirkin
2017-06-02 15:35 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.