| Message ID | 53815623.8020506@davidnewall.com |
|---|---|
| State | Superseded |
| Headers | show |
From: David Newall <davidn@davidnewall.com> Date: Sun, 25 May 2014 12:02:03 +0930 > On 25/05/14 03:13, David Miller wrote: >> This patch was substantially corrupted by your email client. > > We should be sending these things as mime attachments. Having to put > patches inline is brittle, absurd and a waste of everyone's time. Is > there actually anybody here who doesn't have a mime-compatible MUA? It makes replying and commenting inline easy. It's not our problem that so many email clients make sending plain unmolested ASCII text difficult. But at least we've gone out of our way to document how to do so in the kernel tree. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/05/14 12:32, David Miller wrote: > From: David Newall<davidn@davidnewall.com> > Date: Sun, 25 May 2014 12:02:03 +0930 > >> On 25/05/14 03:13, David Miller wrote: >>> This patch was substantially corrupted by your email client. >> >We should be sending these things as mime attachments. > It makes replying and commenting inline easy. Patches are intrinsically corrupted by commenting on them inline, and that doesn't matter. What does matter is when a patch that people will need to test is corrupted, and sending them as mime attachments is the best answer I know of. It's trivial to copy and paste from an attachment to the body so that you can comment; far easier than copying and pasting a patch verbatim (i.e. without corrupting it.) > It's not our problem that so many email clients make sending > plain unmolested ASCII text difficult. It wasn't the email client; it was the xfce-terminal copy that corrupted it. It's proven to corrupt this patch in two different ways; the other, which I caught before send, was because of unreliable scrollback. In fact it is our problem when we insist that patches be sent in a way which we know is brittle and error-prone; our problem and our fault. Just imagine if you could have back all of the time you've wasted looking at included code, only to discover that it had been corrupted in some way or another; and then multiply that by everybody else who's wasted time the same way. The argument that it makes it easy to comment is unconvincing to me because the alternative is so easy. I apologise for this noise as I don't believe this is something which will change any time soon; it will change, just not soon. I'm quite willing to drop the issue. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Newall > On 25/05/14 03:13, David Miller wrote: > > This patch was substantially corrupted by your email client. > > We should be sending these things as mime attachments. Having to put > patches inline is brittle, absurd and a waste of everyone's time. Is > there actually anybody here who doesn't have a mime-compatible MUA? Yes - anyone using the email client from the world's largest desktop computer software company. It doesn't have any method for displaying text attachments. It has a scheme for executing attachments, for which it will use an interpreter based on the filename extension. (Yes - this is why it is very good at propagating viruses.) FWIW it can send valid patches quite easily - just copy/paste from wordpad. (Possibly after hacking the registry to allow lines longer than 72 characters. David -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Newall <davidn@davidnewall.com> Date: Sun, 25 May 2014 12:02:03 +0930 > + pskb_trim_rcsum(skb, len); You really need to check the return value as this can perform allocations, GFP_ATOMIC ones in fact. Also, why are we not bumping the statistics any more? I didn't see a discussion of that in this thread. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- linux-source-3.13.0/net/bridge/br_netfilter.c.orig 2014-05-17 00:12:23.418906498 +0930 +++ linux-source-3.13.0/net/bridge/br_netfilter.c 2014-05-17 01:04:43.540972961 +0930 @@ -253,73 +253,6 @@ static inline void nf_bridge_update_prot skb->protocol = htons(ETH_P_PPP_SES); } -/* When handing a packet over to the IP layer - * check whether we have a skb that is in the - * expected format - */ - -static int br_parse_ip_options(struct sk_buff *skb) -{ - struct ip_options *opt; - const struct iphdr *iph; - struct net_device *dev = skb->dev; - u32 len; - - if (!pskb_may_pull(skb, sizeof(struct iphdr))) - goto inhdr_error; - - iph = ip_hdr(skb); - opt = &(IPCB(skb)->opt); - - /* Basic sanity checks */ - if (iph->ihl < 5 || iph->version != 4) - goto inhdr_error; - - if (!pskb_may_pull(skb, iph->ihl*4)) - goto inhdr_error; - - iph = ip_hdr(skb); - if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl))) - goto inhdr_error; - - len = ntohs(iph->tot_len); - if (skb->len < len) { - IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INTRUNCATEDPKTS); - goto drop; - } else if (len < (iph->ihl*4)) - goto inhdr_error; - - if (pskb_trim_rcsum(skb, len)) { - IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INDISCARDS); - goto drop; - } - - memset(IPCB(skb), 0, sizeof(struct inet_skb_parm)); - if (iph->ihl == 5) - return 0; - - opt->optlen = iph->ihl*4 - sizeof(struct iphdr); - if (ip_options_compile(dev_net(dev), opt, skb)) - goto inhdr_error; - - /* Check correct handling of SRR option */ - if (unlikely(opt->srr)) { - struct in_device *in_dev = __in_dev_get_rcu(dev); - if (in_dev && !IN_DEV_SOURCE_ROUTE(in_dev)) - goto drop; - - if (ip_options_rcv_srr(skb)) - goto drop; - } - - return 0; - -inhdr_error: - IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INHDRERRORS); -drop: - return -1; -} - /* Fill in the header for fragmented IP packets handled by * the IPv4 connection tracking code. */ @@ -679,6 +612,7 @@ static unsigned int br_nf_pre_routing(co { struct net_bridge_port *p; struct net_bridge *br; + const struct iphdr *iph; __u32 len = nf_bridge_encap_header_len(skb); if (unlikely(!pskb_may_pull(skb, len))) @@ -704,10 +638,30 @@ static unsigned int br_nf_pre_routing(co return NF_ACCEPT; nf_bridge_pull_encap_header_rcsum(skb); + + if (!pskb_may_pull(skb, sizeof(struct iphdr))) + return NF_DROP; - if (br_parse_ip_options(skb)) + iph = ip_hdr(skb); + if (iph->ihl < 5 || iph->version != 4) + return NF_DROP; + + if (!pskb_may_pull(skb, 4 * iph->ihl)) return NF_DROP; + iph = ip_hdr(skb); + if (ip_fast_csum((__u8 *) iph, iph->ihl) != 0) + return NF_DROP; + + len = ntohs(iph->tot_len); + if (skb->len < len || len < 4 * iph->ihl) + return NF_DROP; + + pskb_trim_rcsum(skb, len); + + /* BUG: Should really parse the IP options here. */ + memset(IPCB(skb), 0, sizeof(struct inet_skb_parm)); + nf_bridge_put(skb->nf_bridge); if (!nf_bridge_alloc(skb)) return NF_DROP; @@ -806,9 +760,6 @@ static unsigned int br_nf_forward_ip(con nf_bridge->mask |= BRNF_PKT_TYPE; } - if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb)) - return NF_DROP; - /* The physdev module checks on this */ nf_bridge->mask |= BRNF_BRIDGED; nf_bridge->physoutdev = skb->dev; @@ -862,19 +813,14 @@ static unsigned int br_nf_forward_arp(co #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) static int br_nf_dev_queue_xmit(struct sk_buff *skb) { - int ret; - if (skb->nfct != NULL && skb->protocol == htons(ETH_P_IP) && skb->len + nf_bridge_mtu_reduction(skb) > skb->dev->mtu && !skb_is_gso(skb)) { - if (br_parse_ip_options(skb)) - /* Drop invalid packet */ - return NF_DROP; - ret = ip_fragment(skb, br_dev_queue_push_xmit); + /* BUG: Should really parse the IP options here. */ + memset(IPCB(skb), 0, sizeof(struct inet_skb_parm)); + return ip_fragment(skb, br_dev_queue_push_xmit); } else - ret = br_dev_queue_push_xmit(skb); - - return ret; + return br_dev_queue_push_xmit(skb); } #else static int br_nf_dev_queue_xmit(struct sk_buff *skb) --- linux-source-3.13.0/net/ipv4/ip_options.c.orig 2014-05-16 18:11:10.260370554 +0930 +++ linux-source-3.13.0/net/ipv4/ip_options.c 2014-05-17 01:01:56.738277137 +0930 @@ -475,7 +475,6 @@ error: } return -EINVAL; } -EXPORT_SYMBOL(ip_options_compile); /* * Undo all the changes done by ip_options_compile(). @@ -658,4 +657,3 @@ int ip_options_rcv_srr(struct sk_buff *s } return 0; } -EXPORT_SYMBOL(ip_options_rcv_srr); --- /usr/src/linux-source-3.13.0/net/bridge/br_private.h.orig 2014-05-24 13:51:09.269709831 +0930 +++ /usr/src/linux-source-3.13.0/net/bridge/br_private.h 2014-05-24 13:53:20.243551927 +0930 @@ -18,6 +18,7 @@ #include <linux/netpoll.h> #include <linux/u64_stats_sync.h> #include <net/route.h> +#include <net/ip.h> #include <linux/if_vlan.h> #define BR_HASH_BITS 8 @@ -304,6 +305,7 @@ struct net_bridge }; struct br_input_skb_cb { + struct inet_skb_parm ip; /* we don't interfere with ip's use of cb area */ struct net_device *brdev; #ifdef CONFIG_BRIDGE_IGMP_SNOOPING int igmp;
On 25/05/14 03:13, David Miller wrote: > This patch was substantially corrupted by your email client. We should be sending these things as mime attachments. Having to put patches inline is brittle, absurd and a waste of everyone's time. Is there actually anybody here who doesn't have a mime-compatible MUA? Trying again... -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html