2002-11-14 17:06:03

by Serge Kuznetsov

[permalink] [raw]
Subject: [NET] Possible bug in netif_receive_skb

LeavesHi guys,

I think code for netif_receive_skb have a bug:

int netif_receive_skb(struct sk_buff *skb)
{
struct packet_type *ptype, *pt_prev;
int ret = NET_RX_DROP;
unsigned short type = skb->protocol;

/* some code skipped */

pt_prev = NULL;
for (ptype = ptype_all; ptype; ptype = ptype->next) {
if (!ptype->dev || ptype->dev == skb->dev) {
if (pt_prev) {
if (!pt_prev->data) {
ret = deliver_to_old_ones(pt_prev,
skb, 0);
} else {
atomic_inc(&skb->users);
ret = pt_prev->func(skb, skb->dev,
pt_prev);
}
/* Would be great to add this check here */
if ( ( ret == NET_RX_DROP ) || ( ret == NET_RX_BAD ) )
{
/* Check here if skb was freed in pt_prev_>func(), if not - free it */
...
return ret;
}
}
pt_prev = ptype;
}
}


First of all, just imagine if we have only on TAP attached to ptype_all.

In this case this TAP won't be called, because of pt_prev. ( BTW, why pt_prev, why we shouldn't call the function of first chain-link? )

In second case, imagine if we have three TAPs attached to ptype_all, and second TAP desided to be an universal firewall ( third one is just ordinar TAP ). If that second TAP will call kfree_skb ( skb ), and will return the code NET_RX_DROP or NET_RX_BAD, that return code will be overwritten by third TAP, which will get freed skb, and possible panic.

The same issue for the bottom half of the function ( for ptype_base ).

what if we will add the check for it? Like I added above.


PS: BTW, how to check if skb has been freed ? I didn't found any function for it. Is it possible to add the flag, like skb->freed ?


All the Best!
Serge.


2002-11-14 17:21:46

by David Miller

[permalink] [raw]
Subject: Re: [NET] Possible bug in netif_receive_skb


->func() must either take or free up the SKB, there must be no
violations of this rule.

2002-11-14 17:53:50

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [NET] Possible bug in netif_receive_skb

Em Thu, Nov 14, 2002 at 12:12:07PM -0500, Serge Kuznetsov escreveu:

> PS: BTW, how to check if skb has been freed ? I didn't found any function for
> it. Is it possible to add the flag, like skb->freed ?

Enable slab poisoning.

- Arnaldo

2002-11-14 20:08:39

by Serge Kuznetsov

[permalink] [raw]
Subject: Re: [NET] Possible bug in netif_receive_skb



>
> ->func() must either take or free up the SKB, there must be no
> violations of this rule.
>

Could you explain it more clearly?

How it applies to that two ( even three ) scenarios, I've told?

What if we have the first scenario:

ptype_all->func = func1;
ptype_all->next = NULL;

Will this function be called or not?

Second scenario:

ptype_all->func = func1;
ptype_all->next = &ptype1;

ptype1->func = func2;
ptype1->next = NULL;

Will func2() be called?

Third scenario:

ptype_all->func = func1;
ptype_all->next = &ptype1;

ptype1->func = func2;
ptype1->next = &ptype2;

ptype2->func = func3;
ptype2->next = &ptype3;

ptype3->func = func4;
ptype3->next = NULL;

If func2() freed skb, and return NET_RX_DROP, what will happen?


PS: I still don't understand why we should skip the first step, and call first function on second cycle?

2002-11-14 22:07:57

by Serge Kuznetsov

[permalink] [raw]
Subject: Re: [NET] Possible bug in netif_receive_skb



>
> ->func() must either take or free up the SKB, there must be no
> violations of this rule.
>

Could you explain it more clearly?

How it applies to that two ( even three ) scenarios, I've told?

What if we have the first scenario:

ptype_all->func = func1;
ptype_all->next = NULL;

Will this function be called or not?

Second scenario:

ptype_all->func = func1;
ptype_all->next = &ptype1;

ptype1->func = func2;
ptype1->next = NULL;

Will func2() be called?

Third scenario:

ptype_all->func = func1;
ptype_all->next = &ptype1;

ptype1->func = func2;
ptype1->next = &ptype2;

ptype2->func = func3;
ptype2->next = &ptype3;

ptype3->func = func4;
ptype3->next = NULL;

If func2() freed skb, and return NET_RX_DROP, what will happen?


PS: I still don't understand why we should skip the first step, and call first function on second cycle?

2002-11-15 14:03:30

by Serge Kuznetsov

[permalink] [raw]
Subject: Re: [NET] Possible bug in netif_receive_skb

> Em Thu, Nov 14, 2002 at 12:12:07PM -0500, Serge Kuznetsov escreveu:
>
> > PS: BTW, how to check if skb has been freed ? I didn't found any function for
> > it. Is it possible to add the flag, like skb->freed ?
>
> Enable slab poisoning.
>

Can I check if skb->users is not zero, it mean function hasn't been freed? Or just forget about it?

But what's happen if someone's ->func will return NET_RX_DROP, but not called the kfree_skb ? Is it not good to check for it?
Why should we trust for that function, may be someone forget to do it?

PS: Sorry, if my this questions hurts anyone, but for last almost 10 years, I'm using the Linux, I like it so much, and I want improve it and make it better, as much as possible.


All the Best!
Serge.