From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752824AbdJTKhV (ORCPT ); Fri, 20 Oct 2017 06:37:21 -0400 Received: from mga14.intel.com ([192.55.52.115]:37568 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752092AbdJTKhT (ORCPT ); Fri, 20 Oct 2017 06:37:19 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,405,1503385200"; d="scan'208";a="1233114586" From: "Reshetova, Elena" To: Sean Wang CC: "davem@davemloft.net" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-mediatek@lists.infradead.org" , "linux-rdma@vger.kernel.org" , "linux-hams@vger.kernel.org" , "linux-ppp@vger.kernel.org" , "ganeshgr@chelsio.com" , "nbd@openwrt.org" , "john@phrozen.org" , "matthias.bgg@gmail.com" , "yishaih@mellanox.com" , "saeedm@mellanox.com" , "matanb@mellanox.com" , "tariqt@mellanox.com" , "leonro@mellanox.com" , "ajk@comnets.uni-bremen.de" , "paulus@samba.org" , "zbr@ioremap.net" , "peterz@infradead.org" , "keescook@chromium.org" Subject: RE: [PATCH 02/15] drivers, net, ethernet: convert mtk_eth.dma_refcnt from atomic_t to refcount_t Thread-Topic: [PATCH 02/15] drivers, net, ethernet: convert mtk_eth.dma_refcnt from atomic_t to refcount_t Thread-Index: AQHTSXR7/KSFQ6fCZ0GjC1dX1PSD9KLsUaKAgAA4tjA= Date: Fri, 20 Oct 2017 10:37:13 +0000 Message-ID: <2236FBA76BA1254E88B949DDB74E612B802B37EE@IRSMSX102.ger.corp.intel.com> References: <1508484229-19706-1-git-send-email-elena.reshetova@intel.com> <1508484229-19706-3-git-send-email-elena.reshetova@intel.com> <1508486927.29850.12.camel@mtkswgap22> In-Reply-To: <1508486927.29850.12.camel@mtkswgap22> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjUwMWYzY2ItY2UxMy00MGYwLTkwMzgtOTA1NmVkNzg0ZjdjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJ3NVA3Z1Mwd011QTd6akNkeWZMMFdmc0ZcL2hEMzVuZlNPeTkwdkhmbCtjZFZvTDlvakNZblwva3JVZE1UdkZZK1QifQ== x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v9KAbTl8029315 > On Fri, 2017-10-20 at 10:23 +0300, Elena Reshetova wrote: > > atomic_t variables are currently used to implement reference > > counters with the following properties: > > - counter is initialized to 1 using atomic_set() > > - a resource is freed upon counter reaching zero > > - once counter reaches zero, its further > > increments aren't allowed > > - counter schema uses basic atomic operations > > (set, inc, inc_not_zero, dec_and_test, etc.) > > > > Such atomic variables should be converted to a newly provided > > refcount_t type and API that prevents accidental counter overflows > > and underflows. This is important since overflows and underflows > > can lead to use-after-free situation and be exploitable. > > > > The variable mtk_eth.dma_refcnt is used as pure reference counter. > > Convert it to refcount_t and fix up the operations. > > > > Suggested-by: Kees Cook > > Reviewed-by: David Windsor > > Reviewed-by: Hans Liljestrand > > Signed-off-by: Elena Reshetova > > --- > > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 8 +++++--- > > drivers/net/ethernet/mediatek/mtk_eth_soc.h | 4 +++- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > index 5e81a72..54adfd9 100644 > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > @@ -1817,7 +1817,7 @@ static int mtk_open(struct net_device *dev) > > struct mtk_eth *eth = mac->hw; > > > > /* we run 2 netdevs on the same dma ring so we only bring it up once > */ > > - if (!atomic_read(ð->dma_refcnt)) { > > + if (!refcount_read(ð->dma_refcnt)) { > > int err = mtk_start_dma(eth); > > > > if (err) > > @@ -1827,8 +1827,10 @@ static int mtk_open(struct net_device *dev) > > napi_enable(ð->rx_napi); > > mtk_tx_irq_enable(eth, MTK_TX_DONE_INT); > > mtk_rx_irq_enable(eth, MTK_RX_DONE_INT); > > + refcount_set(ð->dma_refcnt, 1); > > the existing driver seems to have a missing initial atomic_set for the > eth->dma_refcnt. > > > } > > - atomic_inc(ð->dma_refcnt); > > + else > > + refcount_inc(ð->dma_refcnt); > > > > how about add the initial refcount_set into probe handler, and keep > logic else unchanged ? Sure, I guess you mean mtk_probe() function? I can move the refcount_set to be there and remove this change. Should I resend the modified patch to you (maybe then two of the ethernet patches)? Best Regards, Elena. > > > phy_start(dev->phydev); > > netif_start_queue(dev); > > @@ -1868,7 +1870,7 @@ static int mtk_stop(struct net_device *dev) > > phy_stop(dev->phydev); > > > > /* only shutdown DMA if this is the last user */ > > - if (!atomic_dec_and_test(ð->dma_refcnt)) > > + if (!refcount_dec_and_test(ð->dma_refcnt)) > > return 0; > > > > mtk_tx_irq_disable(eth, MTK_TX_DONE_INT); > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h > b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > > index 3d3c24a..a3af466 100644 > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > > @@ -15,6 +15,8 @@ > > #ifndef MTK_ETH_H > > #define MTK_ETH_H > > > > +#include > > + > > #define MTK_QDMA_PAGE_SIZE 2048 > > #define MTK_MAX_RX_LENGTH 1536 > > #define MTK_TX_DMA_BUF_LEN 0x3fff > > @@ -632,7 +634,7 @@ struct mtk_eth { > > struct regmap *pctl; > > u32 chip_id; > > bool hwlro; > > - atomic_t dma_refcnt; > > + refcount_t dma_refcnt; > > struct mtk_tx_ring tx_ring; > > struct mtk_rx_ring > rx_ring[MTK_MAX_RX_RING_NUM]; > > struct mtk_rx_ring rx_ring_qdma; >