Opened 8 months ago

Closed 7 months ago

Last modified 7 months ago

#29204 closed defect (fixed)

Inspect circuit queue during padding decisions

Reported by: mikeperry Owned by:
Priority: Very High Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: wtf-pad, 041-proposed, network-team-roadmap-2019-Q1Q2, nickm-merge
Cc: Actual Points: 3
Parent ID: #28634 Points: 5
Reviewer: dgoulet Sponsor: Sponsor2

Description

We need to inspect the circuit queue or the channel outbuf in some way during padding decisions. The problem is that if a guard stops reading on a channel, and padding keeps getting scheduled by the middle, it will overflow the circuit queue and/or outbuf for the channel and eventually oom.

Ideally, we would make our padding decision based on the EWMA values for the circuit rather than just checking if there were queued cells in the outbuf, but at minimum we need some kind of throttling so we don't keep adding cells to an circuit queue or outbuf above a certain length.

We may also want to use the circuit queue activity to update our last packet sent timers..

Child Tickets

Attachments (1)

Tor-Cell-Flow.jpeg (147.4 KB) - added by mikeperry 8 months ago.

Download all attachments as: .zip

Change History (17)

Changed 8 months ago by mikeperry

Attachment: Tor-Cell-Flow.jpeg added

comment:1 Changed 7 months ago by gaba

Milestone: 2019Q1Q2

comment:2 Changed 7 months ago by gaba

Milestone: 2019Q1Q2Network Team 2019 Q1Q2

Milestone renamed

comment:3 Changed 7 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:4 Changed 7 months ago by gaba

Milestone: Network Team 2019 Q1Q2

comment:5 Changed 7 months ago by mikeperry

Ok I think the first thing to do is to make this be a simple fix that prevents us from triggering OOM conditions. The simplest way to do this is to inspect the circuitmux queue and if it is too full, just not send the padding cell, emit a log, and tell the padding system we did send a padding cell. I filed #29494 for optimizing this solution, which we may or may not need to do depending on the padding machines we make.

comment:6 Changed 7 months ago by asn

Parent ID: #28634

comment:7 Changed 7 months ago by mikeperry

Reviewer: dgoulet
Status: newneeds_review

https://github.com/torproject/tor/pull/719 - the simplest possible fix, to avoid OOM only.

comment:8 Changed 7 months ago by dgoulet

Status: needs_reviewneeds_revision

One single comment in the PR :).

comment:9 Changed 7 months ago by dgoulet

So after discussion with Mike on IRC, the approach in the patch is good. It is correct to only look at the circuit queue since circuit padding needs to pad on a per-circuit level.

Two things. First, I think this is an artifact that shouldn't be in the patch:

  mi->is_padding_timer_scheduled = 0;

Second, because this adds a new consensus param so we'll need a torspec.git patch before we release stable.

comment:10 Changed 7 months ago by mikeperry

Status: needs_revisionneeds_review

Ok. https://github.com/torproject/torspec/pull/62 for the spec. Updated https://github.com/torproject/tor/pull/719 to remove the flag update, which technically doesn't matter either way since the timer checks its own flag internally, and #28821 will unify that around the timer's flag.

comment:11 Changed 7 months ago by dgoulet

Cc: dgoulet removed
Keywords: nickm-merge added
Status: needs_reviewmerge_ready

Once CI passes, this lgtm! Spec as well.

comment:12 Changed 7 months ago by teor

Milestone: Tor: 0.4.1.x-final

I think this is an unreleased bug, and it belongs in master.

comment:13 Changed 7 months ago by teor

Summary: Inspect circuit queue during padidng decisionsInspect circuit queue during padding decisions

comment:14 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Squashed and merged to master.

comment:15 Changed 7 months ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final

Also merged to 0.4.0, since the bug is in fact there too.

comment:16 Changed 7 months ago by mikeperry

Actual Points: 3

(I spent maybe 2-3 points on this. Most of the time was actually spent on #29494)

Note: See TracTickets for help on using tickets.