Opened 8 months ago

Closed 7 months ago

#24914 closed defect (implemented)

Maybe make relay_digest_matches() not use tor_malloc()

Reported by: dgoulet Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay
Cc: alex_y_xu@… Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

The relay_digest_matches() is used in relay_crypt() and it is called for a huge portion of cells that come through the relay.

Roughly speaking, if a relay is at 10MB/s and with cells of size 514 bytes, we are talking about a bit less than 20k cells *per* second meaning more than a million tor_malloc(20) per minute. This is the place:

  backup_digest = crypto_digest_dup(digest);

I think we should find a way to avoid this especially in the fast path of tor in order to avoid memory fragmentation as much as possible.

We could either use the stack, a static value or memarea subsystem.

Child Tickets

Change History (13)

comment:1 Changed 8 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Owner: set to nickm
Status: newaccepted

comment:2 Changed 8 months ago by nickm

Status: acceptedneeds_review

Branch bug24914 has a simple patch for this.

comment:3 Changed 8 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

Why not use crypto_digest_assign() and crypto_digest_t in the new checkpoint data structure (inline)?

Asking because crypto_digest_checkpoint() and crypto_digest_restore() really looks like the assign function.

Also extra whitespaces:

+crypto_digest_checkpoint(crypto_digest_checkpoint_t *checkpoint,
+                              const crypto_digest_t *digest)

comment:4 Changed 8 months ago by Hello71

also, this is the only non-test caller of crypto_digest_assign, so it seems that we could obsolete that.

also, I'm suspicious of the memset(0). I don't totally understand, but either this is security-critical, and we should use memwipe, or it isn't, and don't bother zeroing it at all. I suspect gcc will optimize this memset out anyways.

comment:5 Changed 8 months ago by Hello71

Cc: alex_y_xu@… added

comment:6 Changed 8 months ago by nickm

The rationale for not using crypto_digest_assign() here is that crypto_digest_t is opaque, so it can't be stack-allocated. Does that seem plausbile?

I agree that the memset should be memwipe.

comment:7 in reply to:  6 Changed 8 months ago by dgoulet

Replying to nickm:

The rationale for not using crypto_digest_assign() here is that crypto_digest_t is opaque, so it can't be stack-allocated. Does that seem plausbile?

I agree that the memset should be memwipe.

Ah indeed. Ok makes sense for both. I'm not sure we need to zero it here also but maybe it is security related so no strong opinion.

comment:8 Changed 8 months ago by nickm

Status: needs_revisionneeds_review

Yeah: I don't see an attack from not zeroing it, but I think it's generally good to zero crypto stuff reflexively.

I've added a couple of fixup commits; better now?

comment:9 Changed 8 months ago by dgoulet

lgtm;

comment:10 Changed 8 months ago by nickm

Status: needs_reviewmerge_ready

Okay. Squashed it, and holding it in merge_ready till the 0.3.4 merge window opens.

comment:11 Changed 8 months ago by teor

Do you think we should backport this change?
Or are CPU usage / RAM fragmentation bugs not severe enough to backport?

comment:12 Changed 8 months ago by nickm

I think this is something we should plan for "no backport" on: We've done this allocation in every version of Tor that's had the current relay crypto algorithm, ever. Unless we found that it had a massive performance impact, I think it's not a backport candidate.

comment:13 Changed 7 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merging to master (which is now 0.3.4).

Note: See TracTickets for help on using tickets.