| Message ID | 5370CB47.4010400@davidnewall.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
David Newall <davidn@davidnewall.com> wrote: > I've got a patch which fixes the faulty checksums, and now ping > works with RR or TS set. I'm not quite sure, though, that it fixes > the right thing. I wonder if the problem is less that the checksum > becomes wrong, and more that the route and timestamps ought not to > be changed by the bridge interface. Agree, bridge should not alter ip options. -- 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 12/05/14 23:21, Florian Westphal wrote:
> Agree, bridge should not alter ip options.
It would be easy to remove the call to ip_options_compile instead of
recalculating checksum after it, but I suspect there may be good reasons
why this, too, would be wrong. The source file is br_netfilter.c,
suggesting that a change in options is needed in some situations.
In the situation that caught my attention, it obviously does it wrong
(probably didn't add 0.0.0.0 to the route record, probably just
incremented the pointer; and seriously damaged the timestamps as well as
an incremented pointer without actually adding a value.)
I'm in a quandary.
Is it possible that bridge has exceeded it's mandate? I can't find it
now, but I saw a comment that it just copies packets unchanged. I think
it's use now goes further than that would allow.
I welcome words of advice.
--
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
> I've got a patch which fixes the faulty checksums, and now ping works > with RR or TS set. I'm not quite sure, though, that it fixes the right > thing. I wonder if the problem is less that the checksum becomes wrong, > and more that the route and timestamps ought not to be changed by the > bridge interface. Looks like your testcase: - works in 2.6.36 and older - crashes starting with 2.6.37 (-rc1), likely due to Bandan's commit 462fb2af9788a82 (bridge : Sanitize skb before it enters the IP stack) [1] - crash fix is in 2.6.38.4, likely due to Eric's commit f8e9881c2aef1e9 (bridge: reset IPCB in br_parse_ip_options) [2] - doesn't work post-crashfix [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=462fb2af9788a82a534f8184abfde31574e1cfa0 [2] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=f8e9881c2aef1e982e5abc25c046820cd0b7cf64 -- 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
--- br_netfilter.c.orig 2014-05-12 22:10:59.809988125 +0930 +++ br_netfilter.c 2014-05-12 22:27:46.769299379 +0930 @@ -261,8 +261,10 @@ static int br_parse_ip_options(struct sk_buff *skb) { struct ip_options *opt; - const struct iphdr *iph; + struct iphdr *iph; struct net_device *dev = skb->dev; + __sum16 oldsum; + int err; u32 len; if (!pskb_may_pull(skb, sizeof(struct iphdr))) @@ -298,8 +300,15 @@ if (iph->ihl == 5) return 0; + oldsum = iph->check; opt->optlen = iph->ihl*4 - sizeof(struct iphdr); - if (ip_options_compile(dev_net(dev), opt, skb)) + err = ip_options_compile(dev_net(dev), opt, skb); + ip_send_check(iph); + if (iph->check != oldsum) + LIMIT_NETDEBUG(KERN_ERR + pr_fmt("br_parse_ip_options: bad sum %x replaced by %x\n"), + oldsum, iph->check); + if (err) goto inhdr_error; /* Check correct handling of SRR option */