From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> Received: from mail-qt1-f195.google.com (mail-qt1-f195.google.com [209.85.160.195]) by dpdk.org (Postfix) with ESMTP id A1ABD2C01 for ; Fri, 30 Nov 2018 04:27:04 +0100 (CET) Received: by mail-qt1-f195.google.com with SMTP id k12so4502165qtf.7 for ; Thu, 29 Nov 2018 19:27:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=A5wmiLBE4qr1/fXYWJA2QPRpLlxTB6VMkqZwrewzPpo=; b=GxG9E0VPDH6UFA24LcZw5hUMOzxIkfkVY0rpECkiil3fdxuld7EjXnd6YLPX3Kdz2v 9c5jrDIDi8+/urFsfz/9EUW9MXZ0e7yFkx6/9xskVxMmr+U1WlaCcasWZxz2gTkxp6fS 2cd35oOpw0D7g17AmnrbnecrSKZUxGzRjKR1eFKXrmmjpRE55BPuYrkRr7LsG5kaqayw 1xIc4D7r5Pk/5Nov0rTI6bXxiOShZzvfDjfYuFzpPz/qvYc6SMkGFGpPo01Rr0DgDtE3 /1QjsqhSWia4oZMWRvQXRyF9S1yT0rE+V/fiX/VeaYHqPGTm1Dd7/tSBcA7WoAKwViIV fGEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=A5wmiLBE4qr1/fXYWJA2QPRpLlxTB6VMkqZwrewzPpo=; b=gUobRwpBhigbPo+9JUv1OFhCXkL7HtZdakfMmsvJpRz7wGzOApTeJqRVznTrr34R4B aWMgkIebpijm7gizCzDeF/srGxT3ODCoI0PoKoTsbOPvF7PWrhottRuvT07LJ57CmEsG GqfLDc5ecvdvQXUxAVOengdqq1AB77KpeEbHzCc8mOUUyZh8nxd+BIYncXoKjA8PyyF5 QsE0+Oj+E8ni1smaMOjKSbGVV04QxX0uBSqLO87RG+bPOrvE7tYAvjDhpAo9xG4fxyfO es+93kKA8T9uuzxucTEFx/XfhzdizvVrSSIK3fio+Fk2Mq/LB5BNxt15GBtuna9rApAe 2mdQ== X-Gm-Message-State: AA+aEWZ3+0/LsgZSXDd4wLR+fHBiA2Pw4Q+CGWA0iuMrj7N43cIktKi8 YbG0KRQHuvBXmvUVk5sW6y8= X-Google-Smtp-Source: AFSGD/UWhGVtDpea8Jrkuc2neY3jldXIutPfnb/+xmX1RN7J8+KMCm/yLblxVMeSJsbhvgX0a2rQEQ== X-Received: by 2002:a0c:afd1:: with SMTP id t17mr3981362qvc.93.1543548423946; Thu, 29 Nov 2018 19:27:03 -0800 (PST) Received: from [192.168.1.10] (pool-96-255-82-34.washdc.fios.verizon.net. [96.255.82.34]) by smtp.gmail.com with ESMTPSA id q5sm3446449qtq.20.2018.11.29.19.27.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 29 Nov 2018 19:27:03 -0800 (PST) To: Haifeng Lin , dev@dpdk.org Cc: chas3@att.com References: <1543469561-8864-1-git-send-email-haifeng.lin@huawei.com> From: Chas Williams <3chas3@gmail.com> Message-ID: <8fdfaff2-8f59-8d4b-db6c-a8a3c26fb4e2@gmail.com> Date: Thu, 29 Nov 2018 22:27:03 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <1543469561-8864-1-git-send-email-haifeng.lin@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] net/bonding: fix double fetch for active_slave_count X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Nov 2018 03:27:04 -0000 I guess this is slightly more correct. There is still a race here though. After you make your copy of active_slave_count, the number of active slaves could go to 0 and the memcpy() would copy an invalid element, acitve_slaves[0]. There is no simple fix to this problem. Your patch reduces the opportunity for a race but doesn't eliminate it. What you are using this API for? On 11/29/18 12:32 AM, Haifeng Lin wrote: > 1. when memcpy slaves the internals->active_slave_count 1 > 2. return internals->active_slave_count is 2 > 3. the slaves[1] would be a random invalid value > > Signed-off-by: Haifeng Lin > --- > drivers/net/bonding/rte_eth_bond_api.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c > index 21bcd50..ed7b02e 100644 > --- a/drivers/net/bonding/rte_eth_bond_api.c > +++ b/drivers/net/bonding/rte_eth_bond_api.c > @@ -815,6 +815,7 @@ > uint16_t len) > { > struct bond_dev_private *internals; > + uint16_t active_slave_count; > > if (valid_bonded_port_id(bonded_port_id) != 0) > return -1; > @@ -824,13 +825,14 @@ > > internals = rte_eth_devices[bonded_port_id].data->dev_private; > > - if (internals->active_slave_count > len) > + active_slave_count = internals->active_slave_count; > + if (active_slave_count > len) > return -1; > > memcpy(slaves, internals->active_slaves, > - internals->active_slave_count * sizeof(internals->active_slaves[0])); > + active_slave_count * sizeof(internals->active_slaves[0])); > > - return internals->active_slave_count; > + return active_slave_count; > } > > int >