Re: Patch 1/3 - make DIOCRADDADDRS to accept on IP address per ioctl() call

2015-11-09 Thread Alexandr Nedvedicky 
> 
> I'm wondering - how does it affect tools that load several thousands of IPs
> into a table?  Like spamd, bgpd (for spam lists etc.), or pfctl for IP black
> lists (as distributed by ET).
> 
> There are valid use cases with HUGE tables, but I have to admit that I didn't
> test your diff yet. Just a concern that loading IPs one after another might
> take forever.
> 

I could measure no difference on sample of 1 unique IPv4 addresses.
Both (pfr_add_addrs/pfr_add_addr) could load them within 1sec.

pfr_add_addrs:
# wc -l test.table.pf ; date ; pfctl -t test -T add -f test.table.pf ; date
10 test.table.pf
Mon Nov  9 18:21:18 CET 2015
1 table created.
10/10 addresses added.
Mon Nov  9 18:21:19 CET 2015


pfr_add_addr:
Mon Nov  9 18:31:27 CET 2015
# wc -l test.table.pf ; date ; pfctl -t test -T add -f test.table.pf ; date
10 test.table.pf
Mon Nov  9 18:31:27 CET 2015
1 table created.  10/10 addresses added.
Mon Nov  9 18:31:28 CET 2015

My test machine is Toshiba Tecra with Centrino 2.

regards
sasha

Re: Patch 1/3 - make DIOCRADDADDRS to accept on IP address per ioctl() call

2015-11-09 Thread Reyk Floeter 

> On 09.11.2015, at 22:21, Alexandr Nedvedicky  
> wrote:
> 
> On Sun, Nov 08, 2015 at 01:18:22PM +0100, Alexander Bluhm wrote:
>> On Sun, Nov 08, 2015 at 02:37:58AM +0100, Alexander Bluhm wrote:
 +  for (i = 0; (i < size) && (rv == 0); i++) {
>>> 
>>> rv is unitialized in the first interation
>>> 
 +  io.pfrio_buffer = addr++;
 +  rv = ioctl(dev, DIOCRADDADDR, &io);
>>> 
>>> I would suggest to return (-1) if ioctl fails...
>>> 
 +  add++;
 +  }
>> 
>> To keep the illusion of an atomic operation, we could remove the
>> addresses we just added before the one add failed.
>> 
> 
> actually pfctl_radix.c is just tip of the iceberg,  there are other tools than
> pfctl, which manipulate with PF-tables:
>   authpf
>   bgpd
>   pfutils
> 
> The more I'm thinking about s/SIOCADDADDRS/SIOCADDADDR the less I like it.  I
> feel good about s/pfr_add_addrs/pfr_add_addr. The pfr_add_addr() should be a
> back end for SIOCADDADDRS ioctl operation, which I think should go back.  The
> ioctl in kernel will iterate over the array of addresses coming from userland.
> It seems to me as more convenient approach. I'm working on prototype, I
> hope I'll send updated patches soon.
> 

I'm wondering - how does it affect tools that load several thousands of IPs 
into a table?
Like spamd, bgpd (for spam lists etc.), or pfctl for IP black lists (as 
distributed by ET).

There are valid use cases with HUGE tables, but I have to admit that I didn't 
test your
diff yet. Just a concern that loading IPs one after another might take forever.

Reyk

Re: Patch 1/3 - make DIOCRADDADDRS to accept on IP address per ioctl() call

2015-11-09 Thread Alexandr Nedvedicky 
On Sun, Nov 08, 2015 at 01:18:22PM +0100, Alexander Bluhm wrote:
> On Sun, Nov 08, 2015 at 02:37:58AM +0100, Alexander Bluhm wrote:
> > > + for (i = 0; (i < size) && (rv == 0); i++) {
> > 
> > rv is unitialized in the first interation
> > 
> > > + io.pfrio_buffer = addr++;
> > > + rv = ioctl(dev, DIOCRADDADDR, &io);
> > 
> > I would suggest to return (-1) if ioctl fails...
> > 
> > > + add++;
> > > + }
> 
> To keep the illusion of an atomic operation, we could remove the
> addresses we just added before the one add failed.
> 

actually pfctl_radix.c is just tip of the iceberg,  there are other tools than
pfctl, which manipulate with PF-tables:
authpf
bgpd
pfutils

The more I'm thinking about s/SIOCADDADDRS/SIOCADDADDR the less I like it.  I
feel good about s/pfr_add_addrs/pfr_add_addr. The pfr_add_addr() should be a
back end for SIOCADDADDRS ioctl operation, which I think should go back.  The
ioctl in kernel will iterate over the array of addresses coming from userland.
It seems to me as more convenient approach. I'm working on prototype, I
hope I'll send updated patches soon.

thanks and
regards
sasha

Re: Patch 1/3 - make DIOCRADDADDRS to accept on IP address per ioctl() call

2015-11-09 Thread Alexandr Nedvedicky 
Hello,

> On Wed, Oct 28, 2015 at 06:19:48PM +0100, Alexandr Nedvedicky wrote:
> > The idea has been proposed by Claudio at Varazdin.
> 
> I guess the idea is to eliminate the workq.  Or is ther naother
> reason to change it?

the primary goal is to kill work queues.

> 
> Comments inline
> 
thank you very much for very good code review.

> > -   *nadd = io.pfrio_nadd;
> > -   return (0);
> > +   io.pfrio_size = 1;  /* TODO: check .pfrio_size is needed */
> > +   for (i = 0; (i < size) && (rv == 0); i++) {
> 
> rv is unitialized in the first interation

it's fixed by change below:

@@ -184,7 +184,8 @@
 int *nadd, int flags)
 {
struct pfioc_table io;
-   int i, rv, add = 0;
+   int i, add = 0;
+   int rv = 0; 
 
> 
> > +   io.pfrio_buffer = addr++;
> > +   rv = ioctl(dev, DIOCRADDADDR, &io);
> 
> I would suggest to return (-1) if ioctl fails...
I'll write my response in email answering your note on 'illusion of atomicity'

> 
> pfr_add_addr() handles exactly 1 address, don't pass io->pfrio_size.
> 

yes that's true...

error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer,
-   io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
+   io->pfrio_flags | PFR_FLAG_USERIOCTL);
break;

> >  
> >  int
> > +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int 
> > flags)
> 
> Do not pass size.

-pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int flags)
+pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int flags)

> 
> Should you check for !(flags & PFR_FLAG_DUMMY) here?  It was done
> in the old code before pfr_insert_kentries().
> 
> > +   rv = pfr_insert_kentry(kt, &ad, tzero);
> 
> The old code ignored wether pfr_insert_kentries() succeeded.
> 

thanks for catching this.

@@ -288,20 +288,20 @@
senderr(EINVAL);
p = pfr_lookup_addr(kt, &ad, 1);
if (p == NULL) {
-   rv = pfr_insert_kentry(kt, &ad, tzero);
+   if (!(flags & PFR_FLAG_DUMMY))
+   rv = pfr_insert_kentry(kt, &ad, tzero);
+   else
+   rv = 0;
}

> 
> No { } for one line if block.

there is no longer one line if block after fixing  PFR_FLAG_DUMMY flag.

> 
> PFR_FB_DUPLICATE was used when there were two identical addresses
> in the passed list.  This cannot happen anymore.  It should be
> PFR_FB_NONE in this case.
> 
> > +   } else if (rv != 0) {
> > +   ad.pfra_fback = PFR_FB_NONE;
> > +   } else {
> > +   ad.pfra_fback = PFR_FB_ADDED;
> > +   }
> 
> Perhaps write this block as
> 
>   if (p == NULL)
>   ad.pfra_fback = PFR_FB_ADDED;
>   else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not))
>   ad.pfra_fback = PFR_FB_CONFLICT;
>   else
>   ad.pfra_fback = PFR_FB_NONE;
> 

I thinks we still must check rv coming from pfr_insert_kentry():

if (flags & PFR_FLAG_FEEDBACK) {
-   if (p != NULL) {
-   if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not)
-   ad.pfra_fback = PFR_FB_CONFLICT;
-   else
-   ad.pfra_fback = PFR_FB_DUPLICATE;
-   } else if (rv != 0) {
+   if (p == NULL)
+   ad.pfra_fback = (rv == 0) ? PFR_FB_ADDED : PFR_FB_NONE;
+   else if ((p->pfrke_flags & PFRKE_FLAG_NOT) != ad.pfra_not)
+   ad.pfra_fback = PFR_FB_CONFLICT;
+   else
ad.pfra_fback = PFR_FB_NONE;
-   } else {
-   ad.pfra_fback = PFR_FB_ADDED;
-   }

 
> > +_bad:
> > +   if (flags & PFR_FLAG_FEEDBACK)
> > +   pfr_reset_feedback(addr, size, flags);
> 
> Don't use size, it must be 1.

the size argument got dropped.

> 
> > +   return (rv);
> 
> rv may be unitialized

thanks for catching that.

> 
> pfr_add_addrs() is not used anymore, remove it.

pfr_add_addrs() is gone in new patch.

thanks and
regards
sasha

8<---8<-8<

Index: sbin/pfctl/pfctl_radix.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
retrieving revision 1.32
diff -u -p -r1.32 pfctl_radix.c
--- sbin/pfctl/pfctl_radix.c21 Jan 2015 21:50:33 -  1.32
+++ sbin/pfctl/pfctl_radix.c9 Nov 2015 20:43:52 -
@@ -184,6 +184,8 @@ pfr_add_addrs(struct pfr_table *tbl, str
 int *nadd, int flags)
 {
struct pfioc_table io;
+   int i, add = 0;
+   int rv = 0; 
 
if (tbl == NULL || size < 0 || (size && addr == NULL)) {
errno = EINVAL;
@@ -192,14 +194,19 @@ pfr_add_addrs(struct pfr_table *tbl, str
bzero(&io, sizeof io);

Re: Patch 1/3 - make DIOCRADDADDRS to accept on IP address per ioctl() call

2015-11-08 Thread Alexander Bluhm 
On Sun, Nov 08, 2015 at 02:37:58AM +0100, Alexander Bluhm wrote:
> > +   for (i = 0; (i < size) && (rv == 0); i++) {
> 
> rv is unitialized in the first interation
> 
> > +   io.pfrio_buffer = addr++;
> > +   rv = ioctl(dev, DIOCRADDADDR, &io);
> 
> I would suggest to return (-1) if ioctl fails...
> 
> > +   add++;
> > +   }

To keep the illusion of an atomic operation, we could remove the
addresses we just added before the one add failed.

bluhm

Re: Patch 1/3 - make DIOCRADDADDRS to accept on IP address per ioctl() call

2015-11-07 Thread Alexander Bluhm 
On Wed, Oct 28, 2015 at 06:19:48PM +0100, Alexandr Nedvedicky wrote:
> The idea has been proposed by Claudio at Varazdin.

I guess the idea is to eliminate the workq.  Or is ther naother
reason to change it?

Comments inline

> Index: sbin/pfctl/pfctl_radix.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 pfctl_radix.c
> --- sbin/pfctl/pfctl_radix.c  21 Jan 2015 21:50:33 -  1.32
> +++ sbin/pfctl/pfctl_radix.c  27 Oct 2015 23:24:59 -
> @@ -184,6 +184,7 @@ pfr_add_addrs(struct pfr_table *tbl, str
>  int *nadd, int flags)
>  {
>   struct pfioc_table io;
> + int i, rv, add = 0;
>  
>   if (tbl == NULL || size < 0 || (size && addr == NULL)) {
>   errno = EINVAL;
> @@ -192,14 +193,18 @@ pfr_add_addrs(struct pfr_table *tbl, str
>   bzero(&io, sizeof io);
>   io.pfrio_flags = flags;
>   io.pfrio_table = *tbl;
> - io.pfrio_buffer = addr;
>   io.pfrio_esize = sizeof(*addr);
> - io.pfrio_size = size;
> - if (ioctl(dev, DIOCRADDADDRS, &io))
> - return (-1);
> - if (nadd != NULL)
> - *nadd = io.pfrio_nadd;
> - return (0);
> + io.pfrio_size = 1;  /* TODO: check .pfrio_size is needed */
> + for (i = 0; (i < size) && (rv == 0); i++) {

rv is unitialized in the first interation

> + io.pfrio_buffer = addr++;
> + rv = ioctl(dev, DIOCRADDADDR, &io);

I would suggest to return (-1) if ioctl fails...

> + add++;
> + }
> +
> + if ((rv == 0) && (nadd != NULL))
> + *nadd = add;
> +
> + return (rv);

... then you can return (0) here and don't need the rv variable.

>  }
>  
>  int
> Index: sys/net/pf_ioctl.c
> ===
> RCS file: /cvs/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.291
> diff -u -p -r1.291 pf_ioctl.c
> --- sys/net/pf_ioctl.c13 Oct 2015 19:32:31 -  1.291
> +++ sys/net/pf_ioctl.c27 Oct 2015 23:25:23 -
> @@ -834,7 +834,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
>   case DIOCRGETTSTATS:
>   case DIOCRCLRTSTATS:
>   case DIOCRCLRADDRS:
> - case DIOCRADDADDRS:
> + case DIOCRADDADDR:
>   case DIOCRDELADDRS:
>   case DIOCRSETADDRS:
>   case DIOCRGETASTATS:
> @@ -887,7 +887,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
>   case DIOCRDELTABLES:
>   case DIOCRCLRTSTATS:
>   case DIOCRCLRADDRS:
> - case DIOCRADDADDRS:
> + case DIOCRADDADDR:
>   case DIOCRDELADDRS:
>   case DIOCRSETADDRS:
>   case DIOCRSETTFLAGS:
> @@ -1816,16 +1816,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
>   break;
>   }
>  
> - case DIOCRADDADDRS: {
> + case DIOCRADDADDR: {
>   struct pfioc_table *io = (struct pfioc_table *)addr;
>  
>   if (io->pfrio_esize != sizeof(struct pfr_addr)) {
>   error = ENODEV;
>   break;
>   }
> - error = pfr_add_addrs(&io->pfrio_table, io->pfrio_buffer,
> - io->pfrio_size, &io->pfrio_nadd, io->pfrio_flags |
> - PFR_FLAG_USERIOCTL);
> + error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer,
> + io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);

pfr_add_addr() handles exactly 1 address, don't pass io->pfrio_size.

>   break;
>   }
>  
> Index: sys/net/pf_table.c
> ===
> RCS file: /cvs/src/sys/net/pf_table.c,v
> retrieving revision 1.115
> diff -u -p -r1.115 pf_table.c
> --- sys/net/pf_table.c7 Oct 2015 11:57:44 -   1.115
> +++ sys/net/pf_table.c27 Oct 2015 23:25:26 -
> @@ -266,6 +266,54 @@ pfr_clr_addrs(struct pfr_table *tbl, int
>  }
>  
>  int
> +pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int 
> flags)

Do not pass size.

> +{
> + struct pfr_ktable   *kt;
> + struct pfr_kentry   *p;
> + struct pfr_addr  ad;
> + int  rv;
> + time_t   tzero = time_second;
> +
> + ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK);
> + if (pfr_validate_table(tbl, 0, flags & PFR_FLAG_USERIOCTL))
> + return (EINVAL);
> + kt = pfr_lookup_table(tbl);
> + if (kt == NULL || !(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
> + return (ESRCH);
> + if (kt->pfrkt_flags & PFR_TFLAG_CONST)
> + return (EPERM);
> + if (COPYIN(addr, &ad, sizeof(ad), flags))
> + senderr(EFAULT);
> + if (pfr_validate_addr(&ad))
> + senderr(EINVAL);
> + p = pfr_lookup_addr(kt, &ad, 1);
> + if (p == NULL) {

Should you check for !(flags & PFR

Patch 1/3 - make DIOCRADDADDRS to accept on IP address per ioctl() call

2015-10-28 Thread Alexandr Nedvedicky 
Hello,

this is the first patch in series of three. All patches modify PF radix
table API such the ioctl() functions accept one IP address per call.
The idea has been proposed by Claudio at Varazdin.

I still have to untangle pfr_commit_ktable() and DIOCRSETADDRS ioctl.  Both
seem to be more complicated than DIOCRADDADDRS and DIOCRDELADDRS (subject of
next patch).

Patch changes DIOCRADDADDRS ioctl to DIOCRADDADDRS, which accepts one IP
address only per ioctl(2) call. Patch updates kernel and pfctl(8) only. Other
components, which happen to use DIOCRADDADDRS will be updated by extra patch.

thanks and
regards
sasha

P.S. I still need to update pf(4) manpage, I'll do it as soon as there we
will reach agreement on proposed patches.

8<---8<---8<--8<

Index: sbin/pfctl/pfctl_radix.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_radix.c,v
retrieving revision 1.32
diff -u -p -r1.32 pfctl_radix.c
--- sbin/pfctl/pfctl_radix.c21 Jan 2015 21:50:33 -  1.32
+++ sbin/pfctl/pfctl_radix.c27 Oct 2015 23:24:59 -
@@ -184,6 +184,7 @@ pfr_add_addrs(struct pfr_table *tbl, str
 int *nadd, int flags)
 {
struct pfioc_table io;
+   int i, rv, add = 0;
 
if (tbl == NULL || size < 0 || (size && addr == NULL)) {
errno = EINVAL;
@@ -192,14 +193,18 @@ pfr_add_addrs(struct pfr_table *tbl, str
bzero(&io, sizeof io);
io.pfrio_flags = flags;
io.pfrio_table = *tbl;
-   io.pfrio_buffer = addr;
io.pfrio_esize = sizeof(*addr);
-   io.pfrio_size = size;
-   if (ioctl(dev, DIOCRADDADDRS, &io))
-   return (-1);
-   if (nadd != NULL)
-   *nadd = io.pfrio_nadd;
-   return (0);
+   io.pfrio_size = 1;  /* TODO: check .pfrio_size is needed */
+   for (i = 0; (i < size) && (rv == 0); i++) {
+   io.pfrio_buffer = addr++;
+   rv = ioctl(dev, DIOCRADDADDR, &io);
+   add++;
+   }
+
+   if ((rv == 0) && (nadd != NULL))
+   *nadd = add;
+
+   return (rv);
 }
 
 int
Index: sys/net/pf_ioctl.c
===
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.291
diff -u -p -r1.291 pf_ioctl.c
--- sys/net/pf_ioctl.c  13 Oct 2015 19:32:31 -  1.291
+++ sys/net/pf_ioctl.c  27 Oct 2015 23:25:23 -
@@ -834,7 +834,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCRGETTSTATS:
case DIOCRCLRTSTATS:
case DIOCRCLRADDRS:
-   case DIOCRADDADDRS:
+   case DIOCRADDADDR:
case DIOCRDELADDRS:
case DIOCRSETADDRS:
case DIOCRGETASTATS:
@@ -887,7 +887,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
case DIOCRDELTABLES:
case DIOCRCLRTSTATS:
case DIOCRCLRADDRS:
-   case DIOCRADDADDRS:
+   case DIOCRADDADDR:
case DIOCRDELADDRS:
case DIOCRSETADDRS:
case DIOCRSETTFLAGS:
@@ -1816,16 +1816,15 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
break;
}
 
-   case DIOCRADDADDRS: {
+   case DIOCRADDADDR: {
struct pfioc_table *io = (struct pfioc_table *)addr;
 
if (io->pfrio_esize != sizeof(struct pfr_addr)) {
error = ENODEV;
break;
}
-   error = pfr_add_addrs(&io->pfrio_table, io->pfrio_buffer,
-   io->pfrio_size, &io->pfrio_nadd, io->pfrio_flags |
-   PFR_FLAG_USERIOCTL);
+   error = pfr_add_addr(&io->pfrio_table, io->pfrio_buffer,
+   io->pfrio_size, io->pfrio_flags | PFR_FLAG_USERIOCTL);
break;
}
 
Index: sys/net/pf_table.c
===
RCS file: /cvs/src/sys/net/pf_table.c,v
retrieving revision 1.115
diff -u -p -r1.115 pf_table.c
--- sys/net/pf_table.c  7 Oct 2015 11:57:44 -   1.115
+++ sys/net/pf_table.c  27 Oct 2015 23:25:26 -
@@ -266,6 +266,54 @@ pfr_clr_addrs(struct pfr_table *tbl, int
 }
 
 int
+pfr_add_addr(struct pfr_table *tbl, struct pfr_addr *addr, int size, int flags)
+{
+   struct pfr_ktable   *kt;
+   struct pfr_kentry   *p;
+   struct pfr_addr  ad;
+   int  rv;
+   time_t   tzero = time_second;
+
+   ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK);
+   if (pfr_validate_table(tbl, 0, flags & PFR_FLAG_USERIOCTL))
+   return (EINVAL);
+   kt = pfr_lookup_table(tbl);
+   if (kt == NULL || !(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
+   return (ESRCH);
+   if (kt->pfrkt_flags & PFR_TFLAG_CONST)
+   return (EPERM);
+   if (COPYIN(addr, &ad