Misc Coverity fixes in xnb(4)
ClosedPublic
Actions

Authored by asomers on Jan 18 2017, 9:51 PM.

Details

Reviewers

royger

Commits

rS319222: MFC r314148, r314150
rS316362: MFC r314148, r314150
rS314148: Misc Coverity fixes in xnb(4)

Summary

Misc Coverity fixes in xnb(4)

Most of these are null pointer dereferences or missing error checks in the
unit tests. One is a missing error check in xnb_attach_failed. None can
cause real problems in running systems.

Reported By: Coverity
CIDs: 1092469 1092468 1092467 2092466 1092465 1092512 1092511 1092510
CIDs: 1092510 1092509 1092508 1092507

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

asomers updated this revision to Diff 24172.Jan 18 2017, 9:51 PM

asomers retitled this revision from to Misc Coverity fixes in xnb(4).

asomers updated this object.

asomers edited the test plan for this revision. (Show Details)

asomers added a reviewer: royger.

Herald added a subscriber: imp. · View Herald TranscriptJan 18 2017, 9:51 PM

@royger, in addition to reviewing this change, can you please test it as well? I cannot, as I no longer have a Xen system.

Sorry to ask, but how should I test it?

sys/dev/xen/netback/netback.c
1104 ↗	(On Diff #24172)	Do you really need this voids here?

In D9234#200946, @royger wrote:

Sorry to ask, but how should I test it?

If you instantiate an xnb(4) interface, you should be able to do "sysctl dev.xnb.0.unit_test_results" which will run the builtin unit tests. However, you can't run them on a non-Xen system, which is why I haven't done it. That should be sufficient testing.

sys/dev/xen/netback/netback.c
1104 ↗	(On Diff #24172)	The `(void)`s tell Coverity that you don't care about the return value. If they're missing, then Coverity flags it as a missing error check.

This is the output I get:

xnb(xnb_ring2pkt:1536): Unknown extra info type 255. Discarding packet
xnb(xnb_dump_txreq:302): netif_tx_request index =0
xnb(xnb_dump_txreq:303): netif_tx_request.gref =0
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =8
xnb(xnb_dump_txreq:306): netif_tx_request.id =69
xnb(xnb_dump_txreq:307): netif_tx_request.size =1000
xnb(xnb_dump_txreq:302): netif_tx_request index =1
xnb(xnb_dump_txreq:303): netif_tx_request.gref =255
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =0
xnb(xnb_dump_txreq:306): netif_tx_request.id =0
xnb(xnb_dump_txreq:307): netif_tx_request.size =0
xnb(xnb_rxpkt2rsp:2070): Got error -1 for hypervisor gnttab_copy status
xnb(xnb_ring2pkt:1536): Unknown extra info type 255. Discarding packet
xnb(xnb_dump_txreq:302): netif_tx_request index =0
xnb(xnb_dump_txreq:303): netif_tx_request.gref =0
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =8
xnb(xnb_dump_txreq:306): netif_tx_request.id =69
xnb(xnb_dump_txreq:307): netif_tx_request.size =1000
xnb(xnb_dump_txreq:302): netif_tx_request index =1
xnb(xnb_dump_txreq:303): netif_tx_request.gref =255
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =0
xnb(xnb_dump_txreq:306): netif_tx_request.id =0
xnb(xnb_dump_txreq:307): netif_tx_request.size =0
xnb(xnb_rxpkt2rsp:2070): Got error -1 for hypervisor gnttab_copy status

I'm not familiar with this at all, but the code seems fine so unless you have any objections (or the output above is not as expected), I'm going to commit this.

Thanks!

This revision is now accepted and ready to land.Feb 23 2017, 2:53 PM

In D9234#201647, @royger wrote:

This is the output I get:

xnb(xnb_ring2pkt:1536): Unknown extra info type 255. Discarding packet
xnb(xnb_dump_txreq:302): netif_tx_request index =0
xnb(xnb_dump_txreq:303): netif_tx_request.gref =0
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =8
xnb(xnb_dump_txreq:306): netif_tx_request.id =69
xnb(xnb_dump_txreq:307): netif_tx_request.size =1000
xnb(xnb_dump_txreq:302): netif_tx_request index =1
xnb(xnb_dump_txreq:303): netif_tx_request.gref =255
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =0
xnb(xnb_dump_txreq:306): netif_tx_request.id =0
xnb(xnb_dump_txreq:307): netif_tx_request.size =0
xnb(xnb_rxpkt2rsp:2070): Got error -1 for hypervisor gnttab_copy status
xnb(xnb_ring2pkt:1536): Unknown extra info type 255. Discarding packet
xnb(xnb_dump_txreq:302): netif_tx_request index =0
xnb(xnb_dump_txreq:303): netif_tx_request.gref =0
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =8
xnb(xnb_dump_txreq:306): netif_tx_request.id =69
xnb(xnb_dump_txreq:307): netif_tx_request.size =1000
xnb(xnb_dump_txreq:302): netif_tx_request index =1
xnb(xnb_dump_txreq:303): netif_tx_request.gref =255
xnb(xnb_dump_txreq:304): netif_tx_request.offset=0
xnb(xnb_dump_txreq:305): netif_tx_request.flags =0
xnb(xnb_dump_txreq:306): netif_tx_request.id =0
xnb(xnb_dump_txreq:307): netif_tx_request.size =0
xnb(xnb_rxpkt2rsp:2070): Got error -1 for hypervisor gnttab_copy status

I'm not familiar with this at all, but the code seems fine so unless you have any objections (or the output above is not as expected), I'm going to commit this.

Thanks!

That looks like the dmesg output. There should've also been some output directly from the "sysctl" command. Did you see anything? The dmesg stuff is just warnings printed by the main xnb code due to errors that were injected by the tests.

Oh, right. This is the actual output, looks like 1 test failed:

dev.xnb.0.xenbus_peer_domid: 4
dev.xnb.0.xenbus_connection_state: Connected
dev.xnb.0.xenbus_dev_type: vif
dev.xnb.0.xenstore_path: backend/vif/4/0
dev.xnb.0.dump_rings:
	                                 TX                 RX
	        req_cons                  0              69349
	         nr_ents                256                256
	    rsp_prod_pvt                  0              69349
	           sring   fffff8015efc2000   fffff8015efc3000
	 sring->req_prod                  0              69605
	sring->req_event                  1              69606
	 sring->rsp_prod                  0              69349
	sring->rsp_event                  1              69350

dev.xnb.0.unit_test_results: xnb_sscanf_hhn:2531 Assertion Error: sscanf(mystr, "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" "202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f" "404142434445464748494a4b4c4d4e4f%hhn", &dest[4]) == 1
52 Tests Passed
1 Tests FAILED

In D9234#201663, @royger wrote:

Oh, right. This is the actual output, looks like 1 test failed:

dev.xnb.0.xenbus_peer_domid: 4
dev.xnb.0.xenbus_connection_state: Connected
dev.xnb.0.xenbus_dev_type: vif
dev.xnb.0.xenstore_path: backend/vif/4/0
dev.xnb.0.dump_rings:
	                                 TX                 RX
	        req_cons                  0              69349
	         nr_ents                256                256
	    rsp_prod_pvt                  0              69349
	           sring   fffff8015efc2000   fffff8015efc3000
	 sring->req_prod                  0              69605
	sring->req_event                  1              69606
	 sring->rsp_prod                  0              69349
	sring->rsp_event                  1              69350

dev.xnb.0.unit_test_results: xnb_sscanf_hhn:2531 Assertion Error: sscanf(mystr, "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f" "202122232425262728292a2b2c2d2e2f303132333435363738393a3b3c3d3e3f" "404142434445464748494a4b4c4d4e4f%hhn", &dest[4]) == 1
52 Tests Passed
1 Tests FAILED

I see what the problem is. This test is expecting a bug in the kernel's sscanf implementation that was present circa 2012. To all appearances, it still is. I'm not sure why this test fails now. But I'll fix it separately.

Closed by commit rS314148: Misc Coverity fixes in xnb(4) (authored by asomers). · Explain WhyFeb 23 2017, 4:31 PM

This revision was automatically updated to reflect the committed changes.