Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

zebra: pbr hash cleanup #15556

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

zebra: pbr hash cleanup #15556

wants to merge 1 commit into from

Conversation

paulzlabn
Copy link
Contributor

@paulzlabn paulzlabn commented Mar 14, 2024 •
edited
Loading 

In the zebra PBR rule database, the hash was previously calculated
over many fields, including most of the filter and action values.

But a given rule should be identifiable by just:

1. the identity of its originating daemon (ZAPI socket FD); and
2. the 'unique' field in struct pbr_rule

However, pbrd assigns a 'unique' field value per map (which may be assigned to multiple interfaces, yielding multiple corresponding rules). We could possibly modify pbrd to generate a 'unique' field value per rule instance, but the simpler solution for the moment is to also hash on the interface name.

This commit changes the PBR hash function to use only the following parameters:

1. the identity of its originating daemon (ZAPI socket FD); and
2. the 'unique' field in struct pbr_rule
3. the interface name

The goal of the above change is to get a better handle on what parameters uniquely identify a rule, to (eventually) allow the assignment or parallel generation of a globally-unique identifier for each rule that can be used with a dataplane.

This commit also removes the inefficient pbr_rule_lookup_unique() code that iterated linearly over the entire rule database.

ISSUES:
I invite review comments particularly on these issues

  1. The claim that socket, unique, and interface are sufficient to uniquely identify a PBR rule globally
  2. I don't have a good idea of how to test these changes to ensure there isn't some unexpected dependency on the previous hashing scheme. Ideas welcome.

@frrbot frrbot bot added the zebra label Mar 14, 2024
@paulzlabn paulzlabn force-pushed the working/ziemba/zebra-pbr-hash branch from 9160d7b to 67b4242 Compare March 14, 2024 21:03
@ton31337
Copy link
Member

Is this still a WIP?

@paulzlabn
Copy link
Contributor Author

Is this still a WIP?

I added some discussion items in the description. I'd like at least donaldsharp and mjs to review, as well as anyone else who might have an interest. For some reason I am unable to add reviewers to this PR.

mjstapp
mjstapp reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had just a couple of questions

zebra/zebra_pbr.c Outdated

key = jhash_3words(rule->rule.filter.fwmark, rule->vrf_id,
rule->rule.filter.ip_proto, key);
key = jhash_2words(rule->rule.unique, rule->sock, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tuple of vrf+ifname is necessary, I think? small change, but will be safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have a given ifname in more than one vrf? (I thought no?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, in netns-based vrfs I believe it is possible, so we accomodate that ... in many places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added vrf_id back to the hash in this latest push

zebra/zebra_pbr.c Outdated

key = jhash_3words(rule->rule.filter.fwmark, rule->vrf_id,
rule->rule.filter.ip_proto, key);
key = jhash_2words(rule->rule.unique, rule->sock, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having some random init value for the 'key' would be nice - it's a good practice to follow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the same thing as a "seeded" hash? It looks like that is a defense against DDOS attacks so I'm guessing in this case an attacker would craft a set of PBR rules to slow down lookups?

In general it could be a good thing. Maybe better implemented in the hash library so all callers benefit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is just about having the various examples in the codebase stay aligned as much as possible. In general, we try to encourage some kind of initialization for the key, so it'd be good to do that here also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please help me understand - my eyeball estimate is that around a third of the jhash_ algorithms in FRR start with some arbitrary fixed seed, and the rest use 0. What does it accomplish? What's the basis for choosing a particular 32-bit seed?

I'm not finding either general information (via goog) or any comments in frr/docs/developer.

@mjstapp mjstapp requested a review from donaldsharp March 19, 2024 20:54
@mjstapp
Copy link
Contributor

mjstapp commented Mar 19, 2024

I've added Donald Sharp as a reviewer (with his permission, I think)
I think you can remove the "WIP" from the headline - it may be discouraging folks from taking a look.

Is this still a WIP?

I added some discussion items in the description. I'd like at least donaldsharp and mjs to review, as well as anyone else who might have an interest. For some reason I am unable to add reviewers to this PR.

@paulzlabn paulzlabn changed the title zebra: pbr hash cleanup WIP zebra: pbr hash cleanup Mar 25, 2024
@paulzlabn
zebra: pbr hash cleanup WIP
d2e5e9a 
    In the zebra PBR rule database, the hash was previously calculated
    over many fields, including most of the filter and action values.

    But a given rule should be identifiable by just:

	1. the identity of its originating daemon (ZAPI socket FD); and
	2. the 'unique' field in struct pbr_rule

    However, pbrd assigns a 'unique' field value per map (which may
    be assigned to multiple interfaces, yielding multiple corresponding
    rules). We could possibly modify pbrd to generate a 'unique'
    field value per rule instance, but the simpler solution for the
    moment is to also hash on the interface name.

    This commit changes the PBR hash function to use only the
    following parameters:

	1. the identity of its originating daemon (ZAPI socket FD); and
	2. the 'unique' field in struct pbr_rule
	3. the VRF ID
	4. the interface name

    The goal of the above change is to get a better handle on what
    parameters uniquely identify a rule, to (eventually) allow the
    assignment or parallel generation of a globally-unique identifier
    for each rule that can be used with a dataplane.

    This commit also removes the inefficient pbr_rule_lookup_unique()
    code that iterated linearly over the entire rule database.

Signed-off-by: G. Paul Ziemba <[email protected]>
@paulzlabn paulzlabn force-pushed the working/ziemba/zebra-pbr-hash branch from 67b4242 to d2e5e9a Compare March 25, 2024 18:06
@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this PR closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjstapp mjstapp mjstapp left review comments

@donaldsharp donaldsharp Awaiting requested review from donaldsharp

Assignees
No one assigned
Labels
autoclose master size/L zebra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulzlabn @ton31337 @mjstapp