All of lore.kernel.org
 help / color / mirror / Atom feed
* Reviewing a piece of code, locking questions
@ 2004-05-28 21:46 Matthias Dettling
  2004-05-28 22:59 ` Henrik Nordstrom
  0 siblings, 1 reply; 7+ messages in thread
From: Matthias Dettling @ 2004-05-28 21:46 UTC (permalink / raw)
  To: netfilter-devel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2004-06-08 18:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-05-28 21:46 Reviewing a piece of code, locking questions Matthias Dettling
2004-05-28 22:59 ` Henrik Nordstrom
2004-05-30 23:33   ` Matthias Dettling
2004-05-31  8:36     ` Henrik Nordstrom
2004-06-08 17:31       ` conntrack matching Matthias Dettling
2004-06-08 18:03         ` Henrik Nordstrom
2004-06-08 18:21           ` Peter Stamfest

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.