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

[nc]

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>

[donner]

sys/net/route/nhop_ctl.c

389–391

I'd summarize the cases:

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

[nc]

Thanks for the suggestion.

[rgrimes]

sys/net/route/nhop_ctl.c

389–391

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

[nc]

Good catch, reverting to my old diff.

[donner]

sys/net/route/nhop_ctl.c

389–391

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]

sys/net/route/nhop_ctl.c

389–391

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.

[nc]

sys/net/route/nhop_ctl.c

389–391

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

[nc]

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.

[donner]

sys/net/route/nhop_ctl.c

389–391

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]

sys/net/route/nhop_ctl.c

382

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

390

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))

[melifaro]

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

[nc]

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.

[donner]

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.

[nc]

sys/net/route/nhop_ctl.c

109

Yes.

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

[nc]

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