Opened 8 months ago

Closed 4 months ago

Last modified 4 months ago

#28636 closed defect (fixed)

Address WTF-PAD comments by Nick (2018-11-27 over IRC)

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.4.0.1-alpha
Severity: Normal Keywords: wtf-pad, tor-relay, tor-cell, padding, 041-proposed, network-team-roadmap-2019-Q1Q2
Cc: nickm Actual Points: 5
Parent ID: #28632 Points: 3
Reviewer: nickm Sponsor: Sponsor2

Description (last modified by asn)

Nick expressed the following concerns/comments about the branch:

  • Figure out how dormant mode interacts with the padding code.
    15:27 <+nickm> okay.  short version is that tor can enter a "dormant" state when it is inactive for a really long time, or when the controller tells it to do so.
    15:28 <+nickm> we should probably figure out whether being dormant prevents you from sendig padding and vice versa...
    15:28 <+nickm> ... and whether it should prevent you from sending padding (and vice versa)...
    
  • Do type checking when downcasting the vcarious probability distributions.
  • <+nickm> Also there are all these generic dist_ops functions that you could use ... but the API seems to encourage you not to use them. having wrapper functions instead that call dist->dist_ops.func(dist, ...) would be neat
  • <+nickm> oh, one list thing: I think src/lib/math is not allowed to include lib/crypt_ops, since that would introduce a circularity. Better make sure that it doesn't

Child Tickets

Change History (32)

comment:1 Changed 8 months ago by asn

Description: modified (diff)

comment:2 Changed 8 months ago by teor

Summary: Address comments by Nick (2018-11-27 over IRC)Address WTF-PAD comments by Nick (2018-11-27 over IRC)

comment:3 Changed 8 months ago by nickm

To be more clear: I'd be happy to help with some/all of these, and I am not 100% sure that they all need to go in before WTF-pad is merged if any of them will be very hard... though it should really go in before it's turned on by default.

comment:4 in reply to:  3 Changed 8 months ago by asn

Replying to nickm:

To be more clear: I'd be happy to help with some/all of these, and I am not 100% sure that they all need to go in before WTF-pad is merged if any of them will be very hard... though it should really go in before it's turned on by default.

Yep, agreed. That's why I made this ticket a child of #28632 and not of #28142.
Like you said, we MUST address the above before turning it on by default, but not necessarily before merging the big branch.

comment:5 Changed 8 months ago by asn

Some logs from Riastradh on how to fix the probdistr stuff:

 21:49 < Riastradh> nickm: You rightly identified that the intended way to get at functions like uniform_sample is via the dist_ops!  See bin_cdfs and bin_samples in test_prob_distr.c for an 
                    example of how to use them generically -- these (and the test_psi_dist_sample function) were why I built that abstraction.
 21:50 < Riastradh> nickm: I did not build wrapper functions because I just needed it for the tests and I didn't want to spend any more time than necessary architecting an abstraction without 
                    knowing whether it would bear fruit beyond its erstwhile solitary use case.
 21:52 < Riastradh> nickm: If you want to make broader use than the switch in circpad_distribution_sample, I would encourage a dist_sample(dist) { return dist->ops->sample(dist); } function, and 
                    dist_cdf(dist, x) { return dist->ops->cdf(dist, x); }, &c.
 21:58 < Riastradh> nickm: And if you did this, I would encorage hiding the member functions uniform_sample, uniform_cdf, &c., in prob_distr.c.  Indeed, you could do that right now in 
                    circpad_distribution_sample with no adverse effects.
 22:01 < Riastradh> nickm: As for potential circularities between crypto and math, it would be reasonable to separate the purely computational functions in prob_distr.c from the struct dist 
                    abstraction which uses crypto_rand_*; one could go in math and the other could go in crypto or something downstream of both math and crypto.
 22:01 < Riastradh> nickm: I didn't do that splitting up because I'm not familiar with the large-scale organization of the code base so I didn't want to commit to particular organizational 
                    suggestions that would turn out to be moot.
 22:03 < Riastradh> nickm: If you want to split up prob_distr.c, nothing _above_ `Public API for probability distributions' depends on anything below it, so that's the easy point.  
                    (sample_geometric should maybe go above that instead of below it.)

comment:6 Changed 6 months ago by asn

Description: modified (diff)

comment:7 Changed 6 months ago by asn

Milestone: Tor: 0.4.0.x-finalTor: unspecified

comment:8 Changed 6 months ago by mikeperry

Keywords: 041-proposed added

comment:9 Changed 6 months ago by nickm

Sponsor: Sponsor2

comment:10 Changed 6 months ago by mikeperry

So during one of Nick's code reviews, there was some discussion about renaming the circuit machine definitions and state structs to spec and state.. But we didn't pick good names for the machine state because it is confusing with the states of the machines.

I like either circuit_machine_mutable_t or circuit_machine_mutable_info_t or circuit_machine_runtime_t or circuit_machine_runtime_info_t rather than the circuit_machine_state_t we landed on from the review.

comment:11 Changed 5 months ago by asn

Points: 2

comment:12 Changed 5 months ago by mikeperry

Priority: MediumHigh

comment:13 Changed 5 months ago by asn

Points: 23

I will overload this ticket by adding another thing we should fix to improve code quality: We could use the probability distribution structs directly instead of the generic circpad_distribution abstraction, to reduce the possibility of messing up prob distr parameters like we did in #29527. Also see Riastradh's comment in https://trac.torproject.org/projects/tor/ticket/29527?replyto=13#comment:13.

comment:14 Changed 5 months ago by asn

OK I'm trying to map out what needs to happen for this ticket, because there is lots of info in here:

a) Need to see how circuitmapping should interact with the dormant subsystem. I think I can figure this out, maybe after speaking with Mike about the ideal behavior here.

b) Need to add type-checking to the downcasting macros in prob_distr.c . Some of it was done in d82a8a7f9d268728b2447b2dbbaa346140784f9b but I'm not sure if this is good enough. Might need to think of the right API here to come up with the best plan. Nick let me know if you have any ideas here, but please don't spend too much time, becaues I can probably figure it out too.

I was thinking of perhaps ditching the generic dist abstraction and only using the underlying functions (in a switch statement when needed (e.g. in the unittests)) so that we don't have type safety issues. Or otherwise adding some sort of magic field and introducing initialization functions for all the distributions, and then checking the magic field in the downcasting macro.

c) <+nickm> Also there are all these generic dist_ops functions that you could use ... but the API seems to encourage you not to use them. having wrapper functions instead that call dist->dist_ops.func(dist, ...) would be neat

I'm not sure what this comment calls for. Can you please tell me some more about it? Riastradh seems to expand on this on comment:5 but I cannot quite get it.

d) <+nickm> oh, one list thing: I think src/lib/math is not allowed to include lib/crypt_ops, since that would introduce a circularity. Better make sure that it doesn't

Is this circular dependency still a problem? i see that src/lib/math/ includes lib/crypt_ops but I don't see the other way around. Is this for future proofing this, or is there currently a problem? Also, would we be OK with the suggestion that Riastradh gives in comment:5 where we split prob_distr.c into the computational part, and the sampling part which calls crypto_rand()?

e) I should do the renaming that mike suggests in comment:10.

f) not sure if I will have time to do the API change that comment:13 recommends. Will see abou this.

comment:15 in reply to:  14 Changed 5 months ago by nickm

Replying to asn:

b) Need to add type-checking to the downcasting macros in prob_distr.c . Some of it was done in d82a8a7f9d268728b2447b2dbbaa346140784f9b but I'm not sure if this is good enough. Might need to think of the right API here to come up with the best plan. Nick let me know if you have any ideas here, but please don't spend too much time, becaues I can probably figure it out too.

I think that it's best to use inline functions for this kind of thing, so that you can assert. Something like this:

static inline dist_to_geometric(struct dist *obj)
{
  tor_assert(obj->ops == &geometric_ops);
  return SUBTYPE_P(obj, struct geometric, base));
}

You could use a macro to define a bunch of these, since they're all basically the same.

c) <+nickm> Also there are all these generic dist_ops functions that you could use ... but the API seems to encourage you not to use them. having wrapper functions instead that call dist->dist_ops.func(dist, ...) would be neat


I'm not sure what this comment calls for. Can you please tell me some more about it? Riastradh seems to expand on this on comment:5 but I cannot quite get it.

It looks like these functions already exist. They are dist_name, dist_sample, dist_cdf, and so on. They do need documentation, though.

Once those functions are documented, we should just use them everywhere outside of prob_distr.c. (I think we already do in most places?) I think we should make the definition of struct dist_ops hidden, so that other modules don't use it. We'd have to make the various _dist_ops extern constants only expose their pointers, but that's actually the only thing that the rest of the code uses.

d) <+nickm> oh, one list thing: I think src/lib/math is not allowed to include lib/crypt_ops, since that would introduce a circularity. Better make sure that it doesn't


Is this circular dependency still a problem? i see that src/lib/math/ includes lib/crypt_ops but I don't see the other way around. Is this for future proofing this, or is there currently a problem? Also, would we be OK with the suggestion that Riastradh gives in comment:5 where we split prob_distr.c into the computational part, and the sampling part which calls crypto_rand()?

I was wrong; lib_math is allowed to include lib/crypt_ops; see lib/math/.may_include. Current versions of check-includes will detect circularities, so you don't need to worry about this in the future.

Based on that, I think that splitting the code into sampling and computation would be elegant, but I wouldn't call it a super high priority.

For the randomness part, btw, it would be good to look into crypto_fast_rng() here. It's up to 100 times faster for short outputs, and this code is likely to get called a lot.

comment:16 Changed 4 months ago by gaba

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

comment:17 Changed 4 months ago by asn

Status: newneeds_review

OK I pushed branch bug28636: https://github.com/torproject/tor/pull/788

It does the following:

  • Stops padding when dormant mode is enabled. I'm not sure if this is a good idea, but I think it's fairly unlikely that there will be useful circuits to pad when dormant mode is enabled since it means that the client has been inactive for many many hours. So perhaps this does not matter so much, and I erred on the side of network health.
  • Implemented type-safe downcasting functions.
  • Documented the public API of prob distr.
  • Used crypto fast RNG instead of the old RNG.
  • Renamed circpad_machine_state_t to circpad_machine_runtime_t.

On the downside, I did not manage to hide the dist_ops structure as suggested by Nick in comment:15 because the messy GEOMETRIC() etc. init macros are using it in various places around the codebase and I couldn't get it to work. I was thinking of ditching those messy macros and making my own init functions (perhaps using a macro) but I couldn't figure out a pleasant interface for it that would look real good, so I decided to postpone this refactoring task since the above had already taken me the 2 points that were originally specified for this task, and we still have reviews to go.

comment:18 Changed 4 months ago by asn

Reviewer: nickm

comment:19 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

Hi! I left two comments. One should be easy to fix, but the one about dormant mode should be less so.

If the dormant-mode change is hard, then I'd suggest that you rebase the branch to include a fix for the RNG issue, and remove the dormant-mode change. We can merge the branch after that, and you can do the dormant-mode thing in a separate branch. Sound ok?

comment:20 in reply to:  19 Changed 4 months ago by asn

Status: needs_revisionneeds_review

Replying to nickm:

Hi! I left two comments. One should be easy to fix, but the one about dormant mode should be less so.

If the dormant-mode change is hard, then I'd suggest that you rebase the branch to include a fix for the RNG issue, and remove the dormant-mode change. We can merge the branch after that, and you can do the dormant-mode thing in a separate branch. Sound ok?

Thanks for the review.

I fixed the RNG issue, and added a fixup with a minimal fix for the dormant-mode (after discussion with Mike). I also opened #29821 for implementing circuit padding in dormant mode the proper way.

comment:21 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

The RNG patch is okay, but the patch to look at dormant mode isn't wrote. net_is_disabled() and net_is_completely_disbled() check for DisableNetwork and hibernation status. is_participating_on_network() is what we need here.

comment:22 Changed 4 months ago by asn

Status: needs_revisionneeds_review

Thanks for the help. Pushed fixup for that.

comment:23 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

ONe more change -- that test is backwards now.

comment:24 Changed 4 months ago by nickm

Status: needs_revisionmerge_ready

lgtm now

comment:25 Changed 4 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Status: merge_readyneeds_revision
Version: Tor: 0.4.0.1-alpha

Here are things that need revision:

  • Please fill in the actual points on this ticket.
  • This is a bug on code that was released in 0.4.0.1-alpha, so it needs a changes file.
    • Was any of this code released earlier than 0.4.0.1-alpha?
  • I did a review, I found at least one potential bug, and one typo:
  • I don't know enough to know if this code needs spec changes:
    • The only functional change I can see is padding is disabled in dormant mode.

Here are the things I did:

  • The CI passed on the original pull request.
  • The pull request is based on master, so I'm setting the milestone to 0.4.1 with no backport.
  • This branch has fixups, squashed to bug28636_squashed and made a pull request:
  • I reviewed the squashed version.

comment:26 Changed 4 months ago by teor

The squashed pull request failed due to #29693.

comment:27 in reply to:  25 Changed 4 months ago by asn

Status: needs_revisionneeds_review

Replying to teor:

Here are things that need revision:

  • Please fill in the actual points on this ticket.
  • This is a bug on code that was released in 0.4.0.1-alpha, so it needs a changes file.
    • Was any of this code released earlier than 0.4.0.1-alpha?
  • I did a review, I found at least one potential bug, and one typo:
  • I don't know enough to know if this code needs spec changes:
    • The only functional change I can see is padding is disabled in dormant mode.

Great catches. Thanks!

Pushed a commit 9fcea183d1 (in https://github.com/torproject/tor/pull/788) that should satisfy all the above. Let me know if you don't like it.

Last edited 4 months ago by asn (previous) (diff)

comment:28 Changed 4 months ago by asn

Status: needs_reviewmerge_ready

comment:29 Changed 4 months ago by teor

I squashed "Improvements based on Tim's review." into "circpad/prob_distr: Use crypto_fast_rng() instead of the old RNG."

I'm waiting for CI to pass, then I will merge to master.

Reminder: Please fill in the actual points on this ticket.

comment:30 Changed 4 months ago by teor

The branch in my repository failed on Appveyor due to #29706, which has already been merged to master. Rebuilding, to make sure that the failure does not happen all the time.

comment:31 Changed 4 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Ok, it passed. Merged to master.

comment:32 Changed 4 months ago by asn

Actual Points: 5
Note: See TracTickets for help on using tickets.