On Thu, Sep 26, 2013 at 11:25:01AM -0700, Pravin Shelar wrote:

On Thu, Sep 26, 2013 at 1:25 AM, Steffen Klassert
[off-list ref] wrote:

quoted

On Wed, Sep 25, 2013 at 09:55:50AM -0700, Pravin Shelar wrote:

quoted

On Tue, Sep 24, 2013 at 10:54 PM, Steffen Klassert
[off-list ref] wrote:

quoted

We might extend the used aera of a skb beyond the total
headroom when we install the ipip header. Fix this by
calling skb_cow_head() unconditionally.

It is better to call skb_cow_head() from ipip_tunnel_xmit() as it is
consistent with gre.

I think this would just move the bug from ipip to gre. ipgre_xmit()
uses dev->needed_headroom which is based on the guessed output device
in ip_tunnel_bind_dev(). If the device we get from the route lookup
in ip_tunnel_xmit() is different from the guessed one and the resulting
max_headroom is bigger than dev->needed_headroom, we run into that bug
because skb_cow_head() will not be called with the updated
dev->needed_headroom.

Thats why ip_tunnel_xmit() update dev->needed_headroom.
Just to be clear I was talking abt calling skb_cow_head with
dev->needed_headroom in ipip_xmit and leave ip_tunnel_xmit as it is.
So that most of cases we will not need to adjust headroom in ip_tunnel
xmit.

skb_cow_head() does not do much if there is enough headroom, so
calling it here is uncritical. But we should adjust the headroom
as soon as we know that it is insufficient.

Also, I really wonder how you want to adjust the headroom in
ipip_tunnel_xmit() to a correct value. We know the needed
headroom after the route lookup in ip_tunnel_xmit() and
we have to adust it here because ip_tunnel_xmit() calls
iptunnel_xmit() which does a __skb_push() before it
installs the IP header.

Please keep in mind tat this is a bug fix that might be interesting
for stable too, we should try to keep the changes at a minimum.

Another thing that I noticed, with commit 0e6fbc5b
(ip_tunnels: extend iptunnel_xmit()) you moved the IP header
installation to iptunnel_xmit() and changed skb_push()
to __skb_push(). This made this bug quite hard to track
down because instead of triggering a skb under panic,
it did a silent memory corruption and crashed at random
other places. Maybe we should change this back to skb_push().