[RFC PATCH] ipvs: skb defrag for L7 helpers

[RFC PATCH] ipvs: skb defrag for L7 helpers

[Hans Schillstrom]

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;

Re: [RFC PATCH] ipvs: skb defrag for L7 helpers

[Simon Horman]

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>
> 

Re: [RFC PATCH] ipvs: skb defrag for L7 helpers

[Julian Anastasov]

 	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>

Re: [RFC PATCH] ipvs: skb defrag for L7 helpers

[Hans Schillstrom]

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>
> > 

Re: [RFC PATCH] ipvs: skb defrag for L7 helpers

[Hans Schillstrom]

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

Re: [RFC PATCH] ipvs: skb defrag for L7 helpers

[Simon Horman]

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.

Re: [RFC PATCH] ipvs: skb defrag for L7 helpers

[Hans Schillstrom]

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

Re: [RFC PATCH] ipvs: skb defrag for L7 helpers

[Simon Horman]

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.

:-)