Page MenuHomeFreeBSD

When modifying a route, only allow one of RTF_<BLACKHOLE,REJECT,GATEWAY> to be configured
Needs ReviewPublic

Authored by neel_neelc.org on Jul 8 2020, 9:18 PM.

Details

Reviewers
rgrimes
Summary

When modifying a route, only allow one of RTF_<BLACKHOLE,REJECT,GATEWAY> to be configured.

Submitted by: Neel Chauhan <neel AT neelc DOT org>

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

neel_neelc.org created this revision.Jul 8 2020, 9:18 PM
neel_neelc.org requested review of this revision.Jul 8 2020, 9:18 PM
lutz_donnerhacke.de added inline comments.
sys/net/route/nhop_ctl.c
396–398

I'd summarize the cases:

if ( (nh->nh_priv->rt_flags &
     (RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY)) != 0 )

Thanks for the suggestion.

neel_neelc.org marked an inline comment as done.Jul 9 2020, 4:11 PM
rgrimes requested changes to this revision.Jul 9 2020, 4:39 PM
rgrimes added a subscriber: rgrimes.
rgrimes added inline comments.
sys/net/route/nhop_ctl.c
396–398

AH, that looks like it only allows "None OF"

This revision now requires changes to proceed.Jul 9 2020, 4:39 PM

Good catch, reverting to my old diff.

neel_neelc.org marked an inline comment as done.Jul 9 2020, 5:59 PM
sys/net/route/nhop_ctl.c
396–398

There is no guarantee in the language, that the boolean result is converted to "1" for true. The only guarantee is, that the numerical value of false is "0".

How about:

switch (nh->nh_priv->rt_flags & (RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY)) {
   case RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY:
     ...
   default:
     break;
}
rgrimes added inline comments.Jul 10 2020, 4:16 PM
sys/net/route/nhop_ctl.c
396–398

Your first attempt would allow any combination of bits to be set, including only 1 of them and would lead to a return EINVAL. Do we agree on that?
It looks like this new one works, but i I suspect the generated code is not much better. It should be possible to do a function of the form I use a to simplify, the long form should all end up in register, this needs double checked I could have the if backwards.

a = nh->nh_priv->rt_flags & (RTF_BLACKHOLE | RTF_REJECT | RTF_GATEWAY)
if (a && !(a & (a-1)))
    return(EINVAL)

This is the power of 2 method to see if more than 1 bit is set, i believe it reduces this to 1 branch.

neel_neelc.org marked 2 inline comments as done.Jul 16 2020, 1:37 AM
neel_neelc.org added inline comments.
sys/net/route/nhop_ctl.c
396–398

Sorry for the delay.

I'm no expert in bitwise, but I think your logic is correct, just that you need:

if (a && (a & a - 1))

Will upload an updated patch.

Source: https://stackoverflow.com/questions/51094594/how-to-check-if-exactly-one-bit-is-set-in-an-int/51094793

neel_neelc.org marked an inline comment as done.

Here's the updated code, reduced to a single bitwise operator.

Sorry for the delay, I had a lot of things going on at $DAYJOB.

sys/net/route/nhop_ctl.c
396–398

The bit operation is very hard to read.
Such optimization should be done by the compiler only, unless there is a strong reason.

So I'd like to urge you to reconsider the "switch/case" approach instead

melifaro added inline comments.Jul 19 2020, 9:59 AM
sys/net/route/nhop_ctl.c
389

Could you please rename it to something more meaningful? flags or something like that?

397

Can you make it a macro, like in other parts of the kernel?

#define POWER_OF_2(n)   (!((n) & ((n)-1)))
#define IS_POW2(x) (x && ((x & (x - 1)) == 0))

Also, mind adding a test in tests/sys/route for that?

neel_neelc.org retitled this revision from In alter_nhop_from_info(), only allow one of RTF_<BLACKHOLE,REJECT,GATEWAY> to be configured to When adding or modifying a route, only allow one of RTF_<BLACKHOLE,REJECT,GATEWAY> to be configured.
neel_neelc.org edited the summary of this revision. (Show Details)

I added tests, they all pass.

The new behavior is that only one of RTF_<BLACKHOLE,REJECT,GATEWAY> can be configured when adding a route as well. Previously, this was only for modifying a route.

sys/net/route/nhop_ctl.c
109

Let's take for example "x = 4"

ONE_BIT_SET(4) = 4 && (0b100 & 0b011) = 4 && 0 = false

or "x = 5"

ONE_BIT_SET(5) = 5 && (0b101 & 0b100)  = 5 && 4 = true

Is this the intended behavior?
The name "ONE_BIT_SET" suggests otherwise.

neel_neelc.org marked 3 inline comments as done.Jul 20 2020, 4:17 PM
neel_neelc.org added inline comments.
sys/net/route/nhop_ctl.c
109

Yes.

However, "ONE_BIT_SET" should be "MULTI_BIT_SET". Patch upcoming.

neel_neelc.org retitled this revision from When adding or modifying a route, only allow one of RTF_<BLACKHOLE,REJECT,GATEWAY> to be configured to When modifying a route, only allow one of RTF_<BLACKHOLE,REJECT,GATEWAY> to be configured.
neel_neelc.org edited the summary of this revision. (Show Details)

With errors upon running rc.d, I realized that we should only do this when modifying.