Page MenuHomeFreeBSD
Differential D45411 Diff 139222 sys/netinet/tcp_subr.c

Changeset View

Standalone View

View Options

sys/netinet/tcp_subr.c

Show First 20 Lines • Show All 431 Lines • ▼ Show 20 Linesfind_and_ref_tcp_functions(struct tcp_function_set *fs)
blk = find_tcp_functions_locked(fs); blk = find_tcp_functions_locked(fs);
if (blk) if (blk)
refcount_acquire(&blk->tfb_refcnt); refcount_acquire(&blk->tfb_refcnt);
rw_runlock(&tcp_function_lock); rw_runlock(&tcp_function_lock);
return(blk); return(blk);
} }
struct tcp_function_block * struct tcp_function_block *
find_and_ref_tcp_fb(struct tcp_function_block *blk) tcp_find_and_ref_fb(struct tcp_function_block *blk)
{ {
struct tcp_function_block *rblk; struct tcp_function_block *rblk;
rw_rlock(&tcp_function_lock); rw_rlock(&tcp_function_lock);
rblk = find_tcp_fb_locked(blk, NULL); rblk = find_tcp_fb_locked(blk, NULL);
jtlUnsubmitted
Done
Inline Actions

I know it is unrelated, but it seems like this just devolves to rblk = blk if the caller owns a reference on blk (guaranteeing it will be in the list). There might be room for future optimization here.

If we wanted, this is probably where we could check for TCP_FUNC_BEING_REMOVED while we hold the tcp_function_lock. But, I haven't checked all callers to make sure that's appropriate.

jtl: I know it is unrelated, but it seems like this just devolves to `rblk = blk` if the caller owns…
tuexenAuthorUnsubmitted
Done
Inline Actions

We can do that, but that does NOT protect the case, where the TCP_FUNC_BEING_REMOVED was not set inside this function, but gets marked as soon as the tcp_function_lock is released and before the called gets the return value. So where is the difference?

tuexen: We can do that, but that does NOT protect the case, where the `TCP_FUNC_BEING_REMOVED` was not…
tuexenAuthorUnsubmitted
Done
Inline Actions

I think the question is if we can assume or not that blk is already registered.

tuexen: I think the question is if we can assume or not that `blk` is already registered.
if (rblk) if (rblk)
refcount_acquire(&rblk->tfb_refcnt); refcount_acquire(&rblk->tfb_refcnt);
rw_runlock(&tcp_function_lock); rw_runlock(&tcp_function_lock);
return(rblk); return(rblk);
} }
/* Find a matching alias for the given tcp_function_block. */ /* Find a matching alias for the given tcp_function_block. */
int int
Show All 21 Lines if (found) {
fs->function_set_name[TCP_FUNCTION_NAME_LEN_MAX - 1] = '\0'; fs->function_set_name[TCP_FUNCTION_NAME_LEN_MAX - 1] = '\0';
} else { } else {
fs->function_set_name[0] = '\0'; fs->function_set_name[0] = '\0';
} }
rw_runlock(&tcp_function_lock); rw_runlock(&tcp_function_lock);
return (found); return (found);
} }
static struct tcp_function_block * struct tcp_function_block *
find_and_ref_tcp_default_fb(void) tcp_find_and_ref_default_fb(void)
{ {
struct tcp_function_block *rblk; struct tcp_function_block *rblk;
rw_rlock(&tcp_function_lock); rw_rlock(&tcp_function_lock);
rblk = V_tcp_func_set_ptr; rblk = V_tcp_func_set_ptr;
refcount_acquire(&rblk->tfb_refcnt); refcount_acquire(&rblk->tfb_refcnt);
rw_runlock(&tcp_function_lock); rw_runlock(&tcp_function_lock);
return (rblk); return (rblk);
Show All 12 Lines if (tp->t_fb->tfb_tcp_timer_stop_all != NULL)
tp->t_fb->tfb_tcp_timer_stop_all(tp); tp->t_fb->tfb_tcp_timer_stop_all(tp);
/* /*
* Now, we'll find a new function block to use. * Now, we'll find a new function block to use.
* Start by trying the current user-selected * Start by trying the current user-selected
* default, unless this stack is the user-selected * default, unless this stack is the user-selected
* default. * default.
*/ */
tfb = find_and_ref_tcp_default_fb(); tfb = tcp_find_and_ref_default_fb();
if (tfb == tp->t_fb) { if (tfb == tp->t_fb) {
refcount_release(&tfb->tfb_refcnt); refcount_release(&tfb->tfb_refcnt);
tfb = NULL; tfb = NULL;
} }
/* Does the stack accept this connection? */ /* Does the stack accept this connection? */
if (tfb != NULL && tfb->tfb_tcp_handoff_ok != NULL && if (tfb != NULL && tfb->tfb_tcp_handoff_ok != NULL &&
(*tfb->tfb_tcp_handoff_ok)(tp)) { (*tfb->tfb_tcp_handoff_ok)(tp)) {
refcount_release(&tfb->tfb_refcnt); refcount_release(&tfb->tfb_refcnt);
Show All 19 Lines if (tfb != NULL) {
*/ */
refcount_release(&tfb->tfb_refcnt); refcount_release(&tfb->tfb_refcnt);
} }
/* /*
* If that wasn't feasible, use the built-in default * If that wasn't feasible, use the built-in default
* stack which is not allowed to reject anyone. * stack which is not allowed to reject anyone.
*/ */
tfb = find_and_ref_tcp_fb(&tcp_def_funcblk); tfb = tcp_find_and_ref_fb(&tcp_def_funcblk);
if (tfb == NULL) { if (tfb == NULL) {
/* there always should be a default */ /* there always should be a default */
panic("Can't refer to tcp_def_funcblk"); panic("Can't refer to tcp_def_funcblk");
} }
if (tfb->tfb_tcp_handoff_ok != NULL) { if (tfb->tfb_tcp_handoff_ok != NULL) {
if ((*tfb->tfb_tcp_handoff_ok) (tp)) { if ((*tfb->tfb_tcp_handoff_ok) (tp)) {
/* The default stack cannot say no */ /* The default stack cannot say no */
panic("Default stack rejects a new session?"); panic("Default stack rejects a new session?");
▲ Show 20 Lines • Show All 1,615 Lines • ▼ Show 20 Lines#endif
if (lgb != NULL) if (lgb != NULL)
lgb->tlb_errno = output_ret; lgb->tlb_errno = output_ret;
} }
/* /*
* Create a new TCP control block, making an empty reassembly queue and hooking * Create a new TCP control block, making an empty reassembly queue and hooking
* it to the argument protocol control block. The `inp' parameter must have * it to the argument protocol control block. The `inp' parameter must have
* come from the zone allocator set up by tcpcbstor declaration. * come from the zone allocator set up by tcpcbstor declaration.
* The caller needs to provide a pointer to a TCP function block, for which
* it holds a reference count.
*/ */
struct tcpcb * struct tcpcb *
tcp_newtcpcb(struct inpcb *inp) tcp_newtcpcb(struct inpcb *inp, struct tcp_function_block *tfb)
{ {
struct tcpcb *tp = intotcpcb(inp); struct tcpcb *tp = intotcpcb(inp);
#ifdef INET6 #ifdef INET6
int isipv6 = (inp->inp_vflag & INP_IPV6) != 0; int isipv6 = (inp->inp_vflag & INP_IPV6) != 0;
#endif /* INET6 */ #endif /* INET6 */
/* /*
* Historically allocation was done with M_ZERO. There is a lot of * Historically allocation was done with M_ZERO. There is a lot of
* code that rely on that. For now take safe approach and zero whole * code that rely on that. For now take safe approach and zero whole
* tcpcb. This definitely can be optimized. * tcpcb. This definitely can be optimized.
*/ */
bzero(&tp->t_start_zero, t_zero_size); bzero(&tp->t_start_zero, t_zero_size);
/* Initialise cc_var struct for this tcpcb. */ /* Initialise cc_var struct for this tcpcb. */
tp->t_ccv.type = IPPROTO_TCP; tp->t_ccv.type = IPPROTO_TCP;
tp->t_ccv.ccvc.tcp = tp; tp->t_ccv.ccvc.tcp = tp;
rw_rlock(&tcp_function_lock); tp->t_fb = tfb;
jtlUnsubmitted
Done
Inline Actions

Why do we need to do find_tcp_fb_locked()? All it does is walk a list to return listening_tcb->t_fb. And, if it is in the process of being removed, we should catch that when TCP_FUNC_BEING_REMOVED is checked.

jtl: Why do we need to do find_tcp_fb_locked()? All it does is walk a list to return listening_tcb…
jtlUnsubmitted
Done
Inline Actions

As we discussed, I agree that this review's usage of find_tcp_fb_locked() is consistent with current usage elsewhere and any change should be part of a larger change to the way we find stack pointers.

jtl: As we discussed, I agree that this review's usage of find_tcp_fb_locked() is consistent with…
tuexenAuthorUnsubmitted
Done
Inline Actions

I looked into this today. One problem is in register_tcp_functions_as_names(). I registers a function block and in case of an error, it just removes some. It does NOT hold a lock in between... No TCP_FUNC_BEING_REMOVED being set...
I think we should make the adding and removing atomic. This means we need to do all allocations beforehand. I'll bring up a patch for review. Then we can cleanup the search I think.

tuexen: I looked into this today. One problem is in `register_tcp_functions_as_names()`. I registers a…
tuexenAuthorUnsubmitted
Done
Inline Actions

This is now fixed and we know that the TCP function block is registered.

tuexen: This is now fixed and we know that the TCP function block is registered.
tp->t_fb = V_tcp_func_set_ptr;
refcount_acquire(&tp->t_fb->tfb_refcnt);
rw_runlock(&tcp_function_lock);
/* /*
* Use the current system default CC algorithm. * Use the current system default CC algorithm.
*/ */
cc_attach(tp, CC_DEFAULT_ALGO()); cc_attach(tp, CC_DEFAULT_ALGO());
jtlUnsubmitted
Done
Inline Actions

Why not try the default stack? If TCP_FUNC_BEING_REMOVED is set, but the listening TCB was still using that stack, it seems likely this function is racing with the removal code which is trying to switch all the TCBs to the default stack.

jtl: Why not try the default stack? If TCP_FUNC_BEING_REMOVED is set, but the listening TCB was…
jtlUnsubmitted
Done
Inline Actions

As we discussed, this is a design decision. I personally would prefer that we use the default stack rather than fail. However, both choices will produce correct code.

jtl: As we discussed, this is a design decision. I personally would prefer that we use the default…
tuexenAuthorUnsubmitted
Done
Inline Actions

This was discussed in the FreeBSD transport VC. The consensus was to fail the call, if the TCP stack of the listener is to be removed instead of doing something automatically without knowing if this is the intention of the admin.

tuexen: This was discussed in the FreeBSD transport VC. The consensus was to fail the call, if the TCP…
if (CC_ALGO(tp)->cb_init != NULL) if (CC_ALGO(tp)->cb_init != NULL)
if (CC_ALGO(tp)->cb_init(&tp->t_ccv, NULL) > 0) { if (CC_ALGO(tp)->cb_init(&tp->t_ccv, NULL) > 0) {
cc_detach(tp); cc_detach(tp);
if (tp->t_fb->tfb_tcp_fb_fini)
(*tp->t_fb->tfb_tcp_fb_fini)(tp, 1);
refcount_release(&tp->t_fb->tfb_refcnt);
return (NULL); return (NULL);
} }
#ifdef TCP_HHOOK #ifdef TCP_HHOOK
if (khelp_init_osd(HELPER_CLASS_TCP, &tp->t_osd)) { if (khelp_init_osd(HELPER_CLASS_TCP, &tp->t_osd)) {
if (tp->t_fb->tfb_tcp_fb_fini)
(*tp->t_fb->tfb_tcp_fb_fini)(tp, 1);
refcount_release(&tp->t_fb->tfb_refcnt);
return (NULL); return (NULL);
} }
#endif #endif
TAILQ_INIT(&tp->t_segq); TAILQ_INIT(&tp->t_segq);
STAILQ_INIT(&tp->t_inqueue); STAILQ_INIT(&tp->t_inqueue);
tp->t_maxseg = tp->t_maxseg =
#ifdef INET6 #ifdef INET6
▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Lines
#endif #endif
#ifdef TCP_BLACKBOX #ifdef TCP_BLACKBOX
/* Initialize the per-TCPCB log data. */ /* Initialize the per-TCPCB log data. */
tcp_log_tcpcbinit(tp); tcp_log_tcpcbinit(tp);
#endif #endif
tp->t_pacing_rate = -1; tp->t_pacing_rate = -1;
if (tp->t_fb->tfb_tcp_fb_init) { if (tp->t_fb->tfb_tcp_fb_init) {
if ((*tp->t_fb->tfb_tcp_fb_init)(tp, &tp->t_fb_ptr)) { if ((*tp->t_fb->tfb_tcp_fb_init)(tp, &tp->t_fb_ptr)) {
refcount_release(&tp->t_fb->tfb_refcnt);
return (NULL); return (NULL);
} }
} }
#ifdef STATS #ifdef STATS
if (V_tcp_perconn_stats_enable == 1) if (V_tcp_perconn_stats_enable == 1)
tp->t_stats = stats_blob_alloc(V_tcp_perconn_stats_dflt_tpl, 0); tp->t_stats = stats_blob_alloc(V_tcp_perconn_stats_dflt_tpl, 0);
#endif #endif
if (V_tcp_do_lrd) if (V_tcp_do_lrd)
▲ Show 20 Lines • Show All 2,411 Lines • Show Last 20 Lines