* [RFC PATCH] ipvs: skb defrag for L7 helpers @ 2010-11-08 14:51 Hans Schillstrom 2010-11-08 21:43 ` Simon Horman 2010-11-08 21:48 ` Julian Anastasov 0 siblings, 2 replies; 8+ messages in thread From: Hans Schillstrom @ 2010-11-08 14:51 UTC (permalink / raw) To: Simon Horman; +Cc: Julian Anastasov, LVS-Devel Hello I have been struggling with SIP for a while .... L7 helpers like sip needs skb defrag ex virtio only copies the first 128 byte into the skb (incl mac hdr) in that case Call-Id will never be found. There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c index e99f920..c0ac69a 100644 --- a/net/netfilter/ipvs/ip_vs_pe.c +++ b/net/netfilter/ipvs/ip_vs_pe.c @@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) return pe; } +/* skb defrag for L7 helpers */ +char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) +{ + char *p = kmalloc(skb->len, GFP_ATOMIC); + if (!p) + goto err; + if (skb_copy_bits(skb, offset, p, len)) + goto err; + IP_VS_DBG(10, "IPVS defrag: offs:%d len:%d\n", offset, len); + return p; + +err: + if (p) + kfree(p); + return NULL; +} +EXPORT_SYMBOL_GPL(ip_vs_skb_defrag); + /* Register a pe in the pe list */ int register_ip_vs_pe(struct ip_vs_pe *pe) { diff --git a/net/netfilter/ipvs/ip_vs_pe_sip.c b/net/netfilter/ipvs/ip_vs_pe_sip.c index b8b4e96..78caa83 100644 --- a/net/netfilter/ipvs/ip_vs_pe_sip.c +++ b/net/netfilter/ipvs/ip_vs_pe_sip.c @@ -71,6 +71,7 @@ ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb) struct ip_vs_iphdr iph; unsigned int dataoff, datalen, matchoff, matchlen; const char *dptr; + int fr; ip_vs_fill_iphdr(p->af, skb_network_header(skb), &iph); @@ -85,21 +86,30 @@ ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb) dptr = skb->data + dataoff; datalen = skb->len - dataoff; - + fr = 0; + if( skb_shinfo(skb)->nr_frags ) { + dptr = ip_vs_skb_defrag(skb, dataoff, datalen); + if (!dptr) + return -EINVAL; + fr = 1; + } if (get_callid(dptr, dataoff, datalen, &matchoff, &matchlen)) - return -EINVAL; + goto err; p->pe_data = kmalloc(matchlen, GFP_ATOMIC); if (!p->pe_data) - return -ENOMEM; + goto err; /* N.B: pe_data is only set on success, * this allows fallback to the default persistence logic on failure */ memcpy(p->pe_data, dptr + matchoff, matchlen); p->pe_data_len = matchlen; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] ipvs: skb defrag for L7 helpers 2010-11-08 14:51 [RFC PATCH] ipvs: skb defrag for L7 helpers Hans Schillstrom @ 2010-11-08 21:43 ` Simon Horman 2010-11-08 21:56 ` Hans Schillstrom 2010-11-08 21:48 ` Julian Anastasov 1 sibling, 1 reply; 8+ messages in thread From: Simon Horman @ 2010-11-08 21:43 UTC (permalink / raw) To: Hans Schillstrom; +Cc: Julian Anastasov, LVS-Devel On Mon, Nov 08, 2010 at 03:51:56PM +0100, Hans Schillstrom wrote: > Hello > I have been struggling with SIP for a while .... > L7 helpers like sip needs skb defrag > ex virtio only copies the first 128 byte into the skb (incl mac hdr) > in that case Call-Id will never be found. > > There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c Thanks for tracking that down! > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> > > diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c > index e99f920..c0ac69a 100644 > --- a/net/netfilter/ipvs/ip_vs_pe.c > +++ b/net/netfilter/ipvs/ip_vs_pe.c > @@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) > return pe; > } > > +/* skb defrag for L7 helpers */ > +char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) > +{ > + char *p = kmalloc(skb->len, GFP_ATOMIC); > + if (!p) > + goto err; > + if (skb_copy_bits(skb, offset, p, len)) > + goto err; > + IP_VS_DBG(10, "IPVS defrag: offs:%d len:%d\n", offset, len); > + return p; > + > +err: > + if (p) > + kfree(p); > + return NULL; > +} > +EXPORT_SYMBOL_GPL(ip_vs_skb_defrag); > + > /* Register a pe in the pe list */ > int register_ip_vs_pe(struct ip_vs_pe *pe) > { > diff --git a/net/netfilter/ipvs/ip_vs_pe_sip.c b/net/netfilter/ipvs/ip_vs_pe_sip.c > index b8b4e96..78caa83 100644 > --- a/net/netfilter/ipvs/ip_vs_pe_sip.c > +++ b/net/netfilter/ipvs/ip_vs_pe_sip.c > @@ -71,6 +71,7 @@ ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb) > struct ip_vs_iphdr iph; > unsigned int dataoff, datalen, matchoff, matchlen; > const char *dptr; > + int fr; > > ip_vs_fill_iphdr(p->af, skb_network_header(skb), &iph); > > @@ -85,21 +86,30 @@ ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb) > > dptr = skb->data + dataoff; > datalen = skb->len - dataoff; > - > + fr = 0; > + if( skb_shinfo(skb)->nr_frags ) { From a style point of view the line above should probably be: if (skb_shinfo(skb)->nr_frags) { > + dptr = ip_vs_skb_defrag(skb, dataoff, datalen); > + if (!dptr) > + return -EINVAL; > + fr = 1; > + } But I wonder if this logic can be rolld into ip_vs_skb_defrag(), perhaps using ERR_PTR() and friends. Then again, what you have may already be at least as clean as that idea. > if (get_callid(dptr, dataoff, datalen, &matchoff, &matchlen)) > - return -EINVAL; > + goto err; > > p->pe_data = kmalloc(matchlen, GFP_ATOMIC); > if (!p->pe_data) > - return -ENOMEM; > + goto err; > > /* N.B: pe_data is only set on success, > * this allows fallback to the default persistence logic on failure > */ > memcpy(p->pe_data, dptr + matchoff, matchlen); > p->pe_data_len = matchlen; > - > return 0; > +err: > + if (fr) > + kfree(dptr); > + return -EINVAL; > } > > static bool ip_vs_sip_ct_match(const struct ip_vs_conn_param *p, > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h > index a6421e6..08bd547 100644 > --- a/include/net/ip_vs.h > +++ b/include/net/ip_vs.h > @@ -817,6 +817,7 @@ void ip_vs_unbind_pe(struct ip_vs_service *svc); > int register_ip_vs_pe(struct ip_vs_pe *pe); > int unregister_ip_vs_pe(struct ip_vs_pe *pe); > struct ip_vs_pe *ip_vs_pe_getbyname(const char *name); > +extern char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len); Personally I'm not a fan of the extern keyword. > static inline void ip_vs_pe_get(const struct ip_vs_pe *pe) > { > > -- > Regards > Hans Schillstrom <hans.schillstrom@ericsson.com> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] ipvs: skb defrag for L7 helpers 2010-11-08 21:43 ` Simon Horman @ 2010-11-08 21:56 ` Hans Schillstrom 0 siblings, 0 replies; 8+ messages in thread From: Hans Schillstrom @ 2010-11-08 21:56 UTC (permalink / raw) To: Simon Horman; +Cc: Hans Schillstrom, Julian Anastasov, LVS-Devel On Monday, November 08, 2010 22:43:46 Simon Horman wrote: > On Mon, Nov 08, 2010 at 03:51:56PM +0100, Hans Schillstrom wrote: > > Hello > > I have been struggling with SIP for a while .... > > L7 helpers like sip needs skb defrag > > ex virtio only copies the first 128 byte into the skb (incl mac hdr) > > in that case Call-Id will never be found. > > > > There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c > > Thanks for tracking that down! > > > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> > > > > diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c > > index e99f920..c0ac69a 100644 > > --- a/net/netfilter/ipvs/ip_vs_pe.c > > +++ b/net/netfilter/ipvs/ip_vs_pe.c > > @@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) > > return pe; > > } > > > > +/* skb defrag for L7 helpers */ > > +char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) > > +{ > > + char *p = kmalloc(skb->len, GFP_ATOMIC); > > + if (!p) > > + goto err; > > + if (skb_copy_bits(skb, offset, p, len)) > > + goto err; > > + IP_VS_DBG(10, "IPVS defrag: offs:%d len:%d\n", offset, len); > > + return p; > > + > > +err: > > + if (p) > > + kfree(p); > > + return NULL; > > +} > > +EXPORT_SYMBOL_GPL(ip_vs_skb_defrag); > > + > > /* Register a pe in the pe list */ > > int register_ip_vs_pe(struct ip_vs_pe *pe) > > { > > diff --git a/net/netfilter/ipvs/ip_vs_pe_sip.c b/net/netfilter/ipvs/ip_vs_pe_sip.c > > index b8b4e96..78caa83 100644 > > --- a/net/netfilter/ipvs/ip_vs_pe_sip.c > > +++ b/net/netfilter/ipvs/ip_vs_pe_sip.c > > @@ -71,6 +71,7 @@ ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb) > > struct ip_vs_iphdr iph; > > unsigned int dataoff, datalen, matchoff, matchlen; > > const char *dptr; > > + int fr; > > > > ip_vs_fill_iphdr(p->af, skb_network_header(skb), &iph); > > > > @@ -85,21 +86,30 @@ ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb) > > > > dptr = skb->data + dataoff; > > datalen = skb->len - dataoff; > > - > > + fr = 0; > > + if( skb_shinfo(skb)->nr_frags ) { > > >From a style point of view the line above should probably be: > > if (skb_shinfo(skb)->nr_frags) { I have to remind my self all time about that, but some times ... > > > + dptr = ip_vs_skb_defrag(skb, dataoff, datalen); > > + if (!dptr) > > + return -EINVAL; > > + fr = 1; > > + } > > But I wonder if this logic can be rolld into ip_vs_skb_defrag(), > perhaps using ERR_PTR() and friends. Then again, what you have > may already be at least as clean as that idea. I will have another look at it. > > > if (get_callid(dptr, dataoff, datalen, &matchoff, &matchlen)) > > - return -EINVAL; > > + goto err; > > > > p->pe_data = kmalloc(matchlen, GFP_ATOMIC); > > if (!p->pe_data) > > - return -ENOMEM; > > + goto err; > > > > /* N.B: pe_data is only set on success, > > * this allows fallback to the default persistence logic on failure > > */ > > memcpy(p->pe_data, dptr + matchoff, matchlen); > > p->pe_data_len = matchlen; > > - > > return 0; > > +err: > > + if (fr) > > + kfree(dptr); > > + return -EINVAL; > > } > > > > static bool ip_vs_sip_ct_match(const struct ip_vs_conn_param *p, > > diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h > > index a6421e6..08bd547 100644 > > --- a/include/net/ip_vs.h > > +++ b/include/net/ip_vs.h > > @@ -817,6 +817,7 @@ void ip_vs_unbind_pe(struct ip_vs_service *svc); > > int register_ip_vs_pe(struct ip_vs_pe *pe); > > int unregister_ip_vs_pe(struct ip_vs_pe *pe); > > struct ip_vs_pe *ip_vs_pe_getbyname(const char *name); > > +extern char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len); > > Personally I'm not a fan of the extern keyword. I am :-) I'll remove it > > > static inline void ip_vs_pe_get(const struct ip_vs_pe *pe) > > { > > > > -- > > Regards > > Hans Schillstrom <hans.schillstrom@ericsson.com> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] ipvs: skb defrag for L7 helpers 2010-11-08 14:51 [RFC PATCH] ipvs: skb defrag for L7 helpers Hans Schillstrom 2010-11-08 21:43 ` Simon Horman @ 2010-11-08 21:48 ` Julian Anastasov 2010-11-08 22:12 ` Hans Schillstrom 2010-11-08 22:23 ` Simon Horman 1 sibling, 2 replies; 8+ messages in thread From: Julian Anastasov @ 2010-11-08 21:48 UTC (permalink / raw) To: Hans Schillstrom; +Cc: Simon Horman, LVS-Devel Hello, On Mon, 8 Nov 2010, Hans Schillstrom wrote: > Hello > I have been struggling with SIP for a while .... > L7 helpers like sip needs skb defrag > ex virtio only copies the first 128 byte into the skb (incl mac hdr) > in that case Call-Id will never be found. > > There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c > > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> > > diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c > index e99f920..c0ac69a 100644 > --- a/net/netfilter/ipvs/ip_vs_pe.c > +++ b/net/netfilter/ipvs/ip_vs_pe.c > @@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) > return pe; > } > > +/* skb defrag for L7 helpers */ > +char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) > +{ > + char *p = kmalloc(skb->len, GFP_ATOMIC); > + if (!p) > + goto err; > + if (skb_copy_bits(skb, offset, p, len)) > + goto err; Such copy already exists: skb_linearize(). If you are lucky it does not copy data. But for your case the copy should happen. In ip_vs_ftp we even use skb_make_writable because we change the payload. May be ip_vs_sip_fill_param() should deliver the status from skb_linearize (-ENOMEM) after the iph.protocol != IPPROTO_UDP check. ip_vs_conn_fill_param_persist should return this error to ip_vs_sched_persist. What we need is ip_vs_sched_persist to have new argument 'int *ignored' just like ip_vs_schedule, so that we can return *ignored = 1 for the case when ip_vs_conn_fill_param_persist returns any kind of error. Even on ip_vs_conn_new failure we should return *ignored = 1. *ignored = 0 remains only for the case when no dest is selected. Regards -- Julian Anastasov <ja@ssi.bg> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] ipvs: skb defrag for L7 helpers 2010-11-08 21:48 ` Julian Anastasov @ 2010-11-08 22:12 ` Hans Schillstrom 2010-11-08 22:23 ` Simon Horman 1 sibling, 0 replies; 8+ messages in thread From: Hans Schillstrom @ 2010-11-08 22:12 UTC (permalink / raw) To: Julian Anastasov; +Cc: Hans Schillstrom, Simon Horman, LVS-Devel On Monday, November 08, 2010 22:48:37 Julian Anastasov wrote: > > Hello, > > On Mon, 8 Nov 2010, Hans Schillstrom wrote: > > > Hello > > I have been struggling with SIP for a while .... > > L7 helpers like sip needs skb defrag > > ex virtio only copies the first 128 byte into the skb (incl mac hdr) > > in that case Call-Id will never be found. > > > > There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c > > > > Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> > > > > diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c > > index e99f920..c0ac69a 100644 > > --- a/net/netfilter/ipvs/ip_vs_pe.c > > +++ b/net/netfilter/ipvs/ip_vs_pe.c > > @@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) > > return pe; > > } > > > > +/* skb defrag for L7 helpers */ > > +char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) > > +{ > > + char *p = kmalloc(skb->len, GFP_ATOMIC); > > + if (!p) > > + goto err; > > + if (skb_copy_bits(skb, offset, p, len)) > > + goto err; > > Such copy already exists: skb_linearize(). If you > are lucky it does not copy data. But for your case the > copy should happen. In ip_vs_ftp we even use skb_make_writable > because we change the payload. I didn't see that one, but skb_copy_bits is faster in the short run but in the end you still have a fragmented skb. I think skb_linearize is better, Thanks. > > May be ip_vs_sip_fill_param() should deliver the status > from skb_linearize (-ENOMEM) after the iph.protocol != IPPROTO_UDP > check. ip_vs_conn_fill_param_persist should return this > error to ip_vs_sched_persist. What we need is ip_vs_sched_persist > to have new argument 'int *ignored' just like ip_vs_schedule, so > that we can return *ignored = 1 for the case when > ip_vs_conn_fill_param_persist returns any kind of error. > Even on ip_vs_conn_new failure we should return *ignored = 1. > *ignored = 0 remains only for the case when no dest is > selected. OK, sounds like a good Idea I add that to the patch > > Regards > > -- > Julian Anastasov <ja@ssi.bg> Regards Hans ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] ipvs: skb defrag for L7 helpers 2010-11-08 21:48 ` Julian Anastasov 2010-11-08 22:12 ` Hans Schillstrom @ 2010-11-08 22:23 ` Simon Horman 2010-11-08 23:04 ` Hans Schillstrom 1 sibling, 1 reply; 8+ messages in thread From: Simon Horman @ 2010-11-08 22:23 UTC (permalink / raw) To: Julian Anastasov; +Cc: Hans Schillstrom, LVS-Devel On Mon, Nov 08, 2010 at 11:48:37PM +0200, Julian Anastasov wrote: > > Hello, > > On Mon, 8 Nov 2010, Hans Schillstrom wrote: > > >Hello > >I have been struggling with SIP for a while .... > >L7 helpers like sip needs skb defrag > >ex virtio only copies the first 128 byte into the skb (incl mac hdr) > >in that case Call-Id will never be found. > > > >There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c > > > >Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> > > > >diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c > >index e99f920..c0ac69a 100644 > >--- a/net/netfilter/ipvs/ip_vs_pe.c > >+++ b/net/netfilter/ipvs/ip_vs_pe.c > >@@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) > > return pe; > >} > > > >+/* skb defrag for L7 helpers */ > >+char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) > >+{ > >+ char *p = kmalloc(skb->len, GFP_ATOMIC); > >+ if (!p) > >+ goto err; > >+ if (skb_copy_bits(skb, offset, p, len)) > >+ goto err; > > Such copy already exists: skb_linearize(). If you > are lucky it does not copy data. But for your case the > copy should happen. In ip_vs_ftp we even use skb_make_writable > because we change the payload. Good point. > May be ip_vs_sip_fill_param() should deliver the status > from skb_linearize (-ENOMEM) after the iph.protocol != IPPROTO_UDP > check. ip_vs_conn_fill_param_persist should return this > error to ip_vs_sched_persist. What we need is ip_vs_sched_persist > to have new argument 'int *ignored' just like ip_vs_schedule, so > that we can return *ignored = 1 for the case when > ip_vs_conn_fill_param_persist returns any kind of error. > Even on ip_vs_conn_new failure we should return *ignored = 1. > *ignored = 0 remains only for the case when no dest is > selected. Agreed. Somewhat off topic: the *ignored and *verdict parameters seem to be a horrible way to pass this information around. I think we can use ERR_PTR instead, though I'd have to write the patch to see if that is true or not. If its not perhaps we can have a small structure used as the return value of ip_vs_schedule() and pp->conn_schedule(). struct ip_vs_scheduler_status { ip_vs_conn *cp; int flag; } However, I do wonder if the defrag change is stable material. If so, I'll hold off on any re-working ideas. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] ipvs: skb defrag for L7 helpers 2010-11-08 22:23 ` Simon Horman @ 2010-11-08 23:04 ` Hans Schillstrom 2010-11-09 0:27 ` Simon Horman 0 siblings, 1 reply; 8+ messages in thread From: Hans Schillstrom @ 2010-11-08 23:04 UTC (permalink / raw) To: Simon Horman; +Cc: Julian Anastasov, Hans Schillstrom, LVS-Devel On Monday, November 08, 2010 23:23:54 Simon Horman wrote: > On Mon, Nov 08, 2010 at 11:48:37PM +0200, Julian Anastasov wrote: > > > > Hello, > > > > On Mon, 8 Nov 2010, Hans Schillstrom wrote: > > > > >Hello > > >I have been struggling with SIP for a while .... > > >L7 helpers like sip needs skb defrag > > >ex virtio only copies the first 128 byte into the skb (incl mac hdr) > > >in that case Call-Id will never be found. > > > > > >There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c > > > > > >Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> > > > > > >diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c > > >index e99f920..c0ac69a 100644 > > >--- a/net/netfilter/ipvs/ip_vs_pe.c > > >+++ b/net/netfilter/ipvs/ip_vs_pe.c > > >@@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) > > > return pe; > > >} > > > > > >+/* skb defrag for L7 helpers */ > > >+char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) > > >+{ > > >+ char *p = kmalloc(skb->len, GFP_ATOMIC); > > >+ if (!p) > > >+ goto err; > > >+ if (skb_copy_bits(skb, offset, p, len)) > > >+ goto err; > > > > Such copy already exists: skb_linearize(). If you > > are lucky it does not copy data. But for your case the > > copy should happen. In ip_vs_ftp we even use skb_make_writable > > because we change the payload. > > Good point. > > > May be ip_vs_sip_fill_param() should deliver the status > > from skb_linearize (-ENOMEM) after the iph.protocol != IPPROTO_UDP > > check. ip_vs_conn_fill_param_persist should return this > > error to ip_vs_sched_persist. What we need is ip_vs_sched_persist > > to have new argument 'int *ignored' just like ip_vs_schedule, so > > that we can return *ignored = 1 for the case when > > ip_vs_conn_fill_param_persist returns any kind of error. > > Even on ip_vs_conn_new failure we should return *ignored = 1. > > *ignored = 0 remains only for the case when no dest is > > selected. > > Agreed. > > Somewhat off topic: the *ignored and *verdict parameters > seem to be a horrible way to pass this information around. > I think we can use ERR_PTR instead, though I'd have to > write the patch to see if that is true or not. If its > not perhaps we can have a small structure used as > the return value of ip_vs_schedule() and pp->conn_schedule(). > > struct ip_vs_scheduler_status { > ip_vs_conn *cp; > int flag; > } > > However, I do wonder if the defrag change is stable material. > If so, I'll hold off on any re-working ideas. I have been runing it for two days now with SIP and that seems to work. I can send an update version tomorrow. It's time for a power nap now. Regards Hans ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH] ipvs: skb defrag for L7 helpers 2010-11-08 23:04 ` Hans Schillstrom @ 2010-11-09 0:27 ` Simon Horman 0 siblings, 0 replies; 8+ messages in thread From: Simon Horman @ 2010-11-09 0:27 UTC (permalink / raw) To: Hans Schillstrom; +Cc: Julian Anastasov, Hans Schillstrom, LVS-Devel On Tue, Nov 09, 2010 at 12:04:44AM +0100, Hans Schillstrom wrote: > On Monday, November 08, 2010 23:23:54 Simon Horman wrote: > > On Mon, Nov 08, 2010 at 11:48:37PM +0200, Julian Anastasov wrote: > > > > > > Hello, > > > > > > On Mon, 8 Nov 2010, Hans Schillstrom wrote: > > > > > > >Hello > > > >I have been struggling with SIP for a while .... > > > >L7 helpers like sip needs skb defrag > > > >ex virtio only copies the first 128 byte into the skb (incl mac hdr) > > > >in that case Call-Id will never be found. > > > > > > > >There is a skb_find_text() that might be used insead of this, but it requires some changes in ip_vs_pe_sip.c > > > > > > > >Signed-off-by: Hans Schillstrom <hans.schillstrom@ericsson.com> > > > > > > > >diff --git a/net/netfilter/ipvs/ip_vs_pe.c b/net/netfilter/ipvs/ip_vs_pe.c > > > >index e99f920..c0ac69a 100644 > > > >--- a/net/netfilter/ipvs/ip_vs_pe.c > > > >+++ b/net/netfilter/ipvs/ip_vs_pe.c > > > >@@ -76,6 +72,24 @@ struct ip_vs_pe *ip_vs_pe_getbyname(const char *name) > > > > return pe; > > > >} > > > > > > > >+/* skb defrag for L7 helpers */ > > > >+char *ip_vs_skb_defrag(struct sk_buff *skb, int offset, int len) > > > >+{ > > > >+ char *p = kmalloc(skb->len, GFP_ATOMIC); > > > >+ if (!p) > > > >+ goto err; > > > >+ if (skb_copy_bits(skb, offset, p, len)) > > > >+ goto err; > > > > > > Such copy already exists: skb_linearize(). If you > > > are lucky it does not copy data. But for your case the > > > copy should happen. In ip_vs_ftp we even use skb_make_writable > > > because we change the payload. > > > > Good point. > > > > > May be ip_vs_sip_fill_param() should deliver the status > > > from skb_linearize (-ENOMEM) after the iph.protocol != IPPROTO_UDP > > > check. ip_vs_conn_fill_param_persist should return this > > > error to ip_vs_sched_persist. What we need is ip_vs_sched_persist > > > to have new argument 'int *ignored' just like ip_vs_schedule, so > > > that we can return *ignored = 1 for the case when > > > ip_vs_conn_fill_param_persist returns any kind of error. > > > Even on ip_vs_conn_new failure we should return *ignored = 1. > > > *ignored = 0 remains only for the case when no dest is > > > selected. > > > > Agreed. > > > > Somewhat off topic: the *ignored and *verdict parameters > > seem to be a horrible way to pass this information around. > > I think we can use ERR_PTR instead, though I'd have to > > write the patch to see if that is true or not. If its > > not perhaps we can have a small structure used as > > the return value of ip_vs_schedule() and pp->conn_schedule(). > > > > struct ip_vs_scheduler_status { > > ip_vs_conn *cp; > > int flag; > > } > > > > However, I do wonder if the defrag change is stable material. > > If so, I'll hold off on any re-working ideas. > > I have been runing it for two days now with SIP > and that seems to work. > > I can send an update version tomorrow. Ok, perhaps this patch is a good candidate for stable. We can incorporate Julians ideas into a patch for next. And clean-ups can go in after that. > It's time for a power nap now. :-) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-11-09 0:27 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-11-08 14:51 [RFC PATCH] ipvs: skb defrag for L7 helpers Hans Schillstrom 2010-11-08 21:43 ` Simon Horman 2010-11-08 21:56 ` Hans Schillstrom 2010-11-08 21:48 ` Julian Anastasov 2010-11-08 22:12 ` Hans Schillstrom 2010-11-08 22:23 ` Simon Horman 2010-11-08 23:04 ` Hans Schillstrom 2010-11-09 0:27 ` Simon Horman
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.