| Message ID | 1404647816-7564-1-git-send-email-andrey.krieger.utkin@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, 2014-07-06 at 14:56 +0300, Andrey Utkin wrote: > The sock ref counting is off so there is a kernel panic when you run > `atalkd`. See https://bugzilla.kernel.org/show_bug.cgi?id=79441 > This fix is similar to 0ae89beb283a ('can: add destructor for self > generated skbs') > should > Reported-by: Ed Martin <edman007@edman007.com> > Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com> > --- > net/appletalk/ddp.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c > index 01a1082..3d8ab34 100644 > --- a/net/appletalk/ddp.c > +++ b/net/appletalk/ddp.c > @@ -1399,6 +1399,11 @@ drop: > return NET_RX_DROP; > } > > +static inline void atalk_skb_destructor(struct sk_buff *skb) > +{ > + sock_put(skb->sk); > +} > + > /** > * atalk_rcv - Receive a packet (in skb) from device dev > * @skb - packet received > @@ -1489,6 +1494,8 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev, > goto drop; > > /* 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 (sock_queue_rcv_skb(sock, skb) < 0) > @@ -1644,6 +1651,8 @@ static int atalk_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr > if (!skb) > goto out; > > + sock_hold(sk); > + skb->destructor = atalk_skb_destructor; > skb->sk = sk; > skb_reserve(skb, ddp_dl->header_length); > skb_reserve(skb, dev->hard_header_len); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 ! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-07-07 at 10:57 +0200, Eric Dumazet wrote: > 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 ! Reading again this code, I think all you need is to remove the 2 buggy lines. No need for setup destructors. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-07-07 12:26 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>: > Reading again this code, I think all you need is to remove the 2 buggy > lines. > > No need for setup destructors. Reviewing the code again, i find you're right and adding a destructor in that place is stupid, and just not setting skb->sk would make bug disappear, assuming skb->sk is previously NULL. And what about the second case, at ddp.c near line 1650? Is destructor needed here?
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c index 01a1082..3d8ab34 100644 --- a/net/appletalk/ddp.c +++ b/net/appletalk/ddp.c @@ -1399,6 +1399,11 @@ drop: return NET_RX_DROP; } +static inline void atalk_skb_destructor(struct sk_buff *skb) +{ + sock_put(skb->sk); +} + /** * atalk_rcv - Receive a packet (in skb) from device dev * @skb - packet received @@ -1489,6 +1494,8 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev, goto drop; /* Queue packet (standard) */ + sock_hold(sock); + skb->destructor = atalk_skb_destructor; skb->sk = sock; if (sock_queue_rcv_skb(sock, skb) < 0) @@ -1644,6 +1651,8 @@ static int atalk_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr if (!skb) goto out; + sock_hold(sk); + skb->destructor = atalk_skb_destructor; skb->sk = sk; skb_reserve(skb, ddp_dl->header_length); skb_reserve(skb, dev->hard_header_len);
The sock ref counting is off so there is a kernel panic when you run `atalkd`. See https://bugzilla.kernel.org/show_bug.cgi?id=79441 This fix is similar to 0ae89beb283a ('can: add destructor for self generated skbs') Reported-by: Ed Martin <edman007@edman007.com> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com> --- net/appletalk/ddp.c | 9 +++++++++ 1 file changed, 9 insertions(+)