Page MenuHomeFreeBSD

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

Authored by nc 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 - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nc requested review of this revision.Jul 8 2020, 9:18 PM
donner added inline comments.
sys/net/route/nhop_ctl.c
388–390

I'd summarize the cases:

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

Thanks for the suggestion.

nc 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
388–390

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.

nc marked an inline comment as done.Jul 9 2020, 5:59 PM
sys/net/route/nhop_ctl.c
388–390

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;
}
sys/net/route/nhop_ctl.c
388–390

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 marked 2 inline comments as done.Jul 16 2020, 1:37 AM
nc added inline comments.
sys/net/route/nhop_ctl.c
388–390

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 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
388–390

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

sys/net/route/nhop_ctl.c
381

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

389

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?

nc 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.
nc 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
108

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 marked 3 inline comments as done.Jul 20 2020, 4:17 PM
nc added inline comments.
sys/net/route/nhop_ctl.c
108

Yes.

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

nc 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.
nc edited the summary of this revision. (Show Details)

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