Reviewing a piece of code, locking questions

Reviewing a piece of code, locking questions

[Matthias Dettling]

Hello developers,

I have modified a patched version of "ipt_conntrack.c" for matching 
packets on the informations of a related connection that were tracked 
before. The following is the most important piece of my modification in 
the function "match".
There is to remark that the flag fields were extended to "u_int32_t" to 
take up additional flags (IPT_CONNTRACK_RELORIGSRCPORT, 
IPT_CONNTRACK_RELORIGDSTPORT, IPT_CONNTRACK_RELREPLSRCPORT, 
IPT_CONNTRACK_RELREPLDSTPORT).

modifications "ipt_conntrack.c":
static int match(.....)
{
....
/* 
###################################################################################### 
*/
    if(sinfo->flags & IPT_CONNTRACK_RELORIGSRCPORT) {
        READ_LOCK(&ip_conntrack_lock);
       
        if (!ct)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master->expectant || 
FWINV(!port_match(sinfo->rel_srcports[IP_CT_DIR_ORIGINAL][0], \ 
sinfo->rel_srcports[IP_CT_DIR_ORIGINAL][1], \ 
ntohs(ct->master->expectant->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.tcp.port)), 
\ IPT_CONNTRACK_RELORIGSRCPORT))
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        READ_UNLOCK(&ip_conntrack_lock);
    }

    if(sinfo->flags & IPT_CONNTRACK_RELORIGDSTPORT) {
        READ_LOCK(&ip_conntrack_lock);
       
        if (!ct)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master->expectant || 
FWINV(!port_match(sinfo->rel_dstports[IP_CT_DIR_ORIGINAL][0], \ 
sinfo->rel_dstports[IP_CT_DIR_ORIGINAL][1], \ 
ntohs(ct->master->expectant->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.u.tcp.port)), 
\ IPT_CONNTRACK_RELORIGDSTPORT))
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        READ_UNLOCK(&ip_conntrack_lock);
    }

    if(sinfo->flags & IPT_CONNTRACK_RELREPLSRCPORT) {
        READ_LOCK(&ip_conntrack_lock);
       
        if (!ct)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master->expectant || 
FWINV(!port_match(sinfo->rel_srcports[IP_CT_DIR_REPLY][0], \ 
sinfo->rel_srcports[IP_CT_DIR_REPLY][1], \ 
ntohs(ct->master->expectant->tuplehash[IP_CT_DIR_REPLY].tuple.src.u.tcp.port)), 
\ IPT_CONNTRACK_RELREPLSRCPORT))
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        READ_UNLOCK(&ip_conntrack_lock);
    }

    if(sinfo->flags & IPT_CONNTRACK_RELREPLDSTPORT) {
        READ_LOCK(&ip_conntrack_lock);
       
        if (!ct)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master)
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        if (!ct->master->expectant || 
FWINV(!port_match(sinfo->rel_dstports[IP_CT_DIR_REPLY][0], \ 
sinfo->rel_dstports[IP_CT_DIR_REPLY][1], \ 
ntohs(ct->master->expectant->tuplehash[IP_CT_DIR_REPLY].tuple.dst.u.tcp.port)), 
\ IPT_CONNTRACK_RELREPLDSTPORT))
        {
            READ_UNLOCK(&ip_conntrack_lock);
            return 0;
        }

        READ_UNLOCK(&ip_conntrack_lock);
    }
/* 
###################################################################################### 
*/
....
}

modifications "ipt_conntrack.h":
/* 
###################################################################################### 
*/
#define IPT_CONNTRACK_RELORIGSRCPORT    0x00001000
#define IPT_CONNTRACK_RELORIGDSTPORT    0x00002000
#define IPT_CONNTRACK_RELREPLSRCPORT    0x00004000
#define IPT_CONNTRACK_RELREPLDSTPORT    0x00008000
/* 
###################################################################################### 
*/
......
struct ipt_conntrack_info
{
......
/* 
###################################################################################### 
*/
    /* Ports of the master connection, if existent */
    u_int16_t rel_srcports[IP_CT_DIR_MAX][2];
    u_int16_t rel_dstports[IP_CT_DIR_MAX][2];
/* 
###################################################################################### 
*/
......
};

Can someone look at the code to see if there are any mistakes? 
Especially the parts with locking should be reviewed because I am really 
unsure if I have locked properly. Do I need a "WRITE_LOCK" or only a 
"READ_LOCK", is "&ip_conntrack_lock" right or need I 
"&ip_conntrack_expect_tuple" or even some other locking?
This is very important because I have no SMP machine to test the code there.
For me on my i386 it works without problems.

I didn't post the userspace code because this is nearly straightforward 
and I'm almost sure that it is correct.
If it is required I will of course post the whole code.

An other question I have is if it is legal to modifiy the structure 
"ipt_conntrack_info" in "ipt_conntrack.h" or could this cause problems 
or errors?
I have added 2 arrays with the ports of the related connection for the 
matching as also extended the flag fields (see above).

If all should be OK, how can I publish the modification, that it could 
be part of later versions of iptables and the kernel, because I think it 
could be interesting for others too?

Best regards
Matthias Dettling

Re: Reviewing a piece of code, locking questions

[Henrik Nordstrom]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1827 bytes --]

On Fri, 28 May 2004, Matthias Dettling wrote:

> Can someone look at the code to see if there are any mistakes? 
> Especially the parts with locking should be reviewed because I am really 
> unsure if I have locked properly. Do I need a "WRITE_LOCK" or only a 
> "READ_LOCK", is "&ip_conntrack_lock" right or need I 
> "&ip_conntrack_expect_tuple" or even some other locking?

No obvious errors from what I could tell.

You need a READ_LOCK.

WRITE_LOCK is only needed if you are to modify the ip_conntrack table, 
which you are obviously not doing.

> An other question I have is if it is legal to modifiy the structure 
> "ipt_conntrack_info" in "ipt_conntrack.h" or could this cause problems 
> or errors?

It is legal, but will invalidate binary compatibility with existing 
iptables binaries having another libipt_conntrack compiled..

> I have added 2 arrays with the ports of the related connection for the 
> matching as also extended the flag fields (see above).

To simplify the kernel code you could store the ports in network byte 
order in the match structure.. this way you do not need to translate the 
port fields while matching in the kernel.

> If all should be OK, how can I publish the modification, that it could 
> be part of later versions of iptables and the kernel, because I think it 
> could be interesting for others too?

As a tarball with the files to go into patch-o-matic-ng. This includes 
both the patching to the kernel and userspace.

  conntrack_port/	(or whatever you want to name your patch)
	linux.patch	kernel changes
	iptables.patch	userspace changes
	info		meta info describing your patch
	help		short blurb documenting your patch

While you are at it you may want to also merge the other conntrack matches 
I posted a long time ago.. [attached as a conntrack2 match]

Regards
Henrik

[-- Attachment #2: Type: APPLICATION/x-gzip, Size: 10240 bytes --]

Re: Reviewing a piece of code, locking questions

[Matthias Dettling]

Henrik Nordstrom wrote:

>On Fri, 28 May 2004, Matthias Dettling wrote:
>
>  
>
>>Can someone look at the code to see if there are any mistakes? 
>>Especially the parts with locking should be reviewed because I am really 
>>unsure if I have locked properly. Do I need a "WRITE_LOCK" or only a 
>>"READ_LOCK", is "&ip_conntrack_lock" right or need I 
>>"&ip_conntrack_expect_tuple" or even some other locking?
>>    
>>
>
>No obvious errors from what I could tell.
>
>You need a READ_LOCK.
>
>WRITE_LOCK is only needed if you are to modify the ip_conntrack table, 
>which you are obviously not doing.
>
>  
>
>>An other question I have is if it is legal to modifiy the structure 
>>"ipt_conntrack_info" in "ipt_conntrack.h" or could this cause problems 
>>or errors?
>>    
>>
>
>It is legal, but will invalidate binary compatibility with existing 
>iptables binaries having another libipt_conntrack compiled..
>
>  
>
>>I have added 2 arrays with the ports of the related connection for the 
>>matching as also extended the flag fields (see above).
>>    
>>
>
>To simplify the kernel code you could store the ports in network byte 
>order in the match structure.. this way you do not need to translate the 
>port fields while matching in the kernel.
>
>  
>
>>If all should be OK, how can I publish the modification, that it could 
>>be part of later versions of iptables and the kernel, because I think it 
>>could be interesting for others too?
>>    
>>
>
>As a tarball with the files to go into patch-o-matic-ng. This includes 
>both the patching to the kernel and userspace.
>
>  conntrack_port/	(or whatever you want to name your patch)
>	linux.patch	kernel changes
>	iptables.patch	userspace changes
>	info		meta info describing your patch
>	help		short blurb documenting your patch
>
>While you are at it you may want to also merge the other conntrack matches 
>I posted a long time ago.. [attached as a conntrack2 match]
>
>Regards
>Henrik
>  
>
Hello Henrik,

many thanks for your help.
This is what I meant, when saying "a patched version" of ipt_conntrack.c.
I found your patch on the net and applied it. So my work was only 
extending ipt_conntrack.c for matching tracked informations.
This means that my work depends on your patch because I use the same 
functions as your code does. I looked in the current CVS version of 
patch-o-matic about your patch as also in the file 
patch-o-matic-ng-20040302.tar.bz2 but didn't find it.
Because my work needs your patch am I allowed to add your patch to my 
patch (of course I would mention your name)?

You gave me the hint for simplifying kernel code storing the port 
numbers in network byte order. If I'm right I think that you don't do so 
in your patch. Because my work depends strongly on your it would be 
awkward to do so. But if it is really better for performance I could 
change the whole code in this way.

If I have created a patch as tarball where should I send this that it 
will be included in future releases of patch-o-matic?

Best regards
Matthias

Re: Reviewing a piece of code, locking questions

[Henrik Nordstrom]

On Mon, 31 May 2004, Matthias Dettling wrote:

> many thanks for your help.
> This is what I meant, when saying "a patched version" of ipt_conntrack.c.
> I found your patch on the net and applied it. So my work was only 
> extending ipt_conntrack.c for matching tracked informations.

Is this the old patch where I add port matches to ipt_conntrack?

> This means that my work depends on your patch because I use the same 
> functions as your code does. I looked in the current CVS version of 
> patch-o-matic about your patch as also in the file 
> patch-o-matic-ng-20040302.tar.bz2 but didn't find it.

I never got any response from the ipt_conntrack author on these changes.

> Because my work needs your patch am I allowed to add your patch to my 
> patch (of course I would mention your name)?

Yes.

> You gave me the hint for simplifying kernel code storing the port 
> numbers in network byte order. If I'm right I think that you don't do so 
> in your patch. Because my work depends strongly on your it would be 
> awkward to do so. But if it is really better for performance I could 
> change the whole code in this way.

If I don't do so then please change my patch by moving the ntohs calls to 
libipt_conntrack.

> If I have created a patch as tarball where should I send this that it 
> will be included in future releases of patch-o-matic?

Here to the mailinglist.

Regards
Henrik

conntrack matching

[Matthias Dettling]

Henrik Nordstrom wrote:

>On Mon, 31 May 2004, Matthias Dettling wrote:
>
>  
>
>>many thanks for your help.
>>This is what I meant, when saying "a patched version" of ipt_conntrack.c.
>>I found your patch on the net and applied it. So my work was only 
>>extending ipt_conntrack.c for matching tracked informations.
>>    
>>
>
>Is this the old patch where I add port matches to ipt_conntrack?
>
>  
>
>>This means that my work depends on your patch because I use the same 
>>functions as your code does. I looked in the current CVS version of 
>>patch-o-matic about your patch as also in the file 
>>patch-o-matic-ng-20040302.tar.bz2 but didn't find it.
>>    
>>
>
>I never got any response from the ipt_conntrack author on these changes.
>
>  
>
>>Because my work needs your patch am I allowed to add your patch to my 
>>patch (of course I would mention your name)?
>>    
>>
>
>Yes.
>
>  
>
>>You gave me the hint for simplifying kernel code storing the port 
>>numbers in network byte order. If I'm right I think that you don't do so 
>>in your patch. Because my work depends strongly on your it would be 
>>awkward to do so. But if it is really better for performance I could 
>>change the whole code in this way.
>>    
>>
>
>If I don't do so then please change my patch by moving the ntohs calls to 
>libipt_conntrack.
>
>  
>
>>If I have created a patch as tarball where should I send this that it 
>>will be included in future releases of patch-o-matic?
>>    
>>
>
>Here to the mailinglist.
>
>Regards
>Henrik
>
>
>  
>
Hello Henrik,

last week I hadn't much time for answering your mail.
But meantime I thought a while about your recommendation to match 
numbers in network byte order.
I think this proposal would be appropriate, if I only would match a 
single port.
But like your patch I also want to match port ranges. And that's the 
problem.
A single compare of two values is independent of the byte order of the 
machine.
It can result either in true or in false (all the same compared by left 
or by right).
A check if a given value is between a range of values needs arithmetic 
operations (>=, <=) which are in my opinion
dependant of the machine's architecture (big-endian or little-endian).
So I think this is the reason why you are also matching in host byte 
order in your patch.
An other solution were ugly #ifdef constructions (to determine wether 
big-endian or little-endian) and then using the
arithmetic operations dependant of the machine's architecture, but I 
think this is bad style.
So I think the way of translating the port numbers from network byte 
order to host byte order in the kernel for matching them
is the best compromise of performance and clean code.
In my opinion your idea to move ntohs calls to libipt_conntrack doesn't 
make sense, because the user input is already in
host byte order. The only possibility would be to use htons in 
libipt_conntrack an then matching in network byte order, but then the 
problems described above occur.
Am I right in this assumptions, what do you think?

Regards
Matthias

Re: conntrack matching

[Henrik Nordstrom]

On Tue, 8 Jun 2004, Matthias Dettling wrote:

> last week I hadn't much time for answering your mail. But meantime I
> thought a while about your recommendation to match numbers in network
> byte order. I think this proposal would be appropriate, if I only would
> match a single port. But like your patch I also want to match port
> ranges. And that's the problem.

Indeed. If you want to match ranges then matching needs to be done in host 
byte order, not network byte order.

Only for exact matches is there a benefit of having the match data in 
network byte order.

Note that I never meant to move the ntohs functions to userspace. What
would be used in userspace is htons to translate the user input to network 
byte order for matching to the packets in the kernel.

Regards
Henrik

Re: conntrack matching

[Peter Stamfest]

On Tue, 8 Jun 2004, Henrik Nordstrom wrote:

> Date: Tue, 8 Jun 2004 20:03:33 +0200 (CEST)
> From: Henrik Nordstrom <hno@marasystems.com>
> To: Matthias Dettling <m-dettling@gmx.de>
> Cc: Netfilter Developers List <netfilter-devel@lists.netfilter.org>
> Subject: Re: conntrack matching
> 
> On Tue, 8 Jun 2004, Matthias Dettling wrote:
> 
> > last week I hadn't much time for answering your mail. But meantime I
> > thought a while about your recommendation to match numbers in network
> > byte order. I think this proposal would be appropriate, if I only would
> > match a single port. But like your patch I also want to match port
> > ranges. And that's the problem.
> 
> Indeed. If you want to match ranges then matching needs to be done in host 
> byte order, not network byte order.

Funny, my table match module I posted a couple of days ago has the same
"problem" - there Network Adresses have to be compared in host byte order
because it deals with ranges of IP adresses. As always, it is just a
matter of what is more appropriate for the task. Comparing adress ranges
only gets "complicated" with IPv6 Adresses - 128bit Architectures
pending...
 
peter