Messages in this thread | | | Subject | Re: [PATCH] appletalk: Set skb with destructor | From | Eric Dumazet <> | Date | Mon, 07 Jul 2014 10:57:36 +0200 |
| |
On Mon, 2014-07-07 at 11:03 +0300, Andrey Utkin wrote: > 2014-07-07 0:42 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>: > >> /* Queue packet (standard) */ > >> + sock_hold(sock); > >> + skb->destructor = atalk_skb_destructor; > >> skb->sk = sock; > > > > This part is not needed : sock_queue_rcv_skb() already does the right > > thing : It calls skb_set_owner_r(skb, sk); > > > > You should therefore remove the "skb->sk = sock;" line > > If it is so, i think this should be as another patch with its own reasoning. >
No it is not.
Its illegal to set skb->sk to a socket without taking proper reference.
But it is useless to do this before calling sock_queue_rcv_skb(), as I explained to you.
This is adding two extra atomic operations for nothing: skb_orphan() is called from sock_queue_rcv_skb(), so it is kind of stupid to set a destructor that _will_ be immediately called.
We do not do defensive programming, we try to do logical things, and only logical things.
Please re-spin your patch, by integrating my feedback.
Thanks !
| |