Page MenuHomeFreeBSD

Missing NULL checks for tcp_lro with compressed ack's
AbandonedPublic

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
Unit Tests Skipped

Event Timeline

rrs requested review of this revision.Apr 1 2021, 10:19 AM
This revision is now accepted and ready to land.Apr 1 2021, 10:23 AM

Updates per Han's comments on timestamps

This revision now requires review to proceed.Apr 1 2021, 12:49 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 ??

This revision is now accepted and ready to land.Apr 1 2021, 2:41 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.

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
sys/netinet/tcp_lro.c
802

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

1580
-> && (ditto)
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 ;)

sys/netinet/tcp_lro.c
802

Ahh wait it was looking for the opposite before.. duh :)

This revision is now accepted and ready to land.Apr 5 2021, 10:37 AM
sys/netinet/tcp_lro.c
802–808

You can remove the outer "l && " check. It is now superfluous. You already compare "l" with TCPOLEN_TSTAMP_APPA.

1579

ditto: "hdr_len && " is superfluous.