Page MenuHomeFreeBSD
Differential D18885

Make SIFTR work again
ClosedPublic
Actions

Authored by rscheff on Jan 18 2019, 10:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 4:17 PM
Unknown Object (File)
Tue, Dec 3, 7:33 PM
Unknown Object (File)
Nov 7 2024, 8:41 PM
Unknown Object (File)
Nov 7 2024, 12:03 PM
Unknown Object (File)
Nov 6 2024, 8:58 AM
Unknown Object (File)
Nov 1 2024, 10:37 AM
Unknown Object (File)
Nov 1 2024, 1:35 AM
Unknown Object (File)
Oct 9 2024, 12:05 PM
View All 70 Files
Subscribers
imp
transport

Details

Reviewers
cc
lstewart
brooks
imp
Commits
rS343273: MFC r343162:
rS343272: MFC r343162:
rS343162: Make SIFTR work again after r342125 (D18443).
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

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 22040
Build 21268: arc lint + arc unit

Event Timeline

rscheff created this revision.Jan 18 2019, 10:57 AM
Harbormaster completed remote builds in B22016: Diff 52969.Jan 18 2019, 10:57 AM
rscheff edited the summary of this revision. (Show Details)Jan 18 2019, 11:01 AM
rscheff set the repository for this revision to rS FreeBSD src repository - subversion.
rscheff added a subscriber: transport.
Herald added a subscriber: imp. · View Herald TranscriptJan 18 2019, 11:01 AM
cc added inline comments.Jan 18 2019, 2:17 PM
sys/netinet/siftr.c
1206

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

1454

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
1451

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

1454

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 updated this revision to Diff 52983.Jan 18 2019, 2:52 PM
rscheff marked an inline comment as not done.
  • No need to check for uint32 smaller than zero
Harbormaster completed remote builds in B22022: Diff 52983.Jan 18 2019, 2:52 PM
Harbormaster completed remote builds in B22022: Diff 52983.
imp requested changes to this revision.Jan 18 2019, 5:30 PM
imp added inline comments.
sys/netinet/siftr.c
1206

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

1218

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
brooks added a comment.Jan 18 2019, 6:07 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
1427–1428

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 updated this revision to Diff 53002.Jan 18 2019, 6:43 PM
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
Harbormaster completed remote builds in B22037: Diff 53002.Jan 18 2019, 6:43 PM
rscheff marked an inline comment as done.Jan 18 2019, 6:43 PM
rscheff added inline comments.
sys/netinet/siftr.c
1227

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

rscheff added a comment.Jan 18 2019, 6:46 PM

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

brooks added inline comments.Jan 18 2019, 6:49 PM
sys/netinet/siftr.c
1204

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

brooks added a comment.Jan 18 2019, 6:50 PM
In D18885#403442, @rscheff_gmx.at wrote:

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

That would be ideal.

rscheff updated this revision to Diff 53006.Jan 18 2019, 7:18 PM
  • remove port filter
  • revert variable initialization (happening twice)
Harbormaster completed remote builds in B22040: Diff 53006.Jan 18 2019, 7:18 PM
rscheff marked an inline comment as done.Jan 18 2019, 7:32 PM
brooks accepted this revision.Jan 18 2019, 7:47 PM
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 accepted this revision.Jan 18 2019, 8:15 PM
Looks good to me. If the summary can also be updated to represent the new context, that will be better. Thanks.
rscheff edited the summary of this revision. (Show Details)Jan 18 2019, 9:02 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 18 2019, 9:46 PM
Closed by commit rS343162: Make SIFTR work again after r342125 (D18443). (authored by brooks).  ·  Explain Why
This revision was automatically updated to reflect the committed changes.
Revision Contents
Changeset List
PathSize
sys/
netinet/
17 lines
Diff 53006
View Options
sys/netinet/siftr.c
Loading...