Address WTF-PAD comments by Nick (2018-11-27 over IRC)
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
Activity
changed milestone to %Tor: 0.4.1.x-final
added 041-proposed actualpoints::5 component::core tor/tor milestone::Tor: 0.4.1.x-final network-team-roadmap-2019-Q1Q2 padding parent::28632 points::3 priority::high resolution::fixed reviewer::nickm severity::normal sponsor::2 status::closed tor-cell tor-relay type::defect version::tor 0.4.0.1-alpha wtf-pad labels
Trac:
Description: Nick expressed the following concerns/comments about the branch:- We should be careful about using
monotime_*functions in the branch because of performance issues:
15:19 <+nickm> ... it is too slow for a fast path. 15:19 <+nickm> and lots of the ways to use it can be too slow for a fast path 15:21 <+nickm> hrm. So there are two things that can make it slow. 15:22 <+nickm> there are two things that can make this function slow 15:22 <+nickm> first, it tries to get the most precise monotonic time, not the fastest one 15:22 <+nickm> so it might have to go to the kernel and do a context switch and take some time there 15:22 <+nickm> the "coarse" variants of monotime_* don't have that problem. 15:23 <+nickm> The second issue is that using the "usec" variant requires 64-bit division on some platforms, which is noticeably expensive on 32-bit hardware.- 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
to
Nick expressed the following concerns/comments about the branch:
- We should be careful about using
monotime_*functions in the branch because of performance issues:
15:19 <+nickm> ... it is too slow for a fast path. 15:19 <+nickm> and lots of the ways to use it can be too slow for a fast path 15:21 <+nickm> hrm. So there are two things that can make it slow. 15:22 <+nickm> there are two things that can make this function slow 15:22 <+nickm> first, it tries to get the most precise monotonic time, not the fastest one 15:22 <+nickm> so it might have to go to the kernel and do a context switch and take some time there 15:22 <+nickm> the "coarse" variants of monotime_* don't have that problem. 15:23 <+nickm> The second issue is that using the "usec" variant requires 64-bit division on some platforms, which is noticeably expensive on 32-bit hardware.- 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
- We should be careful about using
-
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 (moved) and not of #28142 (moved). Like you said, we MUST address the above before turning it on by default, but not necessarily before merging the big branch.
-
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.) -
Trac:
Description: Nick expressed the following concerns/comments about the branch:- We should be careful about using
monotime_*functions in the branch because of performance issues:
15:19 <+nickm> ... it is too slow for a fast path. 15:19 <+nickm> and lots of the ways to use it can be too slow for a fast path 15:21 <+nickm> hrm. So there are two things that can make it slow. 15:22 <+nickm> there are two things that can make this function slow 15:22 <+nickm> first, it tries to get the most precise monotonic time, not the fastest one 15:22 <+nickm> so it might have to go to the kernel and do a context switch and take some time there 15:22 <+nickm> the "coarse" variants of monotime_* don't have that problem. 15:23 <+nickm> The second issue is that using the "usec" variant requires 64-bit division on some platforms, which is noticeably expensive on 32-bit hardware.- 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
to
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
- We should be careful about using
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.
-
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_distributionabstraction, to reduce the possibility of messing up prob distr parameters like we did in #29527 (moved). Also see Riastradh's comment in https://trac.torproject.org/projects/tor/ticket/29527?replyto=13#comment:13.Trac:
Points: 2 to 3 -
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
distabstraction 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 neatI'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'tIs this circular dependency still a problem? i see that
src/lib/math/includeslib/crypt_opsbut 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 callscrypto_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.
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 neatI'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_opsextern 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'tIs this circular dependency still a problem? i see that
src/lib/math/includeslib/crypt_opsbut 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 callscrypto_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.
-
OK I pushed branch
bug28636: https://github.com/torproject/tor/pull/788It 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_ttocircpad_machine_runtime_t.
On the downside, I did not manage to hide the
dist_opsstructure as suggested by Nick in comment:15 because the messyGEOMETRIC()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.Trac:
Status: new to needs_review -
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?
Trac:
Status: needs_review to needs_revision -
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 (moved) for implementing circuit padding in dormant mode the proper way.
Trac:
Status: needs_revision to needs_review 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.
Trac:
Milestone: Tor: unspecified to Tor: 0.4.1.x-final
Version: N/A to Tor: 0.4.0.1-alpha
Status: merge_ready to needs_revisionThe squashed pull request failed due to #29693 (moved).
-
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.
Trac:
Status: needs_revision to needs_review The branch in my repository failed on Appveyor due to #29706 (moved), which has already been merged to master. Rebuilding, to make sure that the failure does not happen all the time.
mentioned in issue #29527 (moved)
mentioned in issue #29693 (moved)
mentioned in issue #29821 (moved)
moved to tpo/core/tor#28636 (closed)
mentioned in issue tpo/core/tor#29821 (closed)
