childlevel's pom comment

childlevel's pom comment

[quadong]

Hi, I noticed that the childlevel patch currently has this to say about
itself in patch-o-matic:

	"This adds CONFIG_IP_NF_MATCH_CHILDLEVEL option, which be used to
	match the childlevel of a connection."

I know _I_ would be confused if I read this.  Could it be changed to the
following?

	This patch allows you to match on the childlevel of a connection.
	A master connection, such as the command stream of FTP, has a
	childlevel of zero, its first child, such as the data stream of
	FTP, has a childlevel of one.  Usage example:

	iptables ... -m childlevel --level 1 ...

Thanks,
matthew

Re: childlevel's pom comment

[Patrick McHardy]

Hi Mathew,

I'm going to add your updated help-text. But I have doubts about
whether a new match is required at all. A connection can only have
a childlevel of one because only masters have helpers assigned to
them, although in theory the expect-function could register more
expectations. So you can basically match if a connection was
expected or not. The same can also be achieved by a trivial change
to the helper match, just allow zero-string length and use them
as a special value. This should also allow full userspace
compatibility. What do you think ?

Regards
Patrick


quadong@users.sourceforge.net wrote:
> Hi, I noticed that the childlevel patch currently has this to say about
> itself in patch-o-matic:
> 
> 	"This adds CONFIG_IP_NF_MATCH_CHILDLEVEL option, which be used to
> 	match the childlevel of a connection."
> 
> I know _I_ would be confused if I read this.  Could it be changed to the
> following?
> 
> 	This patch allows you to match on the childlevel of a connection.
> 	A master connection, such as the command stream of FTP, has a
> 	childlevel of zero, its first child, such as the data stream of
> 	FTP, has a childlevel of one.  Usage example:
> 
> 	iptables ... -m childlevel --level 1 ...
> 
> Thanks,
> matthew

Re: childlevel's pom comment

[Matthew Strait]

> I'm going to add your updated help-text. But I have doubts about whether
> a new match is required at all. A connection can only have a childlevel
> of one because only masters have helpers assigned to them, although in
> theory the expect-function could register more expectations. So you can
> basically match if a connection was expected or not. The same can also
> be achieved by a trivial change to the helper match, just allow
> zero-string length and use them as a special value. This should also
> allow full userspace compatibility. What do you think ?

Sounds like a good idea.  The only variation I'd make is to have the
helper match take "any" as the special value, because it's hard to pass an
empty string to iptables.  (Trying to pass "" gets you an error message
and trying to pass \"\" ends up giving the module literal quotation marks.
If I'm missing something obvious, let me know.  Here are the changes
needed in the kernel:

--- ipt_helper.c.old    2004-03-03 21:34:05.625216688 -0600
+++ ipt_helper.c        2004-03-03 22:32:06.239867656 -0600
@@ -48,7 +48,7 @@

        if (!ct->master) {
                DEBUGP("ipt_helper: conntrack %p has no master\n", ct);
-               return 0;
+               return info->invert;
        }

        exp = ct->master;
@@ -68,8 +68,11 @@
        DEBUGP("master's name = %s , info->name = %s\n",
                exp->expectant->helper->name, info->name);

-       ret = !strncmp(exp->expectant->helper->name, info->name,
-                      strlen(exp->expectant->helper->name)) ^
info->invert;
+       if(!strcmp(info->name, "any"))
+               ret = 1 ^ info->invert;
+       else
+               ret = !strncmp(exp->expectant->helper->name, info->name,
+                              strlen(exp->expectant->helper->name)) ^
info->invert;
 out_unlock:
        READ_UNLOCK(&ip_conntrack_lock);
        return ret;


I have to change the 0 to info->invert so that "! --helper any" means
"packets that aren't from any helper".  With the 0, that rule would never
match.  It seems to me that this is the correct behavior even without
adding the "any" special case.  If I ask for "! --helper ftp", I would
expect master packets to match, because they are not from an ftp helper.

Trivial documentational changes in userspace:

--- libipt_helper.c.old 2004-03-03 21:39:07.255361936 -0600
+++ libipt_helper.c     2004-03-03 21:52:10.838861040 -0600
@@ -15,6 +15,7 @@
        printf(
 "helper match v%s options:\n"
 "[!] --helper string        Match helper identified by string\n"
+"                           (or any helper if string is "any")\n"
 "\n",
 IPTABLES_VERSION);
 }

--- iptables.8.old      2004-03-03 22:18:55.017383552 -0600
+++ iptables.8  2004-03-03 22:20:43.137999928 -0600
@@ -458,6 +458,8 @@
 For other ports append -portnr to the value, ie. "ftp-2121".
 .PP
 Same rules apply for other conntrack-helpers.
+.PP
+Use "any" to match any helper.
 .RE
 .SS icmp
 This extension is loaded if `--protocol icmp' is specified.  It




If this patch is accepted, I don't see any reason to keep childlevel in
patch-o-matic.

-matthew

Re: childlevel's pom comment

[Patrick McHardy]

Matthew Strait wrote:
>>I'm going to add your updated help-text. But I have doubts about whether
>>a new match is required at all. A connection can only have a childlevel
>>of one because only masters have helpers assigned to them, although in
>>theory the expect-function could register more expectations. So you can
>>basically match if a connection was expected or not. The same can also
>>be achieved by a trivial change to the helper match, just allow
>>zero-string length and use them as a special value. This should also
>>allow full userspace compatibility. What do you think ?
> 
> 
> Sounds like a good idea.  The only variation I'd make is to have the
> helper match take "any" as the special value, because it's hard to pass an
> empty string to iptables.  (Trying to pass "" gets you an error message
> and trying to pass \"\" ends up giving the module literal quotation marks.
> If I'm missing something obvious, let me know.  Here are the changes
> needed in the kernel:
> 
> --- ipt_helper.c.old    2004-03-03 21:34:05.625216688 -0600
> +++ ipt_helper.c        2004-03-03 22:32:06.239867656 -0600
> @@ -48,7 +48,7 @@
> 
>         if (!ct->master) {
>                 DEBUGP("ipt_helper: conntrack %p has no master\n", ct);
> -               return 0;
> +               return info->invert;
>         }
> 
>         exp = ct->master;
> @@ -68,8 +68,11 @@
>         DEBUGP("master's name = %s , info->name = %s\n",
>                 exp->expectant->helper->name, info->name);
> 
> -       ret = !strncmp(exp->expectant->helper->name, info->name,
> -                      strlen(exp->expectant->helper->name)) ^
> info->invert;
> +       if(!strcmp(info->name, "any"))
> +               ret = 1 ^ info->invert;
> +       else
> +               ret = !strncmp(exp->expectant->helper->name, info->name,
> +                              strlen(exp->expectant->helper->name)) ^
> info->invert;

I would prefer if "any" wouldn't need to be matched in the kernel,
something like:

if (info->name[0] == '\0')
	ret = !info->invert;
else
	...

and have userspace do the "any" <=> "" translation.

>  out_unlock:
>         READ_UNLOCK(&ip_conntrack_lock);
>         return ret;
> 
> 
> I have to change the 0 to info->invert so that "! --helper any" means
> "packets that aren't from any helper".  With the 0, that rule would never
> match.  It seems to me that this is the correct behavior even without
> adding the "any" special case.  If I ask for "! --helper ftp", I would
> expect master packets to match, because they are not from an ftp helper.

That is correct. Can you send a seperate patch for this, as a bugfix
this should be submitted to the kernel anyway.

> 
> Trivial documentational changes in userspace:
> 
> --- libipt_helper.c.old 2004-03-03 21:39:07.255361936 -0600
> +++ libipt_helper.c     2004-03-03 21:52:10.838861040 -0600
> @@ -15,6 +15,7 @@
>         printf(
>  "helper match v%s options:\n"
>  "[!] --helper string        Match helper identified by string\n"
> +"                           (or any helper if string is "any")\n"
>  "\n",
>  IPTABLES_VERSION);
>  }
> 
> --- iptables.8.old      2004-03-03 22:18:55.017383552 -0600
> +++ iptables.8  2004-03-03 22:20:43.137999928 -0600
> @@ -458,6 +458,8 @@
>  For other ports append -portnr to the value, ie. "ftp-2121".
>  .PP
>  Same rules apply for other conntrack-helpers.
> +.PP
> +Use "any" to match any helper.
>  .RE
>  .SS icmp
>  This extension is loaded if `--protocol icmp' is specified.  It
> 
> 
> 
> 
> If this patch is accepted, I don't see any reason to keep childlevel in
> patch-o-matic.

Can you please send a new patch which removes the "any" strcmp ?
We can ask Martin about his opinion then (he is the author of
the helper match), but I don't think there will be a problem.

Regards
Patrick

> 
> -matthew
> 

Re: childlevel's pom comment

[Matthew Strait]

> I would prefer if "any" wouldn't need to be matched in the kernel,
> something like:
>
> if (info->name[0] == '\0')
>        ret = !info->invert;
> else
>        ...
>
> and have userspace do the "any" <=> "" translation.

That would be fine with me, but iptables seems to consider the empty
string to be an error condition:

--- libipt_helper.c.old 2004-03-03 21:39:07.000000000 -0600
+++ libipt_helper.c     2004-03-04 14:50:13.408377032 -0600
@@ -46,6 +47,12 @@
        case '1':
                check_inverse(optarg, &invert, &invert, 0);
                strncpy(info->name, optarg, 29);
+
+               /* special case, translate "any" to "" so the kernel
+               doesn't have to run strcmp. */
+               if(!strcmp(info->name, "any"))
+                       info->name[0] = '\0';
+
                if (invert)
                        info->invert = 1;
                *flags = 1;


root@quadong# iptables -A POSTROUTING -t mangle -m helper --helper any
iptables: Invalid argument

It seems like I'd have to make significantly more invasive changes than
are really called for to get it to accept an empty string.  What do you
think?

-matthew

Re: childlevel's pom comment

[Patrick McHardy]

Matthew Strait wrote:
>>I would prefer if "any" wouldn't need to be matched in the kernel,
>>something like:
>>
>>if (info->name[0] == '\0')
>>       ret = !info->invert;
>>else
>>       ...
>>
>>and have userspace do the "any" <=> "" translation.
> 
> 
> That would be fine with me, but iptables seems to consider the empty
> string to be an error condition:
> 
> --- libipt_helper.c.old 2004-03-03 21:39:07.000000000 -0600
> +++ libipt_helper.c     2004-03-04 14:50:13.408377032 -0600
> @@ -46,6 +47,12 @@
>         case '1':
>                 check_inverse(optarg, &invert, &invert, 0);
>                 strncpy(info->name, optarg, 29);
> +
> +               /* special case, translate "any" to "" so the kernel
> +               doesn't have to run strcmp. */
> +               if(!strcmp(info->name, "any"))
> +                       info->name[0] = '\0';
> +
>                 if (invert)
>                         info->invert = 1;
>                 *flags = 1;
> 
> 
> root@quadong# iptables -A POSTROUTING -t mangle -m helper --helper any
> iptables: Invalid argument
> 
> It seems like I'd have to make significantly more invasive changes than
> are really called for to get it to accept an empty string.  What do you
> think?

You just need to remove the check for empty strings in ipt_helper.c:

         /* verify that we actually should match anything */
         if ( strlen(info->name) == 0 )
                 return 0;

Regards
Patrick

> 
> -matthew
> 

[PATCH] matching any helper in ipt_helper.c

[Matthew Strait]

> > It seems like I'd have to make significantly more invasive changes than
> > are really called for to get it to accept an empty string.  What do you
> > think?
>
> You just need to remove the check for empty strings in ipt_helper.c:
>
>         /* verify that we actually should match anything */
>         if ( strlen(info->name) == 0 )
>                 return 0;

Silly me, I assumed that the error was generated in user space.  Ok.  In
that case, let's forget translating "any" to "", because that just makes
the output of "iptables -L" confusing.  Sound good?

Kernel patch:

--- ipt_helper.c.old    2004-03-03 21:34:05.000000000 -0600
+++ ipt_helper.c        2004-03-04 18:38:32.234521176 -0600
@@ -68,8 +68,11 @@
        DEBUGP("master's name = %s , info->name = %s\n",
                exp->expectant->helper->name, info->name);

-       ret = !strncmp(exp->expectant->helper->name, info->name,
-                      strlen(exp->expectant->helper->name)) ^ info->invert;
+       if(info->name[0] == '\0') /* special case meaning "any" */
+               ret = !info->invert;
+       else
+               ret = !strncmp(exp->expectant->helper->name, info->name,
+                              strlen(exp->expectant->helper->name)) ^ info->invert;
 out_unlock:
        READ_UNLOCK(&ip_conntrack_lock);
        return ret;
@@ -89,10 +92,6 @@
        if (matchsize != IPT_ALIGN(sizeof(struct ipt_helper_info)))
                return 0;

-       /* verify that we actually should match anything */
-       if ( strlen(info->name) == 0 )
-               return 0;
-
        return 1;
 }




And documentational changes in iptables:

--- libipt_helper.c.old 2004-03-03 21:39:07.000000000 -0600
+++ libipt_helper.c     2004-03-04 18:31:54.156038304 -0600
@@ -15,6 +15,7 @@
        printf(
 "helper match v%s options:\n"
 "[!] --helper string        Match helper identified by string\n"
+"                           (or any helper if string is \"\")"
 "\n",
 IPTABLES_VERSION);
 }


--- iptables.8.old      2004-03-04 18:35:11.994962216 -0600
+++ iptables.8  2004-03-04 18:34:38.263090240 -0600
@@ -458,6 +458,8 @@
 For other ports append -portnr to the value, ie. "ftp-2121".
 .PP
 Same rules apply for other conntrack-helpers.
+.PP
+If string is "", it will match any packet related to a conntrack-helper.
 .RE
 .SS icmp
 This extension is loaded if `--protocol icmp' is specified.  It

Re: [PATCH] matching any helper in ipt_helper.c

[Patrick McHardy]

Matthew Strait wrote:
> Silly me, I assumed that the error was generated in user space.  Ok.  In
> that case, let's forget translating "any" to "", because that just makes
> the output of "iptables -L" confusing.  Sound good?
>

I actually meant translate in both direction. But no problem, I'm going
to make a patch for iptables myself, if Martin is fine with it we can
remove the childlevel match.

Thanks.

Patrick

Re: [PATCH] matching any helper in ipt_helper.c

[Martin Josefsson]

On Fri, 5 Mar 2004, Patrick McHardy wrote:

> Matthew Strait wrote:
> > Silly me, I assumed that the error was generated in user space.  Ok.  In
> > that case, let's forget translating "any" to "", because that just makes
> > the output of "iptables -L" confusing.  Sound good?
> >
>
> I actually meant translate in both direction. But no problem, I'm going
> to make a patch for iptables myself, if Martin is fine with it we can
> remove the childlevel match.

I'm fine with making ipt_helper able to match any helper if so desired.

/Martin

Re: [PATCH] matching any helper in ipt_helper.c

[Henrik Nordstrom]

On Thu, 4 Mar 2004, Matthew Strait wrote:

> +       if(info->name[0] == '\0') /* special case meaning "any" */
> +               ret = !info->invert;

This also needs to be done earlier in the "no helper" case. You never get
here if there is no helper so ! any is a no-op.

Regards
Henrik

Re: childlevel's pom comment

[Henrik Nordstrom]

On Thu, 4 Mar 2004, Matthew Strait wrote:

> It seems like I'd have to make significantly more invasive changes than
> are really called for to get it to accept an empty string.  What do you
> think?

Not really invasive, but yes some changes is needed in the kernel module 
to relax the input verification. See the check() function.

Regards
Henrik