Page MenuHomeFreeBSD

Make SIFTR work again
ClosedPublic

Authored by rscheff on Jan 18 2019, 10:57 AM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rscheff set the repository for this revision to rS FreeBSD src repository - subversion.
rscheff added a subscriber: transport.
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 marked an inline comment as not done.Jan 18 2019, 2:38 PM
rscheff added inline comments.
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 marked an inline comment as not done.
  • No need to check for uint32 smaller than zero
imp requested changes to this revision.Jan 18 2019, 5:30 PM
imp added inline comments.
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.

This revision now requires changes to proceed.Jan 18 2019, 5:30 PM

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 marked 2 inline comments as done.
  • 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 added inline comments.
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?

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

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.

In D18885#403442, @rscheff_gmx.at wrote:

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

That would be ideal.

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

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

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

This revision was not accepted when it landed; it landed in state Needs Review.Jan 18 2019, 9:46 PM
This revision was automatically updated to reflect the committed changes.