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

[Krzysztof Halasa <khc@pm.waw.pl>]

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