General to-do list 1. Determine whether the man page or the code is correct. 2. Verify that all arguments are parsed (and subsequently processes). 3. Verify that correct error messages are produced as appropriate. Targeted to-do list 1. Man 5 ippool talks about a evolving configuration syntax. The original syntax was more verbose. Remove all the source code dealing with the original verbose syntax and correct ippool(5) manual content to match. Or if in your opinion the original verbose syntax is easier to comprehend, keep it and get rid of the new syntax. 2. ippool –R -m table-name command has error causing core to dump. No documentation about where to find the core dump. Is it the in core table contents being dumped or the running code that is dumped? Found /etc/ippool.core file, maybe that’s the core dump file. 3. The command “ippool -a –m table-name x.x.x.x used for adding a single node entry to the table works but “ippool –l –m table-name command does not show that just added ip address in the node list, but does show a ?(0)/32 node instead. If you try to add the node again you get message saying the node exists in the table already. Same thing occurs even if the ip address is suffixed with /32. 4. There is no documented way to dump the number of times a table ip address has been matched. The “man 8 ippool” lists the –d flag as a global option used for debugging the configuration file processing. Issuing “ippool -l -d –m table-name” displays all of the tables nodes with a hit count as a pair of 2 lines per node. This display needs to be condensed to a single line so it’s easier to parse through looking for the hits. 5. When the host system is shutdown or rebooted the ippool tables that were running are not restarted and the hit count is lost. 6. The “man 5 ippool” manual is mis-named. It should be named ippool.config. 7. In the “man 5 ippool” manual it gives this example “table roll=all type=hash name=servers size=5”. What does “size=5” mean? 8. The “man 5 ippool” manual doesn’t talk about the true syntax of the /etc/ippool.conf file. pool ipf/tree (name test;) { 1.121.136.228; 1.186.172.218; 1.34.169.204/32; 101.109.155.81/16; 104.121.89.129; }; Notice the position of the left and right { } Notice the usage of : I’m thinking the ending }; is an error, should be just } 9. The maximum table size is not documented any where and/or if its possible to increase it. 10. There is no documentation about the ip address being entered in a sorted order. Or about that ippool handles the placement of the entry in the in-core table allowing room for inserted new entry's while maintaining fast search performance. You don’t have to explain to people how this is accomplished, but you should tell them that it’s occurring automatically. Possible enchantments. 1. Would like to see an option that table entries get posted internally with some kind of auto expire date/time that automatically removes the entry when that timer elapses and the entry has no hits. If the entry gets a hit the timer resets and starts timer as of when the hit occurred. Maybe say 3 options, number of minutes, number of hours, and number of days. 2. The usage of the “ippool –R -m table-name” command is to remove the named table from running in core so it can be re-added in mass with updated content. I can all most do the same thing using this command sequence ippool -f /etc/ippool.conf -u this unloads all the entries but leaves the table name in place then this command reloads in mass ippool -f /etc/ippool.conf Would like to see the –u unload option have option to write a file containing all the entries with their hit counts and auto expire value.
Removed my personal email and assigned to myself (my FreeBSD email).
Created attachment 181554 [details] Temporary fix for this PR. I've attached a temporary patch. There are other parser issues that need to be fixed in ippool.c but for now this should get you going.
Having trouble applying the patch you provided. Did this to get the source svnlite co svn://svn.freebsd.org/base/releng/11.0 /usr/src Applied the patch to /usr/src/contrib/ipfilter/tools/ippool.c I see ippool.c size change after the patch is run. Then to just recompile ipfilter I cd /usr/src/sbin/ipf make clean all install Then ls -l /sbin/ippool shows the date and time of the module has changed but the size remains the same. Issued "ippool -R -m table-name" and gets same results as reported previously Looks like the patch is not being compiled. What am I doing wrong?
Created attachment 182179 [details] Candidate ippool patch. Sorry for the lateness of this. This patch should fix all the parsing issues in ippool.c and its man page.
I am still unable to recompile ippool. Please advise on the steps/commands to use.
Download and save the patch file. I'll use /tmp/ippool.diff in my example. Below is a Readers Digest version of patch and install instructions. cd /usr/src && patch -C -p0 < /tmp/ippool.diff && patch -p0 < /tmp/ippool.diff && cd sbin/ipf/ippool && make obj depend includes && make && make install
How do I download the patch from the PR?
Click on the link called Candidate ippool patch at the top of this PR. The link is --> https://bugs.freebsd.org/bugzilla/attachment.cgi?id=182179.
Got compile error. This is how I downloaded the source. svnlite co svn://svn.freebsd.org/base/releng/11.0 /usr/src /root >cd /usr/src/sbin/ipf/ippool /usr/src/sbin/ipf/ippool >make obj depend includes /usr/src/sbin/ipf/ippool >make make: /usr/src/sbin/ipf/ippool/.depend, 1: ignoring stale .depend for /usr/src/sbin/ipf/libipf/libipf.a cc -O2 -pipe -I. -I/usr/src/sbin/ipf/ippool/../../../contrib/ipfilter -I/usr/src/sbin/ipf/ippool/../../../contrib/ipfilter/tools -I/usr/src/sbin/ipf/ippool/../../../sys -I/usr/src/sbin/ipf/ippool/../../../sys/contrib/ipfilter -DSTATETOP -D__UIO_EXPOSE -DUSE_INET6 -g -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-format -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter -Wno-array-bounds -Qunused-arguments -o ippool.full ippool_y.o ippool_l.o kmem.o ippool.o -L/usr/obj/usr/src/sbin/ipf/libipf -lipf -lkvm /usr/bin/ld: cannot find -lipf cc: error: linker command failed with exit code 1 (use -v to see invocation) *** Error code 1 Stop. make: stopped in /usr/src/sbin/ipf/ippool /usr/src/sbin/ipf/ippool >
Follow the same instructions but from ippool's parent directory. Then cd to the ippool directory and make install from there.
Created attachment 182201 [details] complete compile op still having trouble with compile. Read my comments at end of the listing.
install(1) strips the binary of symbols, that's why it's smaller. You will see this by running the file command. file /sbin/ippool* /usr/obj See below: slippy$ ls -l /sbin/ippool /usr/obj/opt/src/svn-current/sbin/ipf/ippool/ippool -r-xr-xr-x 1 root wheel 108040 Apr 29 20:29 /sbin/ippool -rwxr-xr-x 1 root wheel 119040 Apr 29 15:35 /usr/obj/opt/src/svn-current/sbin/ipf/ippool/ippool slippy$ file /sbin/ippool /usr/obj/opt/src/svn-current/sbin/ipf/ippool/ippool /sbin/ippool: ELF 64-bit LSB executable, x86-64, version 1 (FreeBSD), dynamically linked, interpreter /libexec/ld-elf.so.1, for FreeBSD 12.0 (1200023), FreeBSD-style, stripped /usr/obj/opt/src/svn-current/sbin/ipf/ippool/ippool: ELF 64-bit LSB executable, x86-64, version 1 (FreeBSD), dynamically linked, interpreter /libexec/ld-elf.so.1, for FreeBSD 12.0 (1200023), FreeBSD-style, not stripped slippy$ The installed ippool is stripped. The one in /usr/obj is not. What does stripping do? It makes the binary smaller at the expense of the loss of any symbols that may help with debugging. See below. slippy$ nm /sbin/ippool nm: /sbin/ippool: no symbols slippy$ nm /usr/obj/opt/src/svn-current/sbin/ipf/ippool/ippool 000000000061ad40 B _CurrentRuneLocale@@FBSD_1.0 00000000006177b0 d _DYNAMIC 0000000000619c60 B _DefaultRuneLocale@@FBSD_1.0 0000000000617968 d _GLOBAL_OFFSET_TABLE_ w _Jv_RegisterClasses U _ThreadRuneLocale@@FBSD_1.3 0000000000617790 d __CTOR_END__ 0000000000617788 d __CTOR_LIST__ 00000000006177a0 d __DTOR_END__ 0000000000617798 d __DTOR_LIST__ 0000000000417780 r __FRAME_END__ 00000000006177a8 d __JCR_END__ 00000000006177a8 d __JCR_LIST__ 0000000000619c10 A __bss_start 000000000040d670 t __do_global_ctors_aux 0000000000401fd0 t __do_global_dtors_aux 0000000000617bc8 d __do_global_dtors_aux.completed 0000000000617bc0 d __do_global_dtors_aux.p 0000000000617bb8 D __dso_handle U __error@@FBSD_1.0 0000000000617784 d __fini_array_end 0000000000617784 d __fini_array_start U __inet_addr@@FBSD_1.0 U __inet_aton@@FBSD_1.0 U __inet_ntoa@@FBSD_1.0 U __inet_ntop@@FBSD_1.0 U __inet_pton@@FBSD_1.0 0000000000617784 d __init_array_end 0000000000617784 d __init_array_start 000000000061ace0 B __isthreaded@@FBSD_1.0 0000000000619c40 B __mb_sb_limit@@FBSD_1.0 0000000000617784 d __preinit_array_end 0000000000617784 d __preinit_array_start 0000000000617bb0 D __progname U __stack_chk_fail@@FBSD_1.0 000000000061ad00 B __stack_chk_guard@@FBSD_1.0 000000000061ad48 B __stderrp@@FBSD_1.0 0000000000619c38 B __stdinp@@FBSD_1.0 0000000000619c20 B __stdoutp@@FBSD_1.0 U __swbuf@@FBSD_1.0 0000000000619c10 A _edata 0000000000625e90 A _end 000000000040d6a8 T _fini 0000000000401940 T _init U _init_tls@@FBSD_1.0 0000000000401dc0 T _start 0000000000400218 r abitag U abort@@FBSD_1.0 0000000000403cc0 t add_htablehosts 000000000040abd0 T alist_free 000000000040a8f0 T alist_new 000000000040a890 T assigndefined U atexit@@FBSD_1.0 U atoi@@FBSD_1.0 U bcmp@@FBSD_1.0 U bcopy@@FBSD_1.0 000000000040c060 T bcopywrap U calloc@@FBSD_1.0 U close@@FBSD_1.0 U connect@@FBSD_1.0 000000000040a790 T connecttcp 000000000040d5d0 T count4bits 000000000040d530 T count6bits 0000000000400230 r crt_noinit_tag 000000000061bbf0 B environ U exit@@FBSD_1.0 000000000040ade0 t expand_string 000000000040d500 T familyname U fclose@@FBSD_1.0 0000000000617db8 D fd U feof@@FBSD_1.0 U fgetc@@FBSD_1.0 U fgets@@FBSD_1.0 000000000040cf90 T fill6bits 0000000000401f50 t finalizer 000000000040a740 T findword U fopen@@FBSD_1.0 U fprintf@@FBSD_1.0 0000000000402010 t frame_dummy U free@@FBSD_1.0 U freeaddrinfo@@FBSD_1.0 U fwrite@@FBSD_1.0 000000000040ac00 T get_variable U getaddrinfo@@FBSD_1.0 U getenv@@FBSD_1.0 000000000040bf10 T gethost U gethostbyname@@FBSD_1.0 U gethostname@@FBSD_1.0 U getnetbyname@@FBSD_1.0 U getopt@@FBSD_1.0 00000000004074b0 T getrole 00000000004074d0 T gettype 000000000040d650 T initparse U ioctl@@FBSD_1.0 0000000000617dd0 d ipf_errors 000000000040bd80 T ipf_geterror 000000000061bb50 b ipf_geterror.text 000000000040bb60 T ipf_perror 000000000040bbb0 T ipf_perror_fd 000000000040be70 T ipf_strerror 000000000061bba0 b ipf_strerror.text 000000000040bc80 T ipferror 000000000061ade8 b ipht 000000000061b9b0 b iphte 000000000061ba48 b ipld 000000000061ad88 b iplo 0000000000402040 T ippool_parsefile 0000000000402120 T ippool_parsesome 000000000061baf4 B ippool_yybreakondot 000000000061bc2c B ippool_yychar 0000000000623c90 B ippool_yychars 000000000040eb00 r ippool_yycheck 000000000061bc64 B ippool_yydebug 000000000040d9b0 r ippool_yydefred 000000000040fd90 r ippool_yydgoto 000000000061baec B ippool_yydictfixed 000000000061bc60 B ippool_yyerrflag 0000000000405a90 T ippool_yyerror 000000000061baf0 B ippool_yyexpectaddr 00000000004058b0 t ippool_yygetc 000000000040fce0 r ippool_yygindex 000000000061bc70 B ippool_yyin 0000000000405a20 T ippool_yykeytostr 0000000000617db4 D ippool_yylast 000000000040f9c0 r ippool_yylen 0000000000403dd0 T ippool_yylex 000000000061bb0c b ippool_yylex.prior 000000000061bb10 b ippool_yylex.priornum 000000000040fb50 r ippool_yylhs 0000000000617db0 D ippool_yylineNum 000000000061bc30 B ippool_yylval 000000000040dd00 r ippool_yyname 000000000061bc28 B ippool_yynerrs 00000000004021d0 T ippool_yyparse 000000000061bae8 B ippool_yypos 00000000004059b0 T ippool_yyresetdict 000000000040f360 r ippool_yyrindex 000000000040f6b0 r ippool_yyrule 000000000061bb08 B ippool_yysavedepth 0000000000625ca0 B ippool_yysavewords 0000000000405e30 T ippool_yysetdict 0000000000405d90 T ippool_yysetfixeddict 0000000000405a70 T ippool_yysettab 000000000040e7b0 r ippool_yysindex 000000000061ad58 b ippool_yystack 000000000061bae0 B ippool_yystr 000000000040ef30 r ippool_yytable 000000000061bc80 B ippool_yytext 000000000061bafc B ippool_yytokentype 000000000061bbf8 B ippool_yyval 000000000061baf8 B ippool_yyvarnext 0000000000617bd0 d ippool_yywords 000000000061bb00 B ippool_yywordtab 0000000000405f00 T kmemcpy 000000000040a720 T kmemcpywrap 0000000000405fe0 T kstrncpy 000000000061bb18 b kvm_f U kvm_open U kvm_read 000000000040a520 T load_dstlist 000000000040b9c0 T load_dstlistnode 000000000040a1a0 T load_file 0000000000409ec0 T load_hash 000000000040b810 T load_hashnode 0000000000409940 T load_http 0000000000409750 T load_pool 000000000040b650 T load_poolnode 00000000004096f0 T load_url 0000000000406a00 T loadpoolfile 00000000004061c0 T main U malloc@@FBSD_1.0 U memchr@@FBSD_1.0 U memcpy@@FBSD_1.0 U memmove@@FBSD_1.0 U memset@@FBSD_1.0 U nlist@@FBSD_1.0 000000000061bb30 B nohdrfields 000000000040cf20 T ntomask U open@@FBSD_1.0 0000000000405ec0 T openkmem 000000000061acf0 B optarg@@FBSD_1.0 0000000000619c28 B optind@@FBSD_1.0 0000000000619c30 B optreset@@FBSD_1.0 000000000061bb20 B opts 0000000000409580 T parsefields 00000000004092d0 T parsewhoisline U perror@@FBSD_1.0 000000000040cef0 T pool_close 000000000040cf10 T pool_fd 000000000061bb28 B pool_fields 000000000040ced0 T pool_ioctl 000000000040ce90 T pool_open 0000000000406680 T poolcommand 0000000000619b80 d poolfd 0000000000619b90 D poolfields 0000000000406b00 T poolflush 000000000061ad50 b poolioctl 0000000000406d50 T poollist 0000000000407db0 T poollist_dead 0000000000407560 T poollist_live 000000000061bad0 b poolname 00000000004062a0 T poolnodecommand 0000000000407000 T poolstats 000000000061bb40 B pos 0000000000409060 T printdstl_live 000000000040b4a0 T printdstlist 000000000040cd10 T printdstlistdata 000000000040ca70 T printdstlistnode 000000000040d4c0 T printdstlistpolicy U printf@@FBSD_1.0 000000000040b300 T printhash 0000000000408db0 T printhash_live 000000000040c680 T printhashdata 000000000040c440 T printhashnode 000000000040d3d0 T printip 000000000040d370 T printmask 000000000040b0c0 T printpool 0000000000408b40 T printpool_live 000000000040c210 T printpooldata 000000000040d090 T printpoolfield 000000000040c070 T printpoolnode 000000000040d020 T printunit U putc@@FBSD_1.0 U putchar@@FBSD_1.0 U puts@@FBSD_1.0 U read@@FBSD_1.0 U realloc@@FBSD_1.0 U reallocarray@@FBSD_1.4 0000000000408a20 T remove_hash 00000000004088a0 T remove_hashnode 0000000000408790 T remove_pool 0000000000408640 T remove_poolnode 0000000000408600 T resetlexer 000000000040afb0 T set_variable 0000000000407390 T setnodeaddr 00000000004084f0 T showdstls_live 00000000004083f0 T showhashs_live 00000000004082b0 T showpools_live U snprintf@@FBSD_1.0 U socket@@FBSD_1.0 U sprintf@@FBSD_1.0 U sscanf@@FBSD_1.0 U strcasecmp@@FBSD_1.0 U strcat@@FBSD_1.0 U strchr@@FBSD_1.0 U strcmp@@FBSD_1.0 U strcpy@@FBSD_1.0 U strdup@@FBSD_1.0 U strerror@@FBSD_1.0 0000000000617dc8 D string_end 0000000000617dc0 D string_start 000000000061bb38 B string_val U strlen@@FBSD_1.0 U strncmp@@FBSD_1.0 U strncpy@@FBSD_1.0 U strtok@@FBSD_1.0 0000000000625d90 B thishost U ungetc@@FBSD_1.0 00000000004060c0 T usage 000000000061bb24 B use_inet6 000000000061bb48 b vtop U warnx@@FBSD_1.0 U write@@FBSD_1.0 slippy$
Created attachment 182231 [details] text file listing problems Attachment file contains list of 9 outstanding problems that need addressing.
Created attachment 182323 [details] ippool patch V2 Problem 1: Can you post a dump, please. I'm not able to reproduce the dump on my -CURRENT systems here. Yes, only addresses (for now) can be entered. However there is code to use names (DNS or otherwise). I've reverted any changes that use this code until it can be sorted out. Problem 2: -t <type> is not optional, it's part of the key. This has been fixed. Problem 3: ippool -l [ -t <type> ] [ -o <role> ] [ -m <name> ] is more correct. However getopts() doesn't recognize options that the switch statement below it does. (Frankly, the parser in ippool is quite a mess.) For now use the above syntax with the patch I will attach. I won't commit it yet because there's a lot of work yet to do to clean this up. Problem 4: Once again, ippool is quite a mess. I've fixed a lot and a lot more needs fixing. There are a lot more problems you haven't identified yet. Problem 5: This has been fixed in my latest patch. Problem 6: ippool -a is totally broken. The problem is that it fails to recognize IPv4/IPv6 addresses. In doing so it does not set the address family (IPv4, IPv6) in in a structure. This is my current focus and will take quite a bit of work. (Sorry this is taking so long. Day $JOB not direclty related to FreeBSD at the moment.) Problem 7: This works in my latest patch now. -o & -t may be used. You can have the same named pools in different roles and different tables. This works now, though it doesn't work for entries added using the -a option becuase -a fails to assign an address family to entries. (Working on this.) Problem 8: I've yet to address this. Having seid that, I have replaced my fwbuilder built rules with ippool-based rules. It's currently working. Problem 9: This will need to be a long-term project for now. There is an IPv6 checksum issue in ipfilter that needs to be resolved before I tackle documentation. I can't recall the PR number but they have a patch for now while I clean it up for commit. (I'm not happy with my patch yet.) Final comments: Please try the attached patch. There are still a lot of problems and issues that I have yet to address and am still working on, not the least of which poolnodecommand() -- the -a option -- is totally useless because it fails to parse IP addresses correctly because it doesn't take into account IPv6 addresses. Furthermore I don't think anyone has ever tested ippool with IPv6 yet, not that I know of in FreeBSD, NetBSD, or Solaris. My goal here is to get enough of ippool working to go back and commit the IPv6 checksum issue in the kernel (PR 203275) before 11.1 and 10.4 are released. This is pretty much a show stopper for anyone using IPv6. Hope this helps. Please send me that core dump and uname -a with it. I'm specifically interested in the SVN revision number. Also, in the future, please reply in the PR instead of attaching a file. A person casually viewing this PR can kind-of lose context unless they clue into downloading a copy of attachment or viewing it in a separate browser tab. Thanks. ~cy
************* Ernie 5/1/17; Problem #1. "man 5 ippool" says only ip addresses can be used as table content. Now its not worded like that but this meaning is implied. No where does it say fqdn can be used. So reading "man 8 ippool" we see the "option" -R as in ippool -l [-dvR] [-m <name>] [-t <type>] Which is defined as "-R Do not resolve IP addresses to hostnames. This just doesn't make sense. I can not find am situation where any output to the terminal defaults to showing the hostnames. So why have this option at all? I would think this option can be removed from "man 8 ippool" all together. Cy 5/5/17 Yes, only addresses (for now) can be entered. However there is code to use names (DNS or otherwise). I've reverted any changes that use this code until it can be sorted out. Ernie 5/6/17 Verified the R option has been removed from the man 8 ippool. ***************** Ernie 5/1/17; Problem #2. ippool -R [-dnvR] [-m <name>] [-o <role>] [-t <type>] -R Remove an existing pool from within the kernel. Issuing "ippool -R -m table-name" results in the help syntax being shown. Issuing "ippool -R -m table-name -t tree" does remove the named table. A table name has to be unique across all table types. So requiring the -t flag in identifying the table wanted to remove is unneeded. The ippool source needs to be changed to remove this requirement. The "man 8 ippool" should show this as the syntax. ippool -R [-dnv] [-m <name>] Cy 5/5/17: -t <type> is not optional, it's part of the key. This has been fixed. Ernie 5/6/17 No this has not been fixed. -t <type> is still a requirement. It should not be required based on the fact table names are unique. Look at code logic for "ippool -l -m table-name" it finds the correct table just based on the table name. That same code needs to be incorporated into the -R option so the -t <type> is not required. ********************* Ernie 5/1/17; Problem #3. ippool -l [-dvR] [-m <name>] [-t <type>] Issuing "ippool -l" lists the contents of all the in-core tables. Issuing "ippool -l -m table-name" lists the contents of just the named table. Issuing "ippool -l -t tree" lists the contents of all the in-core tables. It looks like the [-t <type>] is not being used and logically makes no sense. The "man 8 ippool" should show this as the syntax. ippool -l [-dv] [-m <name>] Issuing this following command outputs a report containing the number of matches for each ip address in the named table. ippool -l -d -m test poollist: opts = 0x2 Name: test Role: ipf References: 2 Hits: 0 Nodes Starting at 0xca312d00 Address: 1.161.177.165/32 Hits 0 Bytes 0 Name Ref 2 Address: 1.171.138.231/32 Hits 0 Bytes 0 Name Ref 2 Address: 110.200.28.224/32 Hits 0 Bytes 0 Name Ref 2 Address: 110.200.29.248/32 Hits 0 Bytes 0 Name Ref 2 Address: 162.243.114.84/32 The -d flag is defined as " -d Toggle debugging of processing the configuration file." I don't see any debug info being generated here. What I do see is a valuable report. This should not be happening under the pretence of debugging. This report should have its own flag, like -h and the report should be changed to a single line containing the table ip address node plus number of hits. Cy 5/5/17: ippool -l [ -t <type> ] [ -o <role> ] [ -m <name> ] is more correct. However getopts() doesn't recognize options that the switch statement below it does. (Frankly, the parser in ippool is quite a mess.) For now use the above syntax with the patch I attached. I won't commit it yet because there's a lot of work yet to do to clean this up. Ernie 5/6/17 man 8 ippool shows this ippool -l [-dv] [-m <name>] [-t <type>] it should show this ippool -l [-dv] [-m <name>] ippool -l -t <type> Does list content of all in-core tables that match that table type. But since list is in wrap around mode its very hard to read. There is no logical reason for this option. Looks like an left over option for some future function that was never implemented. Remove all code from the -l option that has anything to do with -t or -o ippool -l list content of all in-core tables ippool -l [-m <name>] list content of named in-core table This keeps things simple. ********************* Ernie 5/1/17; Problem #4. ippool -F [-dv] [-o <role>] [-t <type>] -F Flush loaded pools from the kernel. Why is [-dv] missing the n option which means -n This flag (no-change) prevents ippool from actually making any ioctl calls or doing anything which would alter the currently running kernel. Reading this I take it to mean that issuing "ippool -F -o or -t" is saying to flush ALL tables matching the -o or -t value. This doesn't make sense. The -F option in the ippool source needs to be changed removing any logic for the -o & -t option processing and add -m logic to select the named table only. The -n option logic also needs to be added. The "man 8 ippool" should show this as the syntax. ippool -F [-dvn] [-m <name>] and the " -F Flush loaded pools from the kernel." changed to " -F Flush loaded pool content from the kernel, leaving only empty pool name." Cy 5/5/17: Once again, ippool is quite a mess. I've fixed a lot and a lot more needs fixing. There are a lot more problems that you haven't identified yet. Ernie 5/6/17 Lets focus on the documented problems. man 8 ippool shows this ippool -F [-dv] [-o <role>] [-t <type>] Issuing "ippool -F -m table-name" gets a option error msg and the same thing for "ippool -F -n" This change has not been done yet. The removal of the -o & -t flags from the source has not happened yet. The addition of the -m table-name flag to the source has not been done yet. These changes have not been made to "man 8 ippool". *************************** Ernie 5/1/17; Problem #5. ippool -A [-dnvR] [-m <name>] [-o <role>] [-S <seed>] [-t <type>] -A Add a new (empty) pool to the kernel. Issuing "ippool -A -m test2 -o ipf -t tree" results in ippool: illegal option -- A The -A option in the ippool source needs to be corrected. The -o and -t options are required Cy 5/5/17: This has been fixed in my latest patch. Ernie 5/6/17 Tested this issuing this command "ippool -A -m test1 -t tree" worked and "ippool -f /etc/ippool.conf.test1" to load the contents also worked "ippool -A -m test2 -t hash worked but issued this msg WARNING: empty hash table test3, recommend setting size to match expected use This would seem to indicate that a size flag should be required. "ippool -l -m test2" shows table role=ipf type=hash name=test2 size=1 {; }; "ippool -f /etc/ippool.conf.test2" gets this error. 30017:add lookup hash table could not find table to remove node from table This is defiantly bogus and mis-leading. The "ippool -A" command still needs work. ******************* Ernie 5/1/17; Problem #6. ippool -a [-dnvR] [-m <name>] [-o <role>] [-t <type>] [-T ttl] -i <ipaddr>[/<netmask>] -a Add a new data node to an existing pool in the kernel. The -o & -t options are not needed to identify the target table to add the node to. Issuing "ippool -a -m test 99.58.98.79" gives no errors, but issuing "ippool -l -m test" afterwards lists the content showing it containing ?(0)?/32; which is incorrect. This needs detail inspection of the ippool source to correct this. The "man 8 ippool" should show this as the syntax. ippool -a [-dnv] [-m <name>] [-T ttl] -i <ipaddr>[/<netmask>] Cy 5/5/17: ippool -a is totally broken. The problem is that it fails to recognize IPv4/IPv6 addresses. In doing so it does not set the address family (IPv4, IPv6) in a structure. This is my current focus and will take quite a bit of work. (Sorry this is taking so long. Day $JOB not directly related to FreeBSD at the moment.) Ernie 5/6/17 Lets focus on getting IPv4 working so the ?(0)?/32; problem can be addresses. Trying to do IPv6 in ippool now is diluting the effort to much. Lets make IPv6 a future ippool update, so we can get the basic flags to work correctly. There must be IPv4 code in the "ippool -f" command that is handling this correctly. That code can be copied or performed for this -a flag. ************************* Ernie 5/1/17; Problem #7. ippool -r [-dnvR] [-m <name>] [-o <role>] [-t <type>] -i <ipaddr>[/<netmask>] -r Remove an existing data node from a pool in the kernel. The -o & -t options are not needed to identify the target table to remove the node from. Issuing "ippool -r -m test 99.58.98.79" gives no errors, but issuing "ippool -l -m test" afterwards lists the content showing the ?(0)?/32; is no longer there. This needs detail inspection of the ippool source to correct this. The "man 8 ippool" should show this as the syntax. ippool -r [-dnv] [-m <name>] -i <ipaddr>[/<netmask>] Cy 5/5/17: This works in my latest patch now. -o & -t may be used. You can have the same named pools in different roles and different tables. This works now, though it doesn't work for entries added using the -a option because -a fails to assign an address family to entries. (Working on this.) Ernie 5/6/17 Having the same named pools in different roles and different tables should not be allowed. This breaks the unique table name idea we are working with. The -o & -t code needs to be removed. Tested by issuing "ippool -r -m test1 -i 106.75.81.110" worked, test1 is a tree type table and test2 was a hash type table and that worked also. ******************** Ernie 5/1/17; Problem #8. During the boot process these messages are issued IP Filter: v5.1.2 initialized. Default = pass all, Logging = enabled Enabling ipfilter. 132:194:ioctl(add/insert rule) cannot find source lookup pool Installing NAT rules. 0 entries flushed from NAT table 0 entries flushed from NAT list In my ipfilter rule set I have this rule; block in quick from pool/probing_ips to any Rebooting IE: reboot, halt, shutdown, commands or power off button; causes the table hit count to be lost and no table content to be reloaded into core. This really is a major problem. Have to add some kind of way to tell ipfilter to auto dump the in-core table with it's hit count information and auto restore that dumped information into core when the system is booted. Maybe adding an option flag to rc.conf ipfilter_flags="" statement defaulting to internally issuing "ippool -f /etc/ippool.conf" command or playing with the rc.d scripts is needed here. Whatever solution gets employed to solve this problem needs to be documented in "man 5 ippool" Cy 5/5/17: I've yet to address this. Having said that, I have replaced my fwbuilder built rules with ippool-based rules. It's currently working. Ernie 5/6/17 This is not totally true. I see in the current patch-v2 you have added some new defaults to etc/defaults/rc.conf file which are listed below; +ippool_enable="NO" # Set to YES to enable ip filter pools +ippool_program="/sbin/ippool" # where the ippool program lives +ippool_rules="/etc/ippool.rules" # pool definition file for ippool +ippool_flags="" # additional flags for ippool The statement ippool_rules="/etc/ippool.rules" is not correct. It should be ippool_rules="/etc/ippool.conf" because that is the default ippool configuration file documented in the man page. You also added a completely new file etc/rc.d/ippool file that is executed at boot time to load the contents of /etc/ippool.conf into the system core. This is a good first step, but does not address existing in-core table hit count being unloaded and saved to a file so it can be used to re-populate the in-core table at boot time. When the shutdown command is issued there has to be a process to save the in-core table info. This 2nd part can come later after all the flags processing get corrected. For whatever reason this patch has not propagated to /etc/rc.d/ippool and /etc/defaults/rc.conf file, so I manually moved /usr/src/etc/rc.d/ippool to /etc/rc.d/ippool and added ippool_enable="NO" ippool_program="/sbin/ippool" ippool_rules="/etc/ippool.conf" ippool_flags="" to my system /etc/defaults/rc.conf file and then added ippool_enable="YES" to my /etc/rc.conf file. I did a "shutdown -r now" command and during the boot I got there messages IP Filter: v5.1.2 initialized. Default = pass all, Logging = enabled Enabling ipfilter. 132:194:ioctl(add/insert rule) cannot find source lookup pool Installing NAT rules. 0 entries flushed from NAT table 0 entries flushed from NAT list After logging in I did "ippool -s" and there were no tables loaded in-core. A "ippool -f /etc/ippool.conf" command worked and "ippool -s" showed the table was loaded in-core. This is telling me that the /etc/ippool.conf file is good and that most likely /etc/rc.d/ippool was never executed at boot time. There were no script errors in the boot messages. This first part of Problem #8 still needs work. ********************* Ernie 5/6/17 Problem #9. "man 5 ippool" needs to specify the usage of { } and ; in the ippool.conf file content like shown here; pool ipf/tree (name test;) { 1.161.177.165; 1.171.138.231; 110.200.28.224; 110.200.29.248; 162.243.114.84; 162.243.225.157; 88.211.91.195; 88.226.134.147; 99.58.98.78; 99.98.160.77; }; Cy 5/5/17: This will need to be a long-term project for now. There is an IPv6 checksum issue in ipfilter that needs to be resolved before I tackle documentation. I can't recall the PR number but there is a patch for it that I have to clean up for I can commit it. (I'm not happy with my patch yet.) Ernie 5/6/17 This is only documentation to be added to "man 5 ippool". There is nothing wrong in /sbin/ippool that needs correction to solve this problem. ************* Ernie 5/6/17 Problem #10. man 8 ippool shows this ippool -s [-dtv] [-M <core>] [-N <namelist>] The [-dtv] should be [-dv] This is just a documentation change. ************ Your patch-v1 to ippool did correct the "ippool -R -m table_name" core dump problem. The patch-v2 includes this fix and continues to work. I think both you and I can agree on the fact that /sbin/ippool has never worked since day one. That it was written by some one else besides Darren Reed. That the coding style is a can of worms which looks more like a "concept proto-type" than a finished production product. I think the KISS programming method should by employed here. I can tell from the ippool.c patch that you are just adding new code around existing crap instead of deleting or at least commenting out crap logic code. It's time to take a wide brush to the source and remove all the "concept proto-type" code dealing with functions not implemented yet. Everything should be based off of the following requirements. 1. All tables have to have unique table names. 2. All code references to -t or -o flags should be removed. 3. All code that deals with converting fqdn to ip address should be removed from the source. 4. Any code that deals with processing input coming from the whois command should also be removed from the source. whois as input is talked about in "man 5.ippool" 5. Limit table handling to IPv4 address for now so we can get a working /sbin/ippool. Adding IPv6 can happen later after Ipv4 is a working. 6. Limit boot time table handling to just auto populating in-core tables with definition content from the /etc/ippool.conf file. Adding support for capturing the in-core table with its hits count at "shutdown" and the reloading that saved information back into the core at boot time can wait for phase 2 when IPv6 function is added. Now you are the man with his finger in the source. What I have out lined above is just suggestions. I respect whatever direction you choose to take. Take note, my use of ippool is not headed to production. I am using ippool to identify the offending IPv4 address that are probing my public ip address over time. I have a working system based on the few ipool commands that do function correctly. I am volunteering my time to identify and test the ippool command giving you feedback on what works and what doesn't. So other words take your time and fix IPv6 in ipfilter, I am in no rush here. As we get the basic set of command flags to function correctly using "tree & hash" table types, then I plan to test the other "role types" and table types until everything is working correctly.
A commit references this bug: Author: cy Date: Thu May 11 04:39:11 UTC 2017 New revision: 318173 URL: https://svnweb.freebsd.org/changeset/base/318173 Log: Implement outputting of IPv6 addresses in the ippool debug list of tree type pools (ippool -l -d -t tree). Currently IPv6 in ippool tree type pool handling is partially implemented (meaning it doesn't work). This is the first of a series of commits to remediate ippool. This will be MFCed with a yet to be committed series of fixes to ippool after it has been fully remediated. PR: 218433 Changes: head/contrib/ipfilter/lib/printpoolnode.c
Problem #2. The names are not unique. They are unique within a type. slippy# ippool -l -m test table role=ipf type=tree name=test { 10.1.1.91/32; fc00:1:1:1::5b/128; 10.1.2.91/32; 10.1.1.1/32; 10.1.2.1/32; 10.1.1.7/32; 10.1.2.7/32; 10.1.1.254/32; 10.1.2.254/32; }; table role=ipf type=hash name=test size=31 { 10.1.1.91/32; fc00:1:1:1::5b/128; 10.1.2.91/32; fc00:1:1:2::5b/128; 10.1.1.1/32; fc00:1:1:1::1/128; 10.1.2.1/32; fc00:1:1:2::1/128; 10.1.1.7/32; fc00:1:1:1::7/128; 10.1.2.7/32; fc00:1:1:2::7/128; 10.1.1.254/32; fc00:1:1:1::fffe/128; 10.1.2.254/32; fc00:1:1:2::fffe/128; 192.168.56.0/24; 10.2.2.0/24; }; slippy# slippy# ippool -l -t tree table role=ipf type=tree name=test3 { fc00:1:1:1::5b/128; }; table role=ipf type=tree name=test2 { 10.255.0.1/32; 10.255.0.2/32; }; table role=ipf type=tree name=test { 10.1.1.91/32; fc00:1:1:1::5b/128; 10.1.2.91/32; 10.1.1.1/32; 10.1.2.1/32; 10.1.1.7/32; 10.1.2.7/32; 10.1.1.254/32; 10.1.2.254/32; }; table role=ipf type=tree name=vitp { 10.255.0.1/32; 10.255.0.2/32; }; table role=ipf type=tree name=slippy { 10.1.1.91/32; 10.1.2.91/32; 10.1.3.91/32; }; table role=ipf type=tree name=trusted { 10.1.1.0/24; 10.1.2.0/24; 10.1.3.0/24; 10.2.2.1/32; 10.2.2.3/32; 10.2.2.4/32; 10.2.2.6/32; }; slippy# slippy# ippool -l -t hash table role=ipf type=hash name=test size=31 { 10.1.1.91/32; fc00:1:1:1::5b/128; 10.1.2.91/32; fc00:1:1:2::5b/128; 10.1.1.1/32; fc00:1:1:1::1/128; 10.1.2.1/32; fc00:1:1:2::1/128; 10.1.1.7/32; fc00:1:1:1::7/128; 10.1.2.7/32; fc00:1:1:2::7/128; 10.1.1.254/32; fc00:1:1:1::fffe/128; 10.1.2.254/32; fc00:1:1:2::fffe/128; 192.168.56.0/24; 10.2.2.0/24; }; slippy# Problem #3. -t can be either tree or hash. Problem #4. ippool -F -m is invalid. That is why it is not documented as such. You should use ippool -R -m -t instead. Problem #5. Agreed. My latest patch, that I'm still testing here, has fixed this. More testing is require before I can post it here. Problem #8. This is a long one. Hopefully I don't miss a point. Why would you want to reload the counters? That doesn't make sense. Saving the counters to a logfile or syslog is possible though that would be inconsistent with ipfw and pf in FreeBSD. I prefer a global solution that includes all three. (Which BTW, it is possible to run all three in the same kernel simultaneously.) ippool.rules is consistent with ipf.rules. This is a FreeBSD standard adopted a good number of years ago. Darren Reed, the author of ipfilter preferred ipf.conf, ipnat.conf and ippool.conf. When ipfilter was imported into FreeBSD about 20 years ago it was decided to use .rules instead. To change the default now would be a POLA violation. To change the man page might not be a good idea either as many old-timers (like me) are used to it thus changing it is also a POLA violation. The default names of the the files (there are three of them) will not change. Problem #9. The man page is adaquate however describing the general syntax of the file would help. Problem #10. 1. I don't agree that all table names should have unique names. Users are free to name their pools as they desire. Additionally I would like to maintain as much compatibility with NetBSD as possible. (I'll send them patches when I'm done.) 2. I disagree. 3. There is value to being able to resolve names, just as other firewalls such as pf and ipfw do. It's also consistent with the ipf command. 4. I'm not sure if parsing whois output ever worked. It may have at one time. I need to look at it first before I decide whether to remove it. 5. Agreed. Going down this rabbit hole, IPv6 isn't supported by radix_ipf. 6. There is no reason to reload stats and hit counts. In the context of ipfs(8) maybe but then all stats, not just pool stats, would need to be reloaded. I'll post my next patch when it's ready.
Problems 2, 3, and 4 are ALL caused by the same problem. That being the –t flag is being appended to the table name to make a unique identifier. You have now found and documented one of the major design flaws in ippool. This should be simple to remove the appending of the –t flag value to make the table name alone to be unique. This change is NOT a POLA violation because ippool has never worked from day one. Problem 5, 6 and 7. The –t flag also has to be removed. About saving the table hits at shutdown and reloading them at boot time. I consider the hits info just as useful and valuable as the “state table” contents. There is a firewall subsystem to save the “state” table contents at shutdown and the auto restore of it at boot time. I have revised my data acquisition model to no longer use ippool tables.
A commit references this bug: Author: cy Date: Mon May 15 03:39:36 UTC 2017 New revision: 318284 URL: https://svnweb.freebsd.org/changeset/base/318284 Log: Just like r318173, which was for outputting IPv6 addresses in tree pools, implement outputting of IPv6 addresses in the ippool debug list of hash type pools (ippool -l -d -t hash). Currently IPv6 in ippool tree type pool handling is mostly implemented. This continues theseries of commits to remediate ippool. This will be MFCed with a yet to be committed series of fixes to ippool after it has been fully remediated. PR: 218433 Changes: head/contrib/ipfilter/lib/printhashnode.c
A commit references this bug: Author: cy Date: Tue May 16 02:48:46 UTC 2017 New revision: 318333 URL: https://svnweb.freebsd.org/changeset/base/318333 Log: Implement ippool command line IPv6 address parse support (for the -i option). PR: 218433 Changes: head/contrib/ipfilter/tools/ippool.c
Not fixed yet. IPv6 still broken. Partial fix in my local tree. radix_ipf needs a rewrite.
A commit references this bug: Author: cy Date: Fri Mar 22 01:30:52 UTC 2019 New revision: 345400 URL: https://svnweb.freebsd.org/changeset/base/345400 Log: Add rc.d support for ippool(8). I've been using ippool at my site for approximately two years. It's about time this was committed. PR: 218433 MFC after: 2 weeks Changes: head/libexec/rc/rc.conf head/libexec/rc/rc.d/ippool
A commit references this bug: Author: cy Date: Fri Mar 22 01:42:27 UTC 2019 New revision: 345401 URL: https://svnweb.freebsd.org/changeset/base/345401 Log: From r345400, connect ippool to the build/install. PR: 218433 MFC after: 2 weeks X-MFC with: r345400 Changes: head/libexec/rc/rc.d/Makefile
A commit references this bug: Author: cy Date: Fri Mar 22 01:43:55 UTC 2019 New revision: 345403 URL: https://svnweb.freebsd.org/changeset/base/345403 Log: From r345400, remove the ippool rc script when ipfilter is not wanted by the user. PR: 218433 MFC after: 2 weeks X-MFC with: r345400 Changes: head/tools/build/mk/OptionalObsoleteFiles.inc
A commit references this bug: Author: cy Date: Fri Apr 5 01:22:31 UTC 2019 New revision: 345899 URL: https://svnweb.freebsd.org/changeset/base/345899 Log: MFC r345400-345401,345403,345412,345437: Add rc.d support for ippool(8). I've been using ippool at my site for approximately two years. It's about time this was committed. PR: 218433 Changes: _U stable/12/ stable/12/libexec/rc/rc.conf stable/12/libexec/rc/rc.d/Makefile stable/12/libexec/rc/rc.d/ippool stable/12/tools/build/mk/OptionalObsoleteFiles.inc
Still much more to be done. radix_ipf needs a fair bit of love and attention.
A commit references this bug: Author: cy Date: Fri Sep 27 00:29:06 UTC 2019 New revision: 352784 URL: https://svnweb.freebsd.org/changeset/base/352784 Log: Sync with source: Only a role of "ipf" is currentlysupported as the other documented (and undocumented) roles are #ifdef'd out. The plan is to complete ippool(8) as it is even in its current state a powerful feature/tool. PR: 218433 MFC after: 1 month Changes: head/contrib/ipfilter/man/ippool.8
A commit references this bug: Author: cy Date: Fri Sep 27 00:29:10 UTC 2019 New revision: 352785 URL: https://svnweb.freebsd.org/changeset/base/352785 Log: The no resolve (OPT_NORESOLVE) does nothing. Additionally, it (-R) conflicts with the command option of the same name (also -R). Remove the superfluous and confusing non-global non-command -R option. PR: 218433 MFC after: 1 week Changes: head/contrib/ipfilter/tools/ippool.c
A commit references this bug: Author: cy Date: Fri Sep 27 00:29:13 UTC 2019 New revision: 352786 URL: https://svnweb.freebsd.org/changeset/base/352786 Log: Implement the dynamic add (-A) and removal (-R) of ippool pools from the command line. Prior to this the functionality was mostly there however since the pool type (-t) was not recognized by the -A and -R command options -- not recognized by getopt(). Additionally the code to implement the dynamic add and removal of pools didn't work. When dynamically adding (-A) a pool a type (-t) to specify if the pool is a tree or hash pool must be specified. When dynamically removing (-R) a pool, omitting -t will cause a search-and-destroy which will remove both types of pools matching the name given (-m). PR: 218433 MFC after: 1 week Changes: head/contrib/ipfilter/man/ippool.8 head/contrib/ipfilter/tools/ippool.c
A commit references this bug: Author: cy Date: Fri Oct 4 02:09:52 UTC 2019 New revision: 353093 URL: https://svnweb.freebsd.org/changeset/base/353093 Log: MFC r352785: The no resolve (OPT_NORESOLVE) does nothing. Additionally, it (-R) conflicts with the command option of the same name (also -R). Remove the superfluous and confusing non-global non-command -R option. PR: 218433 Changes: _U stable/11/ stable/11/contrib/ipfilter/tools/ippool.c _U stable/12/ stable/12/contrib/ipfilter/tools/ippool.c
A commit references this bug: Author: cy Date: Fri Oct 4 02:11:46 UTC 2019 New revision: 353094 URL: https://svnweb.freebsd.org/changeset/base/353094 Log: MFC r352786: Implement the dynamic add (-A) and removal (-R) of ippool pools from the command line. Prior to this the functionality was mostly there however since the pool type (-t) was not recognized by the -A and -R command options -- not recognized by getopt(). Additionally the code to implement the dynamic add and removal of pools didn't work. When dynamically adding (-A) a pool a type (-t) to specify if the pool is a tree or hash pool must be specified. When dynamically removing (-R) a pool, omitting -t will cause a search-and-destroy which will remove both types of pools matching the name given (-m). PR: 218433 Changes: _U stable/11/ stable/11/contrib/ipfilter/man/ippool.8 stable/11/contrib/ipfilter/tools/ippool.c _U stable/12/ stable/12/contrib/ipfilter/man/ippool.8 stable/12/contrib/ipfilter/tools/ippool.c
A commit references this bug: Author: cy Date: Tue Oct 29 19:36:22 UTC 2019 New revision: 354153 URL: https://svnweb.freebsd.org/changeset/base/354153 Log: MFC r352784: Sync with source: Only a role of "ipf" is currently supported as the other documented (and undocumented) roles are #ifdef'd out. The plan is to complete ippool(8) as it is even in its current state a powerful feature/tool. PR: 218433 Changes: _U stable/11/ stable/11/contrib/ipfilter/man/ippool.8 _U stable/12/ stable/12/contrib/ipfilter/man/ippool.8
^Triage: committed back in 2019.