LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

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

To: Hans Schillstrom <hans.schillstrom@xxxxxxxxxxxx>
Subject: Re: [RFC PATCH] ipvs: skb defrag for L7 helpers
Cc: Julian Anastasov <ja@xxxxxx>, LVS-Devel <lvs-devel@xxxxxxxxxxxxxxx>
From: Simon Horman <horms@xxxxxxxxxxxx>
Date: Tue, 9 Nov 2010 06:43:46 +0900
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@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> 
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>