Missing NULL checks for tcp_lro with compressed ack's
AbandonedPublic
Actions

Authored by rrs on Apr 1 2021, 10:19 AM.

Details

Reviewers

• hselasky
gallatin
tuexen
rrs

Group Reviewers

transport

Summary

So far no code uses compressed acks (that code will be coming in shortly in rack), but Hans found
an interesting bug.. we failed to check for null in two cases when we do a m_gethdr. The proper
thing is to check and if null is returned don't set the flag on it :)

The fall through takes care of the missing mbuf since the ack is still good it just
links it in.

Test Plan

compile and test it with the rack stack that uses compressed acks on a machine
with a shortage of mbufs.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

rrs created this revision.Apr 1 2021, 10:19 AM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptApr 1 2021, 10:19 AM

Herald added a subscriber: melifaro. · View Herald Transcript

rrs requested review of this revision.Apr 1 2021, 10:19 AM

tuexen accepted this revision.Apr 1 2021, 10:23 AM

This revision is now accepted and ready to land.Apr 1 2021, 10:23 AM

Looks good.

Updates per Han's comments on timestamps

This revision now requires review to proceed.Apr 1 2021, 12:49 PM

• hselasky added inline comments.Apr 1 2021, 12:59 PM

sys/netinet/tcp_lro.c
1604	Setting 1 in both cases here??
1926	if "have_tstmp_opt" you don't need to check hdr_len != 0 ??

gallatin accepted this revision.Apr 1 2021, 2:41 PM

This revision is now accepted and ready to land.Apr 1 2021, 2:41 PM

• hselasky added inline comments.Apr 1 2021, 2:58 PM

sys/netinet/tcp_lro.c
1604	The hdr_len value is propagated into some other functions which also only checks for non-zero value. Maybe it is better to set hdr_len to zero.

rrs added inline comments.Apr 2 2021, 11:45 AM

sys/netinet/tcp_lro.c
1604	to 1 in both cases is a typo ;)
1604	hdr_len is used other places so I dislike changing it to 0 since you clearly do have a hdr in those cases. Better to just get the flag settings right "have or have not timestamp" and go from that
1926	Good point.

Address all of Hans comments so far in both the review and email

This revision now requires review to proceed.Apr 2 2021, 11:46 AM

• hselasky added inline comments.Apr 2 2021, 12:02 PM

sys/netinet/tcp_lro.c

802

should be && here once you changed == to != :-)

1580

-> && (ditto)

rrs added inline comments.Apr 5 2021, 10:29 AM

sys/netinet/tcp_lro.c
802	Hmm yeah I wonder why it was not && before, thinking about it if we would have had a option that was the right length but not timestamp we could in theory have considered the option timestamp when it was not, guess there is no option that precisely equals the timestamp option ;)

rrs added inline comments.Apr 5 2021, 10:33 AM

sys/netinet/tcp_lro.c
802	Ahh wait it was looking for the opposite before.. duh :)

Fix the && issue

rrs accepted this revision.Apr 5 2021, 10:37 AM

This revision is now accepted and ready to land.Apr 5 2021, 10:37 AM

• hselasky added inline comments.Apr 5 2021, 11:29 AM

sys/netinet/tcp_lro.c
802	You can remove the outer "l && " check. It is now superfluous. You already compare "l" with TCPOLEN_TSTAMP_APPA.
1580	ditto: "hdr_len && " is superfluous.

rrs abandoned this revision.Apr 30 2021, 6:23 PM

Revision Contents
Changeset List

Path

Size

sys/

netinet/

tcp_lro.c

20 lines

Diff 86681

View Options

Missing NULL checks for tcp_lro with compressed ack'sAbandonedPublicActions