[PATCH] NET: fix kernel panic from no dev->hard_header_len space

[PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

Hi,

ip_output() ignores dev->hard_header_len if dev->hard_header
function == NULL. It's inconsistent with the rest of the kernel. With
some drivers it would mean instant panic, but we are usually saved by
existence of the "default" header space.

With some bad luck things like tun or IPsec may kill the box.

A similar problem may be present in psched_mtu().

Please apply.
I think it should then be applied to stable series as well.

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index b94d1ad..3ae64ba 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -234,8 +234,7 @@ extern int tc_classify(struct sk_buff *s
  */
 static inline unsigned psched_mtu(struct net_device *dev)
 {
-	unsigned mtu = dev->mtu;
-	return dev->hard_header ? mtu + dev->hard_header_len : mtu;
+	return dev->mtu + dev->hard_header_len;
 }
 
 #endif
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index cff9c3a..f5d9051 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -169,7 +169,7 @@ static inline int ip_finish_output2(stru
 	int hh_len = LL_RESERVED_SPACE(dev);
 
 	/* Be paranoid, rather than too clever. */
-	if (unlikely(skb_headroom(skb) < hh_len && dev->hard_header)) {
+	if (unlikely(skb_headroom(skb) < hh_len)) {
 		struct sk_buff *skb2;
 
 		skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev));

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Alexey Kuznetsov]

Hello!

> ip_output() ignores dev->hard_header_len

ip_output() worries about the space, which it needs.

If some place needs more, it is its problem to check.
To the moment where it is used, hard_header_len can even change.

It can be applied, but it does not change the fact, that those
placed which fail now must check the condition as well.


> A similar problem may be present in psched_mtu().

Nothing similar. The result psched_mtu() is compared with skb->len,
how it is seen by qdiscs. If hard_header is NULL, it sees skbs
without header.

Alexey

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> writes:

> ip_output() worries about the space, which it needs.

Well, I wrote ip_output() to give idea about the place but the
actual function, as shown in the patch, is ip_finish_output2().

It currently reads:
        int hh_len = LL_RESERVED_SPACE(dev);

        /* Be paranoid, rather than too clever. */
        if (unlikely(skb_headroom(skb) < hh_len && dev->hard_header)) {
                struct sk_buff *skb2;

                skb2 = skb_realloc_headroom(skb, LL_RESERVED_SPACE(dev));
                if (skb2 == NULL) {
                        kfree_skb(skb);
                        return -ENOMEM;
                }
                if (skb->sk)
                        skb_set_owner_w(skb2, skb->sk);
                kfree_skb(skb);
                skb = skb2;
        }

while

#define LL_RESERVED_SPACE(dev) \
        (((dev)->hard_header_len&~(HH_DATA_MOD - 1)) + HH_DATA_MOD)

so IMHO the above code fragment deals with device needs.

> If some place needs more, it is its problem to check.
> To the moment where it is used, hard_header_len can even change.
>
> It can be applied, but it does not change the fact, that those
> placed which fail now must check the condition as well.

Are you sure about that? It would mean almost devices, including
Ethernet, are at risk:

void ether_setup(struct net_device *dev)
{
        dev->change_mtu         = eth_change_mtu;
        dev->hard_header        = eth_header;
...

int eth_header(struct sk_buff *skb, struct net_device *dev, unsigned short type,
           void *daddr, void *saddr, unsigned len)
{
        struct ethhdr *eth = (struct ethhdr *)skb_push(skb,ETH_HLEN);


>> A similar problem may be present in psched_mtu().
>
> Nothing similar. The result psched_mtu() is compared with skb->len,
> how it is seen by qdiscs. If hard_header is NULL, it sees skbs
> without header.

Right, by "similar problem" I meant ignoring hard_header_len and not
kernel panic.
-- 
Krzysztof Halasa

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

Krzysztof Halasa <khc@pm.waw.pl> writes:

> Are you sure about that? It would mean almost devices, including
                                         ^^^^^^^^^^^^^^
> Ethernet, are at risk:

"almost all devices", of course.
-- 
Krzysztof Halasa

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

Hello Alexey,

Can you clarify this for me, please?

Do the semantics (I'm not talking about bugs) allow skb passed
to dev->hard_header() (if defined) and then to dev->hard_start_xmit()
to have less link layer header space than dev->hard_header_len?

I.e., is dev->hard_header_len only advisory?
-- 
Krzysztof Halasa

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Sun, 30 Jul 2006 18:40:04 +0200

> Do the semantics (I'm not talking about bugs) allow skb passed
> to dev->hard_header() (if defined) and then to dev->hard_start_xmit()
> to have less link layer header space than dev->hard_header_len?
> 
> I.e., is dev->hard_header_len only advisory?

It does seem weird that IP output won't pay attention to
dev->hard_header_len if the device does not provide a
dev->hard_header() method.  That is for sure.

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Alexey Kuznetsov]

Hello!

> It does seem weird that IP output won't pay attention to

Not so weird, actually.

The logic was:

Only initial skb allocation tries to reserve all the space
to avoid copies in the future.

All the rest of places just check, that there is enough space
for their immediate needs. If dev->hard_header() is NULL, it means that
stack does not need any space at all, so that it does not need to worry.

Right logic for reallocation would be:

	if (skb_headroom(skb) < space_which_I_need_now) {
		skb2 = skb_realloc_headroom(skb, space_for_future);
	}

That logic was not followed exactly only because of laziness,
each time some device is found which forgets to check for space,
so reallocation is made in absolutely inappropriate places.
F.e. ip_forward() does not need to reallocate skb when
skb_headroom() < dev->hard_header_len. It does and it is not good.

Good example is ipip tunnel. It sets:

	dev->hard_header_len = sizeof(iphdr) + LL_MAX_HEADER

because it does not know, what device will be used.
It is lots of space and most likely it will not use it.
So, initial allocation reserves lots of space, but all the rest
of stack should not reallocate, tunnel will take care of this itself.

Alexey

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Mon, 31 Jul 2006 22:04:33 +0200

> Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> writes:
> 
> > All the rest of places just check, that there is enough space
> > for their immediate needs. If dev->hard_header() is NULL, it means that
> > stack does not need any space at all, so that it does not need to worry.
> 
> Why do you think dev->hard_header == NULL means there is no need for
> header space? Isn't it dev->hard_header_len = 0? Why would a device
> set hard_header_len to non-zero if it doesn't need header space?

If you have headers to prepend for your device, why do you set the
header building function to NULL? :-)

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Mon, 31 Jul 2006 22:04:33 +0200

> This is non-trivial because hard_header and hard_start_xmit
> functions currently can't return new skb address (hard_header()
> can't use skb_realloc_headroom() at all, xmit() can't use it if
> there is a need to requeue the packet).
> 
> Or can you just realloc the data portion of skb without changing skb
> struct address? The skb may be referenced by other things.

Krzysztof, which device driver exactly creates this problem
in the first place?

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> Krzysztof, which device driver exactly creates this problem
> in the first place?

I have a report (not sure but I think it's that) with hdlc_fr (Frame
Relay).

Grepping through the tree there might be problems with:
- net/8021q/vlan.c (probably not with normal Ethernet, but there is
  a code path which could potentially be a problem with
  NETIF_F_HW_VLAN_TX)
- net/atm/clip.c
- net/appletalk/*
- drivers/net/gianfar.c
- drivers/net/wan/lapbether.c
- drivers/s390/net/netiucv.c will not oops but merely drop the packet
  and print a warning.

and possibly others, I haven't checked the whole tree.
Some (not all) of them might be false positives, though.

Fortunately most of the time skb comes with preallocated header space
(that common skb_reserve(2) I think) and thus the reports aren't
frequent (personally I have never seen that).

> If you have headers to prepend for your device, why do you set the
> header building function to NULL? :-)

hdlc_fr: logical PVC devices have no headers (plain IPv4 etc. as seen
by tcpdump), but they append FR headers (4 or 10 bytes long) just
before passing the skb to physical device.
-- 
Krzysztof Halasa

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Tue, 01 Aug 2006 03:04:28 +0200

> hdlc_fr: logical PVC devices have no headers (plain IPv4 etc. as seen
> by tcpdump), but they append FR headers (4 or 10 bytes long) just
> before passing the skb to physical device.

If you hooked up fr_hard_header into dev->hard_header instead of
invoking it via pvc_xmit(), everything would be fine.

The complexity of this function arises from the fact that it prepends
headers of differing lengths depending upon the protocol type
being encapsulated, and this is the problem you should aim to
solve.

Alexey, any suggestions on how to handle this kind of thing?

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

>> hdlc_fr: logical PVC devices have no headers (plain IPv4 etc. as seen
>> by tcpdump), but they append FR headers (4 or 10 bytes long) just
>> before passing the skb to physical device.
>
> If you hooked up fr_hard_header into dev->hard_header instead of
> invoking it via pvc_xmit(), everything would be fine.

That would have to be master_device->hard_header(), but the network
stack (IP and friends) has to send packets to pvc_device.
I can't make the headers show up on pvc device - that would break
packet interface and Ethernet framing. The headers have to be
visible only on master (physical) device.

> The complexity of this function arises from the fact that it prepends
> headers of differing lengths depending upon the protocol type
> being encapsulated, and this is the problem you should aim to
> solve.

Actually I don't think there is a problem with different header
lengths. The driver indicates it wants 10 bytes and that's enough
for all cases (except Ethernet framing where it indicates and uses
14 bytes and reallocs before prepending another 10 bytes).

> Alexey, any suggestions on how to handle this kind of thing?

What's wrong with my patch?

If it can's be accepted I can just add an empty pvc->hard_header().
That won't make other drivers work reliably, though, and it's IMHO
hardly their author's fault. I don't think we've ever advertised
"hard_header_len is valid only with non-NULL hard_header".
-- 
Krzysztof Halasa

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Alexey Kuznetsov]

Hello!

> > Alexey, any suggestions on how to handle this kind of thing?

Device, which adds something at head must check for space.
Anyone, who adds something at head, must check.
Otherwise, it will remain buggy forever.


> What's wrong with my patch?

As I already said there is nothing wrong with the first chunk.
Except that it hides the real problem.


> hardly their author's fault. I don't think we've ever advertised
> "hard_header_len is valid only with non-NULL hard_header".

Do not get it wrong. dev->hard_header_len is _NEVER_ guaranteed.
The places, which allocate skb, take it as a hint to avoid reallocation.
But each place which stuffs something at head, still must check the space.

The only difference between the situation with dev->hard_header,
is that when dev->hard_header != NULL, the header is added by IP itself.
That's why IP checks it.

Alexey

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Alexey Kuznetsov]

Hello!

> Do the semantics (I'm not talking about bugs) allow skb passed
> to dev->hard_header() (if defined)

No. dev->hard_header() should get enough of space, which is
dev->hard_header_len.

Actually, it is historical hole in design, inherited from ancient
times. Calling conventions of dev->hard_header() just did not allow
to reallocate. BTW in 2.6 it can, if it uses pskb_expand_head().


>			 and then to dev->hard_start_xmit()
> to have less link layer header space than dev->hard_header_len?

Absolutely. It used to happen all the time. All those devices,
which occasionally forget to check for space must be fixed.


> I.e., is dev->hard_header_len only advisory?

For initial allocator it is an advice. For layers, which add something
at head, it is just nothing, if there is enough space. And it is again
an advice, when skb is reallocated.


> Anyway, the issue with kernel panic is real so I think we better
> fix it before 2.6.18, and propagate to "stable" series as well.

:-) Know what? This problem followed us since prehistoric times.
It happened in 2.4-stablest, 2.2-stable, 2.0... The same devices,
the same problem, no matter how much of space it is given to them,
they managed to find a hole and to crash. :-)

Alexey

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

Hi,

Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> writes:

> As I already said there is nothing wrong with the first chunk.
> Except that it hides the real problem.

The problem is already very well hidden :-) I have seen just two
reports of this problem since 1994, and I believe both of them
were related to a device without hard_header() (i.e., it probably
wouldn't show with the patch applied).
I have never seen this problem personally, and I've been using
FR for about 10 years (in 1-3 locations - though I think at first it
did use hard_header()).


How about the second part of the patch? I know the second part isn't
related to kernel panic but don't you think hard_header_len
should be treated uniformly across the tree (i.e., shouldn't depend
on hard_header() being non-NULL)?

If not, fine.

> Do not get it wrong. dev->hard_header_len is _NEVER_ guaranteed.
> The places, which allocate skb, take it as a hint to avoid reallocation.
> But each place which stuffs something at head, still must check the space.
>
> The only difference between the situation with dev->hard_header,
> is that when dev->hard_header != NULL, the header is added by IP itself.
> That's why IP checks it.

Do you mean IP calls hard_header() which, in turn, does skb_push()?

How about Ethernet - is it safe? There is hard_header() which blindly
adds 14 bytes.
-- 
Krzysztof Halasa

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Wed, 02 Aug 2006 02:24:43 +0200

> Do you mean IP calls hard_header() which, in turn, does skb_push()?
> 
> How about Ethernet - is it safe? There is hard_header() which blindly
> adds 14 bytes.

I think Alexey is saying that setting ->hard_header() creates an
agreement between the device and IP that IP will make sure
that dev->hard_header_len bytes are available in the header
area.

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> writes:

> Actually, it is historical hole in design, inherited from ancient
> times. Calling conventions of dev->hard_header() just did not allow
> to reallocate. BTW in 2.6 it can, if it uses pskb_expand_head().

Does that mean that hard_header() and then hard_start_xmit()
can use pskb_expand_head() instead of skb_realloc_headroom()
without restrictions?
-- 
Krzysztof Halasa

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Wed, 02 Aug 2006 02:42:05 +0200

> Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> writes:
> 
> > Actually, it is historical hole in design, inherited from ancient
> > times. Calling conventions of dev->hard_header() just did not allow
> > to reallocate. BTW in 2.6 it can, if it uses pskb_expand_head().
> 
> Does that mean that hard_header() and then hard_start_xmit()
> can use pskb_expand_head() instead of skb_realloc_headroom()
> without restrictions?

It is definitely the case that ->hard_start_xmit() can use
pskb_expand_head(), some ethernet drivers even use it when they need
to clobber the IP and TCP headers to implement TSO and another entity
has a reference to the packet headers.  Just be sure to pass
GFP_ATOMIC to it in such a context.

One example is drivers/net/tg3.c:tg3_start_xmit().

Re: [PATCH] NET: fix kernel panic from no dev->hard_header_len space

[Krzysztof Halasa]

David Miller <davem@davemloft.net> writes:

> I think Alexey is saying that setting ->hard_header() creates an
> agreement between the device and IP that IP will make sure
> that dev->hard_header_len bytes are available in the header
> area.

I think I now understand it: hard_header_len is guaranteed while
calling hard_header() (because the check is done just before the call)
but not elsewhere, particularly not in hard_start_xmit().
dev->hard_header being NULL or not doesn't change anything.

I think hard_start_xmit() can be called without calling hard_header()
first, for example with things like PF_PACKET. This way the
hard_header_len check is skipped.


It looks it needs to be audited. I think either:
a) dev->hard_header_len must be eliminated completely and skb allocations
   have to assume some sane amount of header space (32 bytes or so), or
b) all dev->hard_header() and dev->hard_start_xmit() calling paths must
   be checked to contain at least dev->hard_header_len header space, or
c) dev->hard_header_len must be clearly marked as advisory, no core
   code changes (all drivers must be audited and fixed).
d) another idea?

What do you prefer?

a) would IMHO the best code quality, reallocations where they are needed
   and no strange semantics which can be easily broken by accident
   (nobody would count on nonexistent hard_header_len either).
   Fast path would not need to reallocate skb data, though the check
   would still be in place.
   We could test it by reducing "default" header space to zero,
   possibly a "hacking" kernel config option may be useful?

b) my patch is the starting point but I'm not sure it's practical.
c) IMHO the worst by all means.

I think I could do a) in a couple of weeks so that it could go into
2.6.19.

Back to my patch. I understand the part about ip_output() is ok
for 2.6.18, isn't it?

What about the psched_mtu() thing? While it's not kernel panic,
I think we should fix it. I'm not sure it should return  dev->mtu +
dev->hard_header_len or just dev->mtu, though.
-- 
Krzysztof Halasa