Bug 195883 - Incorrect handling of errors in tcp_output() function
Summary: Incorrect handling of errors in tcp_output() function
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Hans Petter Selasky
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-12-11 08:45 UTC by Hans Petter Selasky
Modified: 2016-12-22 18:17 UTC (History)
5 users (show)

See Also:

Attachments
Suggested patch (1014 bytes, patch)
2014-12-11 08:45 UTC, Hans Petter Selasky 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Petter Selasky freebsd_committer freebsd_triage 2014-12-11 08:45:18 UTC 
Created attachment 150466 [details]
Suggested patch

Hi,

In the tcp_output() function, nothing is protecting TCP timers from elapsing during its execution. Under certain conditions a TCP timeout can overlap if_transmit() and iff the if_transmit() function returns non-zero in that case, the following repeated check will give a different outcome than previously:

((tp->t_flags & TF_FORCEDATA) == 0 ||
!tcp_timer_active(tp, TT_PERSIST))

This leads up to a panic where the inpcb transmit state gets corrupted.

Suggested solution:
Store outcome from previous check and use that result later on.

--HPS
Comment 1 Gleb Smirnoff freebsd_committer freebsd_triage 2014-12-11 09:02:57 UTC 
I'd prefer to store the value it a local variable, instead of mixing it into flags.
Comment 2 Hans Petter Selasky freebsd_committer freebsd_triage 2014-12-11 09:14:56 UTC 
Currently only the first 8 bits of the flags are used, and flags has "int" type, so it shouldn't be a problem. Also it allows us to optimise the flags check in the error case.

--HPS
Comment 3 Hans Petter Selasky freebsd_committer freebsd_triage 2014-12-14 11:02:11 UTC 
(In reply to Gleb Smirnoff from comment #1)
> I'd prefer to store the value it a local variable, instead of mixing it into
> flags.

Could you suggest a name for this new variable?

--HPS
Comment 4 Hiren Panchasara freebsd_committer freebsd_triage 2016-02-25 04:45:54 UTC 
Hans,

Do you want to pick a name and open a review for this, please?

Cheers,
Hiren
Comment 5 Hans Petter Selasky freebsd_committer freebsd_triage 2016-02-25 07:52:28 UTC 
Hi,

I'd like to re-examine the logic in the patch and see if it still applies.

I'll come back to this issue next week.

--HPS
Comment 6 Hans Petter Selasky freebsd_committer freebsd_triage 2016-12-22 18:17:35 UTC 
I believe Gleb Smirnoff has fixed these issues and is in control of the callout API usage in the TCP stack.