| Message ID | 1495093807-11000-1-git-send-email-xiaolou4617@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hello. On Thu, 2017-05-18 at 15:50, linzhang wrote: > This patch fixes the kernel oops when release net_device reference in > advance. In function raw_sendmsg(i think the dgram_sendmsg has the same > problem), there is a race condition between dev_put and dev_queue_xmit > when the device is gong that maybe lead to dev_queue_ximt to see > an illegal net_device pointer. > You have a test case to reproduce this oops? I fear I have not seen one. > So i think that dev_put should be behind of the dev_queue_xmit. > > Also, explicit set skb->sk is needless, sock_alloc_send_skb is > already set it. You could have put this fixup in a different patch. > Signed-off-by: linzhang <xiaolou4617@gmail.com> This looks more like a username instead of a real name. If you have Lin Zhang as you English real name that would be better here. :) > --- > net/ieee802154/socket.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c > index eedba76..a60658c 100644 > --- a/net/ieee802154/socket.c > +++ b/net/ieee802154/socket.c > @@ -301,15 +301,14 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > goto out_skb; > > skb->dev = dev; > - skb->sk = sk; > skb->protocol = htons(ETH_P_IEEE802154); > > - dev_put(dev); > - > err = dev_queue_xmit(skb); > if (err > 0) > err = net_xmit_errno(err); > > + dev_put(dev); > + > return err ?: size; > > out_skb: > @@ -690,15 +689,14 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > goto out_skb; > > skb->dev = dev; > - skb->sk = sk; > skb->protocol = htons(ETH_P_IEEE802154); > > - dev_put(dev); > - > err = dev_queue_xmit(skb); > if (err > 0) > err = net_xmit_errno(err); > > + dev_put(dev); > + > return err ?: size; Going to give this a test ride here now. regards Stefan Schmidt -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On Thu, 2017-05-18 at 15:14, Stefan Schmidt wrote: > Hello. > > On Thu, 2017-05-18 at 15:50, linzhang wrote: > > This patch fixes the kernel oops when release net_device reference in > > advance. In function raw_sendmsg(i think the dgram_sendmsg has the same > > problem), there is a race condition between dev_put and dev_queue_xmit > > when the device is gong that maybe lead to dev_queue_ximt to see > > an illegal net_device pointer. > > > > You have a test case to reproduce this oops? I fear I have not seen > one. If you have a test case handy adding it to the commit would be handy. If you do not have one around we can do without. > > So i think that dev_put should be behind of the dev_queue_xmit. > > > > Also, explicit set skb->sk is needless, sock_alloc_send_skb is > > already set it. > > You could have put this fixup in a different patch. I actually would request you to split this into two patches. One for the removal of the sk setting and one for the race condition fix. > > Signed-off-by: linzhang <xiaolou4617@gmail.com> > > This looks more like a username instead of a real name. If you have Lin > Zhang as you English real name that would be better here. :) This would be also appreciated. > > --- > > net/ieee802154/socket.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c > > index eedba76..a60658c 100644 > > --- a/net/ieee802154/socket.c > > +++ b/net/ieee802154/socket.c > > @@ -301,15 +301,14 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > > goto out_skb; > > > > skb->dev = dev; > > - skb->sk = sk; > > skb->protocol = htons(ETH_P_IEEE802154); > > > > - dev_put(dev); > > - > > err = dev_queue_xmit(skb); > > if (err > 0) > > err = net_xmit_errno(err); > > > > + dev_put(dev); > > + > > return err ?: size; > > > > out_skb: > > @@ -690,15 +689,14 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) > > goto out_skb; > > > > skb->dev = dev; > > - skb->sk = sk; > > skb->protocol = htons(ETH_P_IEEE802154); > > > > - dev_put(dev); > > - > > err = dev_queue_xmit(skb); > > if (err > 0) > > err = net_xmit_errno(err); > > > > + dev_put(dev); > > + > > return err ?: size; > > Going to give this a test ride here now. I gave it a ride in my testbed and I encountered no problems. While I have never seen the race and oops myself doing the dev_put before the xmit can surely lead to such a race and the fix is valid. Once you have done the changes requested above and re-submit your two patches you can add my Acked-by: Stefan Schmidt <stefan@osg.samsung.com> to both of them. regards Stefan Schmidt -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c index eedba76..a60658c 100644 --- a/net/ieee802154/socket.c +++ b/net/ieee802154/socket.c @@ -301,15 +301,14 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) goto out_skb; skb->dev = dev; - skb->sk = sk; skb->protocol = htons(ETH_P_IEEE802154); - dev_put(dev); - err = dev_queue_xmit(skb); if (err > 0) err = net_xmit_errno(err); + dev_put(dev); + return err ?: size; out_skb: @@ -690,15 +689,14 @@ static int dgram_sendmsg(struct sock *sk, struct msghdr *msg, size_t size) goto out_skb; skb->dev = dev; - skb->sk = sk; skb->protocol = htons(ETH_P_IEEE802154); - dev_put(dev); - err = dev_queue_xmit(skb); if (err > 0) err = net_xmit_errno(err); + dev_put(dev); + return err ?: size; out_skb:
This patch fixes the kernel oops when release net_device reference in advance. In function raw_sendmsg(i think the dgram_sendmsg has the same problem), there is a race condition between dev_put and dev_queue_xmit when the device is gong that maybe lead to dev_queue_ximt to see an illegal net_device pointer. So i think that dev_put should be behind of the dev_queue_xmit. Also, explicit set skb->sk is needless, sock_alloc_send_skb is already set it. Signed-off-by: linzhang <xiaolou4617@gmail.com> --- net/ieee802154/socket.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)