Patchwork [1/3] xtables: tarpit: Make tarpit code generic

login
register
mail settings
Submitter Josh Hunt
Date July 7, 2012, 9:20 p.m.
Message ID <1341696061-21863-2-git-send-email-johunt@akamai.com>
Download mbox | patch
Permalink /patch/169616/
State Superseded
Headers show

Comments

Josh Hunt - July 7, 2012, 9:20 p.m.
Moves TCP header manipulation into a separate function to allow code-sharing for
IPv6 support.

Signed-off-by: Josh Hunt <johunt@akamai.com>
---
 extensions/xt_TARPIT.c |  133 +++++++++++++++++++++++++-----------------------
 1 files changed, 69 insertions(+), 64 deletions(-)
Jan Engelhardt - July 8, 2012, 12:58 p.m.
On Saturday 2012-07-07 23:20, Josh Hunt wrote:

>Moves TCP header manipulation into a separate function to allow code-sharing for
>IPv6 support.

Hi Josh,


in your patch, you seem to oversee that the tarpit_generic block that
you are moving into a new function had "return;"s to exit from
the tarpit_tcp function. With your change,

>+	niph->tot_len = htons(nskb->len);
>+	tcph->urg_ptr = 0;
>+	/* Reset flags */
>+	((u_int8_t *)tcph)[13] = 0;
>+
>+	tarpit_generic(oth, tcph, payload, mode);
> 
> 	/* Adjust TCP checksum */
> 	tcph->check = 0;

The checksum code (and what follows) it will be executed in all cases,
even though TCP RST are supposed not to get any reply per the comment.

tarpit_generic would have to return a bool value propagating this
exit style. In other words,


static bool tarpit_generic
{
	if (mode == XTTARPIT_TARPIT)
		/* No replies for RST, FIN , ... */
		if (oth->rst ...)
			return false;
	}
	...
	return true;
}

static void tarpit_tcp
{
	...
	if (!tarpit_generic())
		return;
	tcph->check = 0;
	...
}

It would also be appreciated if you could move each case
(XTTARPIT_{TARPIT,HONEYPOT,RESET}) into their own functions in three
patches added to the front of the set.
(Reduces indent and makes the diffs simpler.)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josh Hunt - July 8, 2012, 1:58 p.m.
On 07/08/2012 07:58 AM, Jan Engelhardt wrote:
>
> On Saturday 2012-07-07 23:20, Josh Hunt wrote:
>
>> Moves TCP header manipulation into a separate function to allow code-sharing for
>> IPv6 support.
>
> Hi Josh,
>
>
> in your patch, you seem to oversee that the tarpit_generic block that
> you are moving into a new function had "return;"s to exit from
> the tarpit_tcp function. With your change,
>
>> +	niph->tot_len = htons(nskb->len);
>> +	tcph->urg_ptr = 0;
>> +	/* Reset flags */
>> +	((u_int8_t *)tcph)[13] = 0;
>> +
>> +	tarpit_generic(oth, tcph, payload, mode);
>>
>> 	/* Adjust TCP checksum */
>> 	tcph->check = 0;
>
> The checksum code (and what follows) it will be executed in all cases,
> even though TCP RST are supposed not to get any reply per the comment.
>
> tarpit_generic would have to return a bool value propagating this
> exit style. In other words,
>
>
> static bool tarpit_generic
> {
> 	if (mode == XTTARPIT_TARPIT)
> 		/* No replies for RST, FIN , ... */
> 		if (oth->rst ...)
> 			return false;
> 	}
> 	...
> 	return true;
> }
>
> static void tarpit_tcp
> {
> 	...
> 	if (!tarpit_generic())
> 		return;
> 	tcph->check = 0;
> 	...
> }

Doh! Yep totally overlooked that. Thanks for catching. Will update and 
resend.

>
> It would also be appreciated if you could move each case
> (XTTARPIT_{TARPIT,HONEYPOT,RESET}) into their own functions in three
> patches added to the front of the set.
> (Reduces indent and makes the diffs simpler.)
>

Sure. I'll do this as well.

Thanks for the review.

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

Patch

diff --git a/extensions/xt_TARPIT.c b/extensions/xt_TARPIT.c
index db24f90..9a09e75 100644
--- a/extensions/xt_TARPIT.c
+++ b/extensions/xt_TARPIT.c
@@ -51,70 +51,7 @@ 
 #include "compat_xtables.h"
 #include "xt_TARPIT.h"
 
-static void tarpit_tcp(struct sk_buff *oldskb, unsigned int hook,
-    unsigned int mode)
-{
-	struct tcphdr _otcph, *oth, *tcph;
-	unsigned int addr_type = RTN_UNSPEC;
-	struct sk_buff *nskb;
-	const struct iphdr *oldhdr;
-	struct iphdr *niph;
-	uint16_t tmp, payload;
-
-	/* A truncated TCP header is not going to be useful */
-	if (oldskb->len < ip_hdrlen(oldskb) + sizeof(struct tcphdr))
-		return;
-
-	oth = skb_header_pointer(oldskb, ip_hdrlen(oldskb),
-	                         sizeof(_otcph), &_otcph);
-	if (oth == NULL)
-		return;
-
-	/* Check checksum. */
-	if (nf_ip_checksum(oldskb, hook, ip_hdrlen(oldskb), IPPROTO_TCP))
-		return;
-
-	/*
-	 * Copy skb (even if skb is about to be dropped, we cannot just
-	 * clone it because there may be other things, such as tcpdump,
-	 * interested in it)
-	 */
-	nskb = skb_copy_expand(oldskb, LL_MAX_HEADER,
-	                       skb_tailroom(oldskb), GFP_ATOMIC);
-	if (nskb == NULL)
-		return;
-
-	/* This packet will not be the same as the other: clear nf fields */
-	nf_reset(nskb);
-	skb_nfmark(nskb) = 0;
-	skb_init_secmark(nskb);
-
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 18)
-	skb_shinfo(nskb)->gso_size = 0;
-	skb_shinfo(nskb)->gso_segs = 0;
-	skb_shinfo(nskb)->gso_type = 0;
-#endif
-
-	oldhdr = ip_hdr(oldskb);
-	tcph = (struct tcphdr *)(skb_network_header(nskb) + ip_hdrlen(nskb));
-
-	/* Swap source and dest */
-	niph         = ip_hdr(nskb);
-	niph->daddr  = xchg(&niph->saddr, niph->daddr);
-	tmp          = tcph->source;
-	tcph->source = tcph->dest;
-	tcph->dest   = tmp;
-
-	/* Calculate payload size?? */
-	payload = nskb->len - ip_hdrlen(nskb) - sizeof(struct tcphdr);
-
-	/* Truncate to length (no data) */
-	tcph->doff    = sizeof(struct tcphdr) / 4;
-	skb_trim(nskb, ip_hdrlen(nskb) + sizeof(struct tcphdr));
-	niph->tot_len = htons(nskb->len);
-	tcph->urg_ptr = 0;
-	/* Reset flags */
-	((u_int8_t *)tcph)[13] = 0;
+static void tarpit_generic(struct tcphdr *oth, struct tcphdr *tcph, uint16_t payload, unsigned int mode) {
 
 	if (mode == XTTARPIT_TARPIT) {
 		/* No replies for RST, FIN or !SYN,!ACK */
@@ -194,6 +131,74 @@  static void tarpit_tcp(struct sk_buff *oldskb, unsigned int hook,
 		tcph->seq     = oth->ack_seq;
 		tcph->ack_seq = oth->seq;
 	}
+}
+
+static void tarpit_tcp(struct sk_buff *oldskb, unsigned int hook,
+    unsigned int mode)
+{
+	struct tcphdr _otcph, *oth, *tcph;
+	unsigned int addr_type = RTN_UNSPEC;
+	struct sk_buff *nskb;
+	const struct iphdr *oldhdr;
+	struct iphdr *niph;
+	uint16_t tmp, payload;
+
+	/* A truncated TCP header is not going to be useful */
+	if (oldskb->len < ip_hdrlen(oldskb) + sizeof(struct tcphdr))
+		return;
+
+	oth = skb_header_pointer(oldskb, ip_hdrlen(oldskb),
+	                         sizeof(_otcph), &_otcph);
+	if (oth == NULL)
+		return;
+
+	/* Check checksum. */
+	if (nf_ip_checksum(oldskb, hook, ip_hdrlen(oldskb), IPPROTO_TCP))
+		return;
+
+	/*
+	 * Copy skb (even if skb is about to be dropped, we cannot just
+	 * clone it because there may be other things, such as tcpdump,
+	 * interested in it)
+	 */
+	nskb = skb_copy_expand(oldskb, LL_MAX_HEADER,
+	                       skb_tailroom(oldskb), GFP_ATOMIC);
+	if (nskb == NULL)
+		return;
+
+	/* This packet will not be the same as the other: clear nf fields */
+	nf_reset(nskb);
+	skb_nfmark(nskb) = 0;
+	skb_init_secmark(nskb);
+
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 18)
+	skb_shinfo(nskb)->gso_size = 0;
+	skb_shinfo(nskb)->gso_segs = 0;
+	skb_shinfo(nskb)->gso_type = 0;
+#endif
+
+	oldhdr = ip_hdr(oldskb);
+	tcph = (struct tcphdr *)(skb_network_header(nskb) + ip_hdrlen(nskb));
+
+	/* Swap source and dest */
+	niph         = ip_hdr(nskb);
+	niph->daddr  = xchg(&niph->saddr, niph->daddr);
+	tmp          = tcph->source;
+	tcph->source = tcph->dest;
+	tcph->dest   = tmp;
+
+	/* Calculate payload size?? */
+	payload = nskb->len - ip_hdrlen(nskb) - sizeof(struct tcphdr);
+
+	/* Truncate to length (no data) */
+	tcph->doff    = sizeof(struct tcphdr) / 4;
+	skb_trim(nskb, ip_hdrlen(nskb) + sizeof(struct tcphdr));
+	niph->tot_len = htons(nskb->len);
+	tcph->urg_ptr = 0;
+	/* Reset flags */
+	((u_int8_t *)tcph)[13] = 0;
+
+	tarpit_generic(oth, tcph, payload, mode);
 
 	/* Adjust TCP checksum */
 	tcph->check = 0;