| Message ID | 4BEDA3CA.4030407@trash.net |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Friday 2010-05-14 21:26, Patrick McHardy wrote: >>> Should this be NF_DROP? As I understand it skb_linearize only failes >>> if it runs out of memory, which probably means dropping is OK. But >>> passing a packet that might need rewriting could be harmful.. >> >> We so far also didn't rewrite the packet. But agreed, its >> a corner case and dropping it is the safer choice. > >This is what I've added to my tree. Tested with asterisk and TSO >enabled NIC, which fails without this patch. > [..patch..] Shouldn't we do this for the other nf_conntrack_xyz too? That would mean getting rid of the size-limited locked packet buffer. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jan Engelhardt wrote: > On Friday 2010-05-14 21:26, Patrick McHardy wrote: >>>> Should this be NF_DROP? As I understand it skb_linearize only failes >>>> if it runs out of memory, which probably means dropping is OK. But >>>> passing a packet that might need rewriting could be harmful.. >>> We so far also didn't rewrite the packet. But agreed, its >>> a corner case and dropping it is the safer choice. >> This is what I've added to my tree. Tested with asterisk and TSO >> enabled NIC, which fails without this patch. >> > > [..patch..] > > Shouldn't we do this for the other nf_conntrack_xyz too? > That would mean getting rid of the size-limited locked packet > buffer. Those got introduced to avoid the linearization. SIP is kind of special because its the only helper mangling packets multiple times with variable sizes and is currently unable to use the data copying scheme. Amanda does this as well, but uses the string search API, which works fine. The best fix to get rid of the copying in other helpers would be to convert them to the string search API. I started doing that a few years ago, but never finished it. One related improvement we could add would be to make only those parts of the skb writable that are actually written to when mangling packets, at least for non-linear non-paged skbs (using frag_list). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c index b20f427..53d8922 100644 --- a/net/netfilter/nf_conntrack_sip.c +++ b/net/netfilter/nf_conntrack_sip.c @@ -1393,10 +1393,8 @@ static int sip_help_tcp(struct sk_buff *skb, unsigned int protoff, nf_ct_refresh(ct, skb, sip_timeout * HZ); - if (skb_is_nonlinear(skb)) { - pr_debug("Copy of skbuff not supported yet.\n"); - return NF_ACCEPT; - } + if (unlikely(skb_linearize(skb))) + return NF_DROP; dptr = skb->data + dataoff; datalen = skb->len - dataoff; @@ -1455,10 +1453,8 @@ static int sip_help_udp(struct sk_buff *skb, unsigned int protoff, nf_ct_refresh(ct, skb, sip_timeout * HZ); - if (skb_is_nonlinear(skb)) { - pr_debug("Copy of skbuff not supported yet.\n"); - return NF_ACCEPT; - } + if (unlikely(skb_linearize(skb))) + return NF_DROP; dptr = skb->data + dataoff; datalen = skb->len - dataoff;