* Re: [2.6 PATCH] ipvs - properly handle non-linear skbs
2003-10-05 21:42 ` Rusty Russell
@ 2003-10-06 10:23 ` Julian Anastasov
2003-10-06 22:16 ` [2.6 PATCH] ipvs - remove some unused fields from the protocols Julian Anastasov
1 sibling, 0 replies; 7+ messages in thread
From: Julian Anastasov @ 2003-10-06 10:23 UTC (permalink / raw)
To: Rusty Russell; +Cc: David S. Miller, Wensong Zhang, netdev
Hello,
On Mon, 6 Oct 2003, Rusty Russell wrote:
> I diffed our two patches. Is see you still use iph temp vars:
> fair enough: I removed mine after some subtle bugs (although it does
> make the code a bit uglier).
>
> Looks really good. Could you just clarify a couple of things for me please?
>
> @@ -527,10 +528,7 @@ struct ip_vs_conn {
> struct ip_vs_dest *dest; /* real server */
> atomic_t in_pkts; /* incoming packet counter */
>
> - /* packet transmitter for different forwarding methods. If it
> - mangles the packet, it must return NF_DROP or NF_STOLEN, otherwise
> - this must be changed to a sk_buff **.
> - */
> + /* packet transmitter for different forwarding methods */
> int (*packet_xmit)(struct sk_buff *skb, struct ip_vs_conn *cp,
> struct ip_vs_protocol *pp);
>
> This comment is still true: the skb pointer from the caller is
> useless, so xmit *must* return NF_DROP or NF_STOLEN. I thought about
> making it return void and the callers always return NF_STOLEN, but
> there was enough change already. You might want to put a comment in
> there.
My merging mistake, your comment is valid. In this way is
better, sometimes we want to return NF_STOLEN, sometimes NF_DROP.
But it is true that it can be only NF_STOLEN. I made some changes
to return NF_DROP if possible, may be someone wants to log later
these packets as dropped ones.
> ip_vs_make_skb_writable(): how is this different from
> skb_ip_make_writable(), except you have to maintain it yourself? The
> advantage of the general one is that Dave looks after it for us 8)
The main difference is that we should not call skb_copy
for cloned skbs, this is a waste of CPU for allocating new skb.
May be you will change skb_ip_make_writable :)
> In ip_vs_out_icmp():
>
> /* Is the embedded protocol header present? */
> if (unlikely(ciph.frag_off & __constant_htons(IP_OFFSET) &&
> - /* FIXME: Remove minhlen, and surely dont_defrag
> - * test is backwards? --RR */
> (pp->minhlen || pp->dont_defrag)))
> return NF_ACCEPT;
>
> If the protocol says "don't defrag" they never see fragmented packets.
The semantic is: Do not defrag for me, I do not care for my
protocol header, you can give me fragmented skbs. This is true for now
for AH and ESP because we treat these protocols like slave ones, i.e.
UDP 500->500 is a master connection and we forward AH/ESP according
to the main connection ignoring any protocol headers and payloads.
But I'm not sure how that will change in the feature.
> AFAICT, minhlen and dont_defrag are now the same everywhere (minhlen
> is not respected: protocols are expected to catch skb_copy_bits
> failing on their own, and they do). Perhaps drop minhlen altogether,
> and just keep dont_defrag?
Yes, I forgot it. We have to remove minhlen and minhlen_icmp.
It must be: && pp->dont_defrag
> Hey, thanks for doing the hard work for me!
np, tonight I'll cleanup these things and may be other
missed ones.
> Cheers,
> Rusty.
> --
> Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 7+ messages in thread* [2.6 PATCH] ipvs - remove some unused fields from the protocols
2003-10-05 21:42 ` Rusty Russell
2003-10-06 10:23 ` Julian Anastasov
@ 2003-10-06 22:16 ` Julian Anastasov
2003-10-07 10:14 ` Rusty Russell
1 sibling, 1 reply; 7+ messages in thread
From: Julian Anastasov @ 2003-10-06 22:16 UTC (permalink / raw)
To: Rusty Russell; +Cc: David S. Miller, Wensong Zhang, netdev
Hello,
The promised removal of some unused fields. For inclusion after
comments from Rusty. Rusty, it seems we can not convert the transmitters
to NF_STOLEN, ip_vs_null_xmit returns NF_ACCEPT.
Regards,
Julian Anastasov <ja@ssi.bg>
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.1357 -> 1.1358
# net/ipv4/ipvs/ip_vs_core.c 1.4 -> 1.5
# include/net/ip_vs.h 1.5 -> 1.6
# net/ipv4/ipvs/ip_vs_proto_icmp.c 1.2 -> 1.3
# net/ipv4/ipvs/ip_vs_proto_tcp.c 1.3 -> 1.4
# net/ipv4/ipvs/ip_vs_proto_ah.c 1.2 -> 1.3
# net/ipv4/ipvs/ip_vs_proto_esp.c 1.2 -> 1.3
# net/ipv4/ipvs/ip_vs_proto_udp.c 1.3 -> 1.4
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/10/07 ja@ssi.bg 1.1358
# [IPVS]: remove some unused fields from the protocols
# --------------------------------------------
#
diff -Nru a/include/net/ip_vs.h b/include/net/ip_vs.h
--- a/include/net/ip_vs.h Tue Oct 7 01:01:36 2003
+++ b/include/net/ip_vs.h Tue Oct 7 01:01:36 2003
@@ -435,11 +435,7 @@
struct ip_vs_protocol *next;
char *name;
__u16 protocol;
- int minhlen;
- int minhlen_icmp;
int dont_defrag;
- int skip_nonexisting;
- int slave; /* if controlled by others */
atomic_t appcnt; /* counter of proto app incs */
int *timeout_table; /* protocol timeout table */
@@ -528,7 +524,10 @@
struct ip_vs_dest *dest; /* real server */
atomic_t in_pkts; /* incoming packet counter */
- /* packet transmitter for different forwarding methods */
+ /* packet transmitter for different forwarding methods. If it
+ mangles the packet, it must return NF_DROP or NF_STOLEN, otherwise
+ this must be changed to a sk_buff **.
+ */
int (*packet_xmit)(struct sk_buff *skb, struct ip_vs_conn *cp,
struct ip_vs_protocol *pp);
diff -Nru a/net/ipv4/ipvs/ip_vs_core.c b/net/ipv4/ipvs/ip_vs_core.c
--- a/net/ipv4/ipvs/ip_vs_core.c Tue Oct 7 01:01:36 2003
+++ b/net/ipv4/ipvs/ip_vs_core.c Tue Oct 7 01:01:36 2003
@@ -660,7 +660,7 @@
/* Is the embedded protocol header present? */
if (unlikely(ciph.frag_off & __constant_htons(IP_OFFSET) &&
- (pp->minhlen || pp->dont_defrag)))
+ pp->dont_defrag))
return NF_ACCEPT;
IP_VS_DBG_PKT(11, pp, skb, offset, "Checking outgoing ICMP for");
@@ -911,7 +911,7 @@
/* Is the embedded protocol header present? */
if (unlikely(ciph.frag_off & __constant_htons(IP_OFFSET) &&
- (pp->minhlen || pp->dont_defrag)))
+ pp->dont_defrag))
return NF_ACCEPT;
IP_VS_DBG_PKT(11, pp, skb, offset, "Checking incoming ICMP for");
diff -Nru a/net/ipv4/ipvs/ip_vs_proto_ah.c b/net/ipv4/ipvs/ip_vs_proto_ah.c
--- a/net/ipv4/ipvs/ip_vs_proto_ah.c Tue Oct 7 01:01:36 2003
+++ b/net/ipv4/ipvs/ip_vs_proto_ah.c Tue Oct 7 01:01:36 2003
@@ -69,11 +69,11 @@
if (!cp) {
/*
* We are not sure if the packet is from our
- * service, so the caller should check skip_nonexisting
+ * service, so our conn_schedule hook should return NF_ACCEPT
*/
IP_VS_DBG(12, "Unknown ISAKMP entry for outin packet "
"%s%s %u.%u.%u.%u->%u.%u.%u.%u\n",
- inverse?"ICMP+":"",
+ inverse ? "ICMP+" : "",
pp->name,
NIPQUAD(iph->saddr),
NIPQUAD(iph->daddr));
@@ -104,11 +104,6 @@
}
if (!cp) {
- /*
- * We are not sure if the packet is from our
- * service, so the caller should check skip_nonexisting
- * or our conn_schedule hook should return NF_ACCEPT
- */
IP_VS_DBG(12, "Unknown ISAKMP entry for inout packet "
"%s%s %u.%u.%u.%u->%u.%u.%u.%u\n",
inverse ? "ICMP+" : "",
@@ -167,11 +162,7 @@
struct ip_vs_protocol ip_vs_protocol_ah = {
.name = "AH",
.protocol = IPPROTO_AH,
- .minhlen = 0,
- .minhlen_icmp = 0,
.dont_defrag = 1,
- .skip_nonexisting = 1,
- .slave = 1,
.init = ah_init,
.exit = ah_exit,
.conn_schedule = ah_conn_schedule,
@@ -179,11 +170,11 @@
.conn_out_get = ah_conn_out_get,
.snat_handler = NULL,
.dnat_handler = NULL,
+ .csum_check = NULL,
.state_transition = NULL,
.register_app = NULL,
.unregister_app = NULL,
.app_conn_bind = NULL,
- .csum_check = NULL,
.debug_packet = ah_debug_packet,
.timeout_change = NULL, /* ISAKMP */
.set_state_timeout = NULL,
diff -Nru a/net/ipv4/ipvs/ip_vs_proto_esp.c b/net/ipv4/ipvs/ip_vs_proto_esp.c
--- a/net/ipv4/ipvs/ip_vs_proto_esp.c Tue Oct 7 01:01:36 2003
+++ b/net/ipv4/ipvs/ip_vs_proto_esp.c Tue Oct 7 01:01:36 2003
@@ -69,7 +69,7 @@
if (!cp) {
/*
* We are not sure if the packet is from our
- * service, so the caller should check skip_nonexisting
+ * service, so our conn_schedule hook should return NF_ACCEPT
*/
IP_VS_DBG(12, "Unknown ISAKMP entry for outin packet "
"%s%s %u.%u.%u.%u->%u.%u.%u.%u\n",
@@ -104,14 +104,9 @@
}
if (!cp) {
- /*
- * We are not sure if the packet is from our
- * service, so the caller should check skip_nonexisting
- * or our conn_schedule hook should return NF_ACCEPT
- */
IP_VS_DBG(12, "Unknown ISAKMP entry for inout packet "
"%s%s %u.%u.%u.%u->%u.%u.%u.%u\n",
- inverse?"ICMP+":"",
+ inverse ? "ICMP+" : "",
pp->name,
NIPQUAD(iph->saddr),
NIPQUAD(iph->daddr));
@@ -166,11 +161,7 @@
struct ip_vs_protocol ip_vs_protocol_esp = {
.name = "ESP",
.protocol = IPPROTO_ESP,
- .minhlen = 0,
- .minhlen_icmp = 0,
.dont_defrag = 1,
- .skip_nonexisting = 1,
- .slave = 1,
.init = esp_init,
.exit = esp_exit,
.conn_schedule = esp_conn_schedule,
diff -Nru a/net/ipv4/ipvs/ip_vs_proto_icmp.c b/net/ipv4/ipvs/ip_vs_proto_icmp.c
--- a/net/ipv4/ipvs/ip_vs_proto_icmp.c Tue Oct 7 01:01:36 2003
+++ b/net/ipv4/ipvs/ip_vs_proto_icmp.c Tue Oct 7 01:01:36 2003
@@ -163,11 +163,7 @@
struct ip_vs_protocol ip_vs_protocol_icmp = {
.name = "ICMP",
.protocol = IPPROTO_ICMP,
- .minhlen = sizeof(struct icmphdr),
- .minhlen_icmp = 8,
.dont_defrag = 0,
- .skip_nonexisting = 0,
- .slave = 0,
.init = icmp_init,
.exit = icmp_exit,
.conn_schedule = icmp_conn_schedule,
diff -Nru a/net/ipv4/ipvs/ip_vs_proto_tcp.c b/net/ipv4/ipvs/ip_vs_proto_tcp.c
--- a/net/ipv4/ipvs/ip_vs_proto_tcp.c Tue Oct 7 01:01:36 2003
+++ b/net/ipv4/ipvs/ip_vs_proto_tcp.c Tue Oct 7 01:01:36 2003
@@ -132,7 +132,7 @@
if (unlikely(cp->app != NULL)) {
/* Some checks before mangling */
- if (pp->csum_check && !pp->slave && !pp->csum_check(*pskb, pp))
+ if (pp->csum_check && !pp->csum_check(*pskb, pp))
return 0;
/* Call application helper if needed */
@@ -180,7 +180,7 @@
if (unlikely(cp->app != NULL)) {
/* Some checks before mangling */
- if (pp->csum_check && !pp->slave && !pp->csum_check(*pskb, pp))
+ if (pp->csum_check && !pp->csum_check(*pskb, pp))
return 0;
/*
@@ -614,11 +614,7 @@
struct ip_vs_protocol ip_vs_protocol_tcp = {
.name = "TCP",
.protocol = IPPROTO_TCP,
- .minhlen = sizeof(struct tcphdr),
- .minhlen_icmp = 8,
.dont_defrag = 0,
- .skip_nonexisting = 0,
- .slave = 0,
.appcnt = ATOMIC_INIT(0),
.init = tcp_init,
.exit = tcp_exit,
diff -Nru a/net/ipv4/ipvs/ip_vs_proto_udp.c b/net/ipv4/ipvs/ip_vs_proto_udp.c
--- a/net/ipv4/ipvs/ip_vs_proto_udp.c Tue Oct 7 01:01:36 2003
+++ b/net/ipv4/ipvs/ip_vs_proto_udp.c Tue Oct 7 01:01:36 2003
@@ -133,7 +133,7 @@
if (unlikely(cp->app != NULL)) {
/* Some checks before mangling */
- if (pp->csum_check && !pp->slave && !pp->csum_check(*pskb, pp))
+ if (pp->csum_check && !pp->csum_check(*pskb, pp))
return 0;
/*
@@ -187,7 +187,7 @@
if (unlikely(cp->app != NULL)) {
/* Some checks before mangling */
- if (pp->csum_check && !pp->slave && !pp->csum_check(*pskb, pp))
+ if (pp->csum_check && !pp->csum_check(*pskb, pp))
return 0;
/*
@@ -401,11 +401,7 @@
struct ip_vs_protocol ip_vs_protocol_udp = {
.name = "UDP",
.protocol = IPPROTO_UDP,
- .minhlen = sizeof(struct udphdr),
- .minhlen_icmp = 8,
.dont_defrag = 0,
- .skip_nonexisting = 0,
- .slave = 0,
.init = udp_init,
.exit = udp_exit,
.conn_schedule = udp_conn_schedule,
^ permalink raw reply [flat|nested] 7+ messages in thread