| Message ID | 20141004141802.GA10878@gondor.apana.org.au |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Sat, Oct 04, 2014 at 09:55:08PM +0800, Herbert Xu wrote: > > > > I'll try to create a patch that essentially reverts the patch > > that led us here. > > Here is a patch that's only compile-tested: > > bridge: Do not compile options in br_parse_ip_options > > Commit 462fb2af9788a82a534f8184abfde31574e1cfa0 > > bridge : Sanitize skb before it enters the IP stack > > broke when IP options are actually used because it mangles the > skb as if it entered the IP stack which is wrong because the > bridge is supposed to operate below the IP stack. > > Since nobody has actually requested for parsing of IP options > this patch fixes it by simply reverting to the previous approach > of ignoring all IP options, i.e., zeroing the IPCB. Fair enough. We lose frag_max_size information from ipv4 defrag, plus netfilter hooks are called without validating ip options. The former has not worked ever with bridge, and the latter evidentily isn't a problem either since this has not worked at all for three years... So I am fine with it, provided we rename br_parse_ip_options() -- thats not what it does after this patch (br_validate_iphdr(), for example?) > If and when somebody who uses IP options and actually needs them > to be parsed by the bridge complains then we can revisit this. Ok, fair enough. Thanks Herbert. -- 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
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Sat, 4 Oct 2014 22:18:02 +0800 > bridge: Do not compile options in br_parse_ip_options > > Commit 462fb2af9788a82a534f8184abfde31574e1cfa0 > > bridge : Sanitize skb before it enters the IP stack > > broke when IP options are actually used because it mangles the > skb as if it entered the IP stack which is wrong because the > bridge is supposed to operate below the IP stack. > > Since nobody has actually requested for parsing of IP options > this patch fixes it by simply reverting to the previous approach > of ignoring all IP options, i.e., zeroing the IPCB. > > If and when somebody who uses IP options and actually needs them > to be parsed by the bridge complains then we can revisit this. > > Reported-by: David Newall <davidn@davidnewall.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Agreed that mangling the packet is definitely wrong here, and since you preserve the CB clearing this change should be fine. Please submit this formally after it's been tested. Thanks. -- 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
Herbert Xu <herbert@gondor.apana.org.au> wrote: > bridge: Do not compile options in br_parse_ip_options > > Commit 462fb2af9788a82a534f8184abfde31574e1cfa0 > > bridge : Sanitize skb before it enters the IP stack > > broke when IP options are actually used because it mangles the > skb as if it entered the IP stack which is wrong because the > bridge is supposed to operate below the IP stack. > > Since nobody has actually requested for parsing of IP options > this patch fixes it by simply reverting to the previous approach > of ignoring all IP options, i.e., zeroing the IPCB. > > If and when somebody who uses IP options and actually needs them > to be parsed by the bridge complains then we can revisit this. > > Reported-by: David Newall <davidn@davidnewall.com> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Tested-by: Florian Westphal <fw@strlen.de> Pablo, could you please apply this? Thanks! -- 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
On Fri, Oct 24, 2014 at 12:41:49PM +0200, Florian Westphal wrote: > Herbert Xu <herbert@gondor.apana.org.au> wrote: > > bridge: Do not compile options in br_parse_ip_options > > > > Commit 462fb2af9788a82a534f8184abfde31574e1cfa0 > > > > bridge : Sanitize skb before it enters the IP stack > > > > broke when IP options are actually used because it mangles the > > skb as if it entered the IP stack which is wrong because the > > bridge is supposed to operate below the IP stack. > > > > Since nobody has actually requested for parsing of IP options > > this patch fixes it by simply reverting to the previous approach > > of ignoring all IP options, i.e., zeroing the IPCB. > > > > If and when somebody who uses IP options and actually needs them > > to be parsed by the bridge complains then we can revisit this. > > > > Reported-by: David Newall <davidn@davidnewall.com> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > > Tested-by: Florian Westphal <fw@strlen.de> Applied, thanks a lot for testing Florian. -- 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/bridge/br_netfilter.c b/net/bridge/br_netfilter.c index a615264..c0fdb4d 100644 --- a/net/bridge/br_netfilter.c +++ b/net/bridge/br_netfilter.c @@ -260,7 +260,6 @@ static inline void nf_bridge_update_protocol(struct sk_buff *skb) 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; @@ -269,7 +268,6 @@ static int br_parse_ip_options(struct sk_buff *skb) goto inhdr_error; iph = ip_hdr(skb); - opt = &(IPCB(skb)->opt); /* Basic sanity checks */ if (iph->ihl < 5 || iph->version != 4) @@ -295,23 +293,11 @@ static int br_parse_ip_options(struct sk_buff *skb) } 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; - } - + /* We should really parse IP options here but until + * somebody who actually uses IP options complains to + * us we'll just silently ignore the options because + * we're lazy! + */ return 0; inhdr_error: