Message-ID: <4BB48983.909@trash.net>
Date:	Thu, 01 Apr 2010 13:54:43 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Jan Engelhardt <jengelh@...ozas.de>
CC:	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 3/5] netfilter: xtables: inclusion of xt_TEE

Jan Engelhardt wrote:
> On Thursday 2010-04-01 12:34, Patrick McHardy wrote:
>>> +static bool
>>> +tee_tg_route4(struct sk_buff *skb, const struct xt_tee_tginfo *info)
>>> +{
>>> +	const struct iphdr *iph = ip_hdr(skb);
>>> +	struct rtable *rt;
>>> +	struct flowi fl;
>>> +	int err;
>>> +
>>> +	memset(&fl, 0, sizeof(fl));
>>> +	fl.iif  = skb->skb_iif;
>> I'm not sure you really should set iif here. We usually (tunnels, REJECT
>> etc) packets generated locally as new packets.
>>> +	fl.mark = skb->mark;
>> The same applies to mark.
> 
> If you use TEE in PREROUTING or INPUT, teeing acts more like FORWARD than
> OUTPUT, though. All TEE does is lookup a route to a new fl.dst, but it keeps
> the original src address in fl.src, so if somebody has some source-based policy
> routing, it could suddenly behave different. What do you think?

That might make it unnessarily complicated to use src-based routing
when using TEE. I guess you'd usually have a host for logging or IDS
somewhere on a private network and TEE packets there. So specifying
oif and gateway seems most useful to me.

>>> +/*
>>> + * To detect and deter routed packet loopback when using the --tee option, we
>>> + * take a page out of the raw.patch book: on the copied skb, we set up a fake
>>> + * ->nfct entry, pointing to the local &route_tee_track. We skip routing
>>> + * packets when we see they already have that ->nfct.
>> So without conntrack, people may create loops? If that's the case,
>> I'd suggest to simply forbid TEE'ing packets to loopback. That
>> doesn't seem to be very useful anyways.
> 
>>> +#ifdef WITH_CONNTRACK
>>> +	if (skb->nfct == &tee_track.ct_general)
>>> +		/*
>>> +		 * Loopback - a packet we already routed, is to be
>>> +		 * routed another time. Avoid that, now.
>>> +		 */
> 	printk("loopback - dropped\n");
>>> +		return NF_DROP;
>>> +#endif
> 
> We are looking at a historic piece of code - and comments, which
> traces back to when xt_NOTRACK was still in POM.
> 
> {
>     →   /* Previously seen (loopback)? Ignore. */
>     →   if ((*pskb)->nfct != NULL)
>     →       →   return IPT_CONTINUE;
> 
>     →   /* Attach fake conntrack entry.·
>     →      If there is a real ct entry correspondig to this packet,·
>     →      it'll hang aroun till timing out. We don't deal with it
>     →      for performance reasons. JK */
>     →   (*pskb)->nfct = &ip_conntrack_untracked.infos[IP_CT_NEW];
>     →   nf_conntrack_get((*pskb)->nfct);
> 
>     →   return IPT_CONTINUE;
> }
> 
> Let's look at the condition "skb->nfct == &tee_track.ct_general" in detail. An
> skb can only already have tee_track when it has been teed.
> 
> The teed packet however never traversed Xtables at all. Of course that changes
> once the nesting patch is applied. But was someone really thinking of this, 6
> years ago?
> 
> That actually made me wonder and dig in history, and it turns out that
> ipt_ROUTE allowed the packet to be fed back into netif_rx (commit
> bee4e80167e3d024bdb80f400f4ecc8de47cfb03 in pom-ng.git), which would
> explain all the loopback stuff. Since modern xt_TEE does not do
> that evil thing, the comment is a walnut-hard remainder of past times.
> 
> I shall remove it now that it has been spotted.

Yeah, but currently it does allow packets to be looped back. These
packets will also go through the netfilter hooks again.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists