Date: Fri, 6 Jun 2014 00:10:26 +0800 From: "bycn82" <bycn82@gmail.com> To: "'Luigi Rizzo'" <rizzo@iet.unipi.it> Cc: "'Alexander V. Chernikov'" <melifaro@ipfw.ru>, 'FreeBSD Net' <net@FreeBSD.org> Subject: RE: [CFT]: ipfw named tables / different tabletypes Message-ID: <000401cf80d8$ad1bb840$075328c0$@gmail.com> In-Reply-To: <20140605155402.GA81905@onelab2.iet.unipi.it> References: <20140521204826.GA67124@onelab2.iet.unipi.it> <537E1029.70007@FreeBSD.org> <20140522154740.GA76448@onelab2.iet.unipi.it> <537E2153.1040005@FreeBSD.org> <20140522163812.GA77634@onelab2.iet.unipi.it> <538B2FE5.6070407@FreeBSD.org> <539044E4.1020904@ipfw.ru> <000c01cf80be$41194370$c34bca50$@gmail.com> <20140605134256.GA81234@onelab2.iet.unipi.it> <000001cf80cd$5dc1d9b0$19458d10$@gmail.com> <20140605155402.GA81905@onelab2.iet.unipi.it>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Sorry for waste you time to explain it again, I will read the code first.
And the latest patch of `PPS` should be OK, I checked the logic carefully this time. I sent it out last weekend.
logic as below, PPS actually will be fulfilled using `PPT`,(N packets per M ticks).
+ case O_PPT:
+ case O_PPS:{
+ ipfw_insn_pps *pps = (ipfw_insn_pps *)cmd;
+ if(pps->limit==0){
+ int limit,duration_in_ticks;
+ if(1000/hz > pps->duration){
+ duration_in_ticks=1;
+ }else{
+ duration_in_ticks=(pps->duration * hz +500)/1000;
+ }
+ limit=(cmd->arg1 *1000 * duration_in_ticks + hz * pps->duration/2)/(hz * pps->duration);
+ pps->limit=limit;
+ pps->duration_in_ticks=duration_in_ticks;
+ }
+ if(pps->start_time+pps->duration_in_ticks>= ticks){
+ if(pps->count < pps->limit){
+ retval = IP_FW_PASS;
+ }else{
+ retval = IP_FW_DENY;
+ }
+ pps->count++;
+ }else{
+ pps->start_time=ticks;
+ pps->count=1;
+ retval = IP_FW_PASS;
+ }
+ l = 0;
+ done = 1;
+ break;
+ }
> -----Original Message-----
> From: 'Luigi Rizzo' [mailto:rizzo@iet.unipi.it]
> Sent: 05 June, 2014 23:54
> To: bycn82
> Cc: 'Alexander V. Chernikov'; 'FreeBSD Net'
> Subject: Re: [CFT]: ipfw named tables / different tabletypes
>
> On Thu, Jun 05, 2014 at 10:49:27PM +0800, bycn82 wrote:
> > Hi Luigi,
> > Yes, use string instead of integer for the ID of table, but the same
> method cannot apply to the feature `set type of table`. And in the
> kernel, compare string will cause more than compare an integer. In my
> opinion, actually they are just alias name for the object. Users already
> accept that every object has an integer ID.
>
> Bill,
> I appreciate your enthusiasm on contributing to the project, but before
> starting to code, you (and everyone else, including myself) should spend
> some time at the drawing board, question your assumptions, and consider
> possible implementations.
>
> Also the fact that previous implementations had bugs or restrictions
> does not imply that we should repeat the same mistakes now.
>
> Specifically:
>
> - comparing strings in the kernel is perfectly fine, we do it all the
> time (sysctl names, filenames, interfaces and whatnot).
> What would be terribly wrong is having to do, on every packet,
> an expensive string or number lookup (note- it's the entire lookup
> process, not a string comparison) to locate the table.
> However, this is not the case.
> The way to implement named objects (tables etc.) is to accept strings
> as object names, and when the rule is added the string is checked
> and converted to a pointers or an integer which permits direct
> access to the table.
> This is as fast as it can be at runtime, which is all what matters.
>
> - at the moment tables and pipes have a single ID, which happens to be a
> sequence of digits for mostly historical reasons (you can read it
> as laziness of the original authors; i can take the blame for pipes).
>
> If we extend the namespace to include strings we improve the user's
> experience and remain compatible with the existing user interface.
>
> Instead, if we require users to create a mapping before using
> alphanumeric names, not only we add a step that was not there before,
> but also create two names for the same object which makes it harder
> to reason about the rulesets, and there is also the issue of how
> to handle references to non-existing tables (which are trivially
> supported now or with the approach i suggested, and would instead
> require rescanning the ruleset whenever a name-number association
> is added or deleted.
> Another troubles with that approach is that you opening a race
> window between the creation of the name-number mapping and its use,
> and this is also something you don't want in a security-related tool.
>
> We have had a similar discussion about your 'pps' extension for ipfw:
> useful feature, but your various implementations have issues (even the
> final one), and we have only so much time for reviewing patches; please
> do not burn it.
>
> cheers
> luigi
[-- Attachment #2 --]
Index: sbin/ipfw/ipfw.8
===================================================================
--- sbin/ipfw/ipfw.8 (revision 266886)
+++ sbin/ipfw/ipfw.8 (working copy)
@@ -602,6 +602,22 @@
Note: logging is done after all other packet matching conditions
have been successfully verified, and before performing the final
action (accept, deny, etc.) on the packet.
+.It Cm pps Ar limit duration
+Rule with the
+.Cm pps
+keyword will allow the first
+.Ar limit
+packets in recent
+.Ar duration
+milliseconds.
+.It Cm ppt Ar limit duration
+Rule with the
+.Cm ppt
+keyword will allow the first
+.Ar limit
+packets in recent
+.Ar duration
+ticks.
.It Cm tag Ar number
When a packet matches a rule with the
.Cm tag
Index: sbin/ipfw/ipfw2.c
===================================================================
--- sbin/ipfw/ipfw2.c (revision 266886)
+++ sbin/ipfw/ipfw2.c (working copy)
@@ -244,6 +244,8 @@
{ "allow", TOK_ACCEPT },
{ "permit", TOK_ACCEPT },
{ "count", TOK_COUNT },
+ { "pps", TOK_PPS },
+ { "ppt", TOK_PPT },
{ "pipe", TOK_PIPE },
{ "queue", TOK_QUEUE },
{ "divert", TOK_DIVERT },
@@ -1232,6 +1234,20 @@
PRINT_UINT_ARG("skipto ", cmd->arg1);
break;
+ case O_PPS:
+ {
+ ipfw_insn_pps *pps=(ipfw_insn_pps *)cmd;
+ printf("pps %d %d",cmd->arg1,pps->duration);
+ break;
+ }
+
+ case O_PPT:
+ {
+ ipfw_insn_pps *pps=(ipfw_insn_pps *)cmd;
+ printf("ppt %d %d",pps->limit,pps->duration_in_ticks);
+ break;
+ }
+
case O_PIPE:
PRINT_UINT_ARG("pipe ", cmd->arg1);
break;
@@ -2985,7 +3001,43 @@
case TOK_COUNT:
action->opcode = O_COUNT;
break;
+
+ case TOK_PPS:
+ action->opcode = O_PPS;
+ ipfw_insn_pps *pps = (ipfw_insn_pps *)action;
+ action->len = F_INSN_SIZE(ipfw_insn_pps);
+ if (isdigit(**av)) {
+ action->arg1 = strtoul(*av, NULL, 10);
+ av++;
+ }else{
+ errx(EX_USAGE, "illegal argument pps `limit` %s", *av);
+ }
+ if (isdigit(**av)) {
+ pps->duration= strtoul(*av, NULL, 10);
+ av++;
+ }else{
+ errx(EX_USAGE,"illegal arugment pps `duration` %s", *av);
+ }
+ break;
+ case TOK_PPT:
+ action->opcode = O_PPT;
+ ipfw_insn_pps *ppt = (ipfw_insn_pps *)action;
+ action->len = F_INSN_SIZE(ipfw_insn_pps);
+ if (isdigit(**av)) {
+ ppt->limit = strtoul(*av, NULL, 10);
+ av++;
+ }else{
+ errx(EX_USAGE, "illegal argument ppt `limit` %s", *av);
+ }
+ if (isdigit(**av)) {
+ ppt->duration_in_ticks= strtoul(*av, NULL, 10);
+ av++;
+ }else{
+ errx(EX_USAGE,"illegal arugment ppt `ticks` %s", *av);
+ }
+ break;
+
case TOK_NAT:
action->opcode = O_NAT;
action->len = F_INSN_SIZE(ipfw_insn_nat);
Index: sbin/ipfw/ipfw2.h
===================================================================
--- sbin/ipfw/ipfw2.h (revision 266886)
+++ sbin/ipfw/ipfw2.h (working copy)
@@ -92,6 +92,8 @@
TOK_NGTEE,
TOK_FORWARD,
TOK_SKIPTO,
+ TOK_PPS,
+ TOK_PPT,
TOK_DENY,
TOK_REJECT,
TOK_RESET,
Index: sys/netinet/ip_fw.h
===================================================================
--- sys/netinet/ip_fw.h (revision 266886)
+++ sys/netinet/ip_fw.h (working copy)
@@ -165,6 +165,8 @@
O_REJECT, /* arg1=icmp arg (same as deny) */
O_COUNT, /* none */
O_SKIPTO, /* arg1=next rule number */
+ O_PPS, /* arg1=limit, pps->duration */
+ O_PPT, /* pps->limit, pps->duration_in_ticks */
O_PIPE, /* arg1=pipe number */
O_QUEUE, /* arg1=queue number */
O_DIVERT, /* arg1=port number */
@@ -378,6 +380,18 @@
} ipfw_insn_log;
/*
+ * This is used for PPS
+ */
+typedef struct _ipfw_insn_pps{
+ ipfw_insn o;
+ uint32_t start_time;
+ uint32_t count;
+ uint32_t duration;
+ uint32_t limit;
+ uint32_t duration_in_ticks;
+} ipfw_insn_pps;
+
+/*
* Data structures required by both ipfw(8) and ipfw(4) but not part of the
* management API are protected by IPFW_INTERNAL.
*/
Index: sys/netpfil/ipfw/ip_fw2.c
===================================================================
--- sys/netpfil/ipfw/ip_fw2.c (revision 266886)
+++ sys/netpfil/ipfw/ip_fw2.c (working copy)
@@ -2188,6 +2188,37 @@
skip_or = 0;
continue;
break; /* not reached */
+
+ case O_PPT:
+ case O_PPS:{
+ ipfw_insn_pps *pps = (ipfw_insn_pps *)cmd;
+ if(pps->limit==0){
+ int limit,duration_in_ticks;
+ if(1000/hz > pps->duration){
+ duration_in_ticks=1;
+ }else{
+ duration_in_ticks=(pps->duration * hz +500)/1000;
+ }
+ limit=(cmd->arg1 *1000 * duration_in_ticks + hz * pps->duration/2)/(hz * pps->duration);
+ pps->limit=limit;
+ pps->duration_in_ticks=duration_in_ticks;
+ }
+ if(pps->start_time+pps->duration_in_ticks>= ticks){
+ if(pps->count < pps->limit){
+ retval = IP_FW_PASS;
+ }else{
+ retval = IP_FW_DENY;
+ }
+ pps->count++;
+ }else{
+ pps->start_time=ticks;
+ pps->count=1;
+ retval = IP_FW_PASS;
+ }
+ l = 0;
+ done = 1;
+ break;
+ }
case O_CALLRETURN: {
/*
Index: sys/netpfil/ipfw/ip_fw_sockopt.c
===================================================================
--- sys/netpfil/ipfw/ip_fw_sockopt.c (revision 266886)
+++ sys/netpfil/ipfw/ip_fw_sockopt.c (working copy)
@@ -703,6 +703,13 @@
goto bad_size;
break;
+ case O_PPS:
+ case O_PPT:
+ have_action=1;
+ if (cmdlen != F_INSN_SIZE(ipfw_insn_pps))
+ goto bad_size;
+ break;
+
case O_PIPE:
case O_QUEUE:
if (cmdlen != F_INSN_SIZE(ipfw_insn))
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?000401cf80d8$ad1bb840$075328c0$>