From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751288AbaJON0c (ORCPT ); Wed, 15 Oct 2014 09:26:32 -0400 Received: from mx0.aculab.com ([213.249.233.131]:44105 "HELO mx0.aculab.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751208AbaJON0a convert rfc822-to-8bit (ORCPT ); Wed, 15 Oct 2014 09:26:30 -0400 From: David Laight To: "'Michael S. Tsirkin'" , "linux-kernel@vger.kernel.org" CC: "David S. Miller" , Rusty Russell , "virtualization@lists.linux-foundation.org" , "netdev@vger.kernel.org" , Jason Wang Subject: RE: [PATCH] virtio_net: fix use after free Thread-Topic: [PATCH] virtio_net: fix use after free Thread-Index: AQHP6HqNesJA5YGO4ESSpoqfbOrRRpwxJNvQ Date: Wed, 15 Oct 2014 13:24:57 +0000 Message-ID: <063D6719AE5E284EB5DD2968C1650D6D1C9D77AB@AcuExch.aculab.com> References: <1413378878-28118-1-git-send-email-mst@redhat.com> In-Reply-To: <1413378878-28118-1-git-send-email-mst@redhat.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.202.99.200] Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Michael S. Tsirkin > commit 0b725a2ca61bedc33a2a63d0451d528b268cf975 > net: Remove ndo_xmit_flush netdev operation, use signalling instead. > > added code that looks at skb->xmit_more after the skb has > been put in TX VQ. Since some paths process the ring and free the skb > immediately, this can cause use after free. > > Fix by storing xmit_more in a local variable. > > Cc: David S. Miller > Signed-off-by: Michael S. Tsirkin > --- > > David, am I using the API correctly? > Seems to work for me. > You used __netif_subqueue_stopped but that seems to use > a slightly more expensive test_bit internally. > The reason I added a variable for the txq here is because it's handy for > BQL patch later on. > > > drivers/net/virtio_net.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 3d0ce44..13d0a8b 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -920,6 +920,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > int qnum = skb_get_queue_mapping(skb); > struct send_queue *sq = &vi->sq[qnum]; > int err; > + struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum); Do you need to cache 'txq' on stack for the entire call? Looks like it is only needed when 'kick' is true. I've not looked to see if saves both 'dev' and 'qnum' being kept. In any case it isn't mentioned in the commit message. David > + bool kick = !skb->xmit_more; > > /* Free up any pending old buffers before queueing new ones. */ > free_old_xmit_skbs(sq); > @@ -956,7 +958,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > } > } > > - if (__netif_subqueue_stopped(dev, qnum) || !skb->xmit_more) > + if (kick || netif_xmit_stopped(txq)) > virtqueue_kick(sq->vq); > > return NETDEV_TX_OK; > -- > MST > -- > 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