On Mon, 2014-07-07 at 11:03 +0300, Andrey Utkin wrote: > 2014-07-07 0:42 GMT+03:00 Eric Dumazet <eric.dumazet@xxxxxxxxx>: > >> /* 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 ! -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
