Re: Unified rc.firewall ipfw me/me6 issue

On Sun, Jan 17, 2010 at 12:04:43PM +0100, Luigi Rizzo wrote:
> On Sun, Jan 17, 2010 at 05:42:58PM +0900, Hajimu UMEMOTO wrote:
> > Hi,
> > 
> > >>>>> On Sun, 10 Jan 2010 19:52:32 +0100
> > >>>>> Luigi Rizzo <rizzo_at_iet.unipi.it> said:
> > 
> > rizzo> We only need one 'me' option that matches v4 and v6, because the
> > rizzo> other two can be implemented as 'ip4 me' and 'ip6 me' at no extra
> > rizzo> cost (the code for 'me' only scans the list corresponding to the
> > rizzo> actual address family of the packet).  I would actually vote for
> > rizzo> removing the 'me6' microinstruction from the kernel, and implement
> > rizzo> it in /sbin/ipfw by generating 'ip6 me'.

now that i think of it -- if we have an "or block" ie something like
	... { opt1 OR me6 OR .. }
the replacement of 'me6' with 'ip6 me' will not work as only one
microinstruction is allowed in an or block.
To fix this we would need some more radical change in expression parsing,
both in userland and in the kernel. I guess one way to handle this is
to signal a syntax error if we find a 'me6' or 'me4' within an OR block
(can be done during the rewriting in the loop below, because it is easy
to know if we are within a block).

cheers
luigi

> > rizzo> Feel free to commit the change yourself.
> > 
> > Thank you.  I've committed 1st patch and 3rd patch.
> > I think it is better removing the 'me6' microinstruction from the
> > kernel, and implement it in /sbin/ipfw by generating 'ip6 me'.
> > However, it seems to me that /sbin/ipfw is not designed to generate
> > two microinstructions (ip6 me) per one 'me6' easily.
> 
> Indeed, it might be useful to insert, at the beginning of function
> ipfw_add, a small preprocessing step that translates all instances
> of 'me6' into 'ip6 me' and then proceed with the current parsing.
> While doing that, one could even NULL-terminate the array av[] so
> we don't need to carry both ac and av throught the code.
> 
> Something like
> 
> 	new_av = safe_calloc(ac*2 + 1, sizeof(char *);
> 	for (src = dst = 0; src < ac; src++) {
> 		if (!strcmp(av[src], "me6")) {
> 			new_av[dst++] = "ip6";
> 			new_av[dst++] = "me";
> 		} else {	
> 			new_av[dst++] = av[src];
> 		}
> 	}
> 	new_av[dst++] = NULL;
> 	av = new_av;
> 	ac = dst;
> 
> should do the job. Replacing the tests for 'ac > 0' and ac>1
> is straightforward though it touches a large number of lines
> (most of the usage is in the 'NEED1' macro.
> 
> cheers
> luigi
> > Sincerely,
> > 
> > --
> > Hajimu UMEMOTO _at_ Internet Mutual Aid Society Yokohama, Japan
> > ume_at_mahoroba.org  ume_at_{,jp.}FreeBSD.org
> > http://www.imasy.org/~ume/
> > _______________________________________________
> > freebsd-net_at_freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-net
> > To unsubscribe, send any mail to "freebsd-net-unsubscribe_at_freebsd.org"