Re: [B.A.T.M.A.N.] bat_events: page allocation failure (batman-adv maint)

[Andrew Lunn <andrew@lunn.ch>]

On Sun, Jan 24, 2010 at 10:00:10PM +0100, Andrew Lunn wrote:
> On Sun, Jan 24, 2010 at 09:42:12PM +0100, Linus L??ssing wrote:
> > Hi Andrew,
> > 
> > Here is the output with the symbols inserted. I can reproduce this
> > memory leak very reliable now, by just activating the vis server
> > (didn't do that with the logs I've posted before) in a couple of
> > minutes. free is showing a nice countdown then :).
> 
> So it is vis server functionality which is leaking? If vis server is
> not enabled, there is no leak?
> 
> If so, we don't need kmemcheck. I expect just looking at the vis code
> is probably enough to find the missing free.

It looks pretty obvious:

Working our way via the call stack:

In batman_skb_recv() we have:

        case BAT_VIS:
                ret = recv_vis_packet(skb);
                break;
        default:
                ret = NET_RX_DROP;
        }
        if (ret == NET_RX_DROP)
                kfree_skb(skb);

i.e. the packet is only freed if recv_vis_packet() returns NET_RX_DROP


In recv_vis_packet() we have:
       switch (vis_packet->vis_type) {
        case VIS_TYPE_SERVER_SYNC:
                /* TODO: handle fragmented skbs properly */
                receive_server_sync_packet(vis_packet, skb_headlen(skb));
                ret = NET_RX_SUCCESS;
                break;

        case VIS_TYPE_CLIENT_UPDATE:
                /* TODO: handle fragmented skbs properly */
                receive_client_update_packet(vis_packet, skb_headlen(skb));
                ret = NET_RX_SUCCESS;
                break;

        default:        /* ignore unknown packet */
                ret = NET_RX_DROP;
                break;
        }
        return ret;

i.e. receive_client_update_packet() and receive_server_sync_packet()
need to sometime result in the packet being freed. batman_skb_recv()
will not free the packet.

void receive_server_sync_packet(struct vis_packet *vis_packet, int vis_info_len)
{
        struct vis_info *info;
        int is_new;
        unsigned long flags;
        int vis_server = atomic_read(&vis_mode);

        spin_lock_irqsave(&vis_hash_lock, flags);
        info = add_packet(vis_packet, vis_info_len, &is_new);
        if (info == NULL)
                goto end;

        /* only if we are server ourselves and packet is newer than the one in
         * hash.*/
        if (vis_server == VIS_TYPE_SERVER_SYNC && is_new) {
                memcpy(info->packet.target_orig, broadcastAddr, ETH_ALEN);
                if (list_empty(&info->send_list))
                        list_add_tail(&info->send_list, &send_list);
        }
end:
        spin_unlock_irqrestore(&vis_hash_lock, flags);
}

No free here. vis_packet is a pointer into the skbuf, but we never add
vis_packet to any queue here, just info is queued.

add_packet():

        info = kmalloc(sizeof(struct vis_info) + vis_info_len, GFP_ATOMIC);
        if (info == NULL)
                return NULL;

        INIT_LIST_HEAD(&info->send_list);
        INIT_LIST_HEAD(&info->recv_list);
        info->first_seen = jiffies;
        memcpy(&info->packet, vis_packet,
               sizeof(struct vis_packet) + vis_info_len);

We allocate memory can copy the contents of the packet into the
allocated memory. So again, the skbuf is not queued.

As far as i can see, the skbuf is never put on a queue. We just copy
the needed data out of it. This means the skbuf does need freeing.

I had a very quick look at receive_client_update_packet(). It seems to
have the same structure.

I will submit a fix for this soon. Is there any other similar code
which might have the same bug?

      Andrew