Make SIFTR work again

[rscheff]

Summary

Correct the error introduced by D18443 in siftr.c 
Disable it only when it has been enabled.

Thanks to Cheng Cui for providing these fixes at

https://github.com/ccui1000/siftr/commits/stable/11/siftr.c

[cc]

sys/netinet/siftr.c

1219 ↗

(On Diff #52969)

You can submit the changes in this function in a separate patch.

1469 ↗

(On Diff #52969)

The "new" variable type is uint32_t.
This fixes the broke part. However, I don't know why the sysctl net.inet.siftr.enabled can still be assigned with a value larger than 2.

[rscheff]

sys/netinet/siftr.c

1465 ↗

(On Diff #52969)

Yes, no need to check for <0 here, the sysctl checks for positive values only apparently.

1469 ↗

(On Diff #52969)

the user could choose to set it to any arbitrary value between 0..(2^32-1); the alternative would be to normalize any non-zero value to SIFTR_ENABLE.

[rscheff]

• No need to check for uint32 smaller than zero

[imp]

sys/netinet/siftr.c

1219 ↗

(On Diff #52983)

Don't allocate 480-byte structures on the stack. That's a problem.

1234 ↗

(On Diff #52983)

this doesn't match any more. why make the change? The stack space used is problematic.

[brooks]

I agree with @imp that the stack change shouldn't be there. I'm not super happy about mixing new features with bug fixes.

sys/netinet/siftr.c

1443 ↗

(On Diff #52983)

There should almost certainly be an:

else
       error = EINVAL;

just in case someone messes up a future caller's implementation. Alternatively a KASSERT() at the top.

[rscheff]

• removing stack variable
• adding check to avoid potential memory leak
• adding error value if enabling or disabling is not possible, if trying the action twice in a row

[rscheff]

sys/netinet/siftr.c

1243 ↗

(On Diff #52983)

While we are at it, shouldn't there be a check to avoid potentially leaking memory by running twice over SIFTR_ENABLE?

[rscheff]

Shall I split off the port filter into a seperate Diff?

[brooks]

sys/netinet/siftr.c

1217 ↗

(On Diff #53002)

Looking again, both of these initializations are performed a few lines down and should not be done here.

[brooks]

In D18885#403442, @rscheff_gmx.at wrote:

Shall I split off the port filter into a seperate Diff?

That would be ideal.

[rscheff]

• remove port filter
• revert variable initialization (happening twice)

[brooks]

Looks good to me. I'd like to get one more +1 and then I'm happy to commit it (or see it committed).

[cc]

Looks good to me. If the summary can also be updated to represent the new context, that will be better. Thanks.