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

Andrew Lunn
2010-01-25T06:46:51Z

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