Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
David Miller [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
Hi, Alexey Kuznetsov [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
From: Krzysztof Halasa [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
From: Krzysztof Halasa [EMAIL PROTECTED] Date: Wed, 02 Aug 2006 02:42:05 +0200 Alexey Kuznetsov [EMAIL PROTECTED] 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(). - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
From: Krzysztof Halasa [EMAIL PROTECTED] Date: Mon, 31 Jul 2006 22:04:33 +0200 Alexey Kuznetsov [EMAIL PROTECTED] 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? :-) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
From: Krzysztof Halasa [EMAIL PROTECTED] 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? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
David Miller [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
From: Krzysztof Halasa [EMAIL PROTECTED] 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? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
David Miller [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
From: Krzysztof Halasa [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
Krzysztof Halasa [EMAIL PROTECTED] writes: Are you sure about that? It would mean almost devices, including ^^ Ethernet, are at risk: almost all devices, of course. -- Krzysztof Halasa - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] NET: fix kernel panic from no dev-hard_header_len space
Alexey Kuznetsov [EMAIL PROTECTED] 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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
