netdev - Re: [PATCH net-next 1/9] drivers: net: xgene: Protect indirect MAC access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKh23FnsvzvhwbxW0qp=NUnAs-YAs4858_A6-5+0o0NPL4=THQ@mail.gmail.com>
Date:   Wed, 26 Apr 2017 21:12:16 -0700
From:   Iyappan Subramanian <isubramanian@....com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>, Quan Nguyen <qnguyen@....com>,
        patches <patches@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH net-next 1/9] drivers: net: xgene: Protect indirect MAC access

On Wed, Apr 26, 2017 at 5:06 PM, Florian Fainelli <f.fainelli@...il.com> wrote:
> On 04/26/2017 04:38 PM, Iyappan Subramanian wrote:
>> From: Quan Nguyen <qnguyen@....com>
>>
>> This patch adds lock to protect indirect mac access sequence.
>>
>> Signed-off-by: Quan Nguyen <qnguyen@....com>
>> Signed-off-by: Iyappan Subramanian <isubramanian@....com>
>> ---
>>  drivers/net/ethernet/apm/xgene/xgene_enet_hw.c    | 2 ++
>>  drivers/net/ethernet/apm/xgene/xgene_enet_main.c  | 1 +
>>  drivers/net/ethernet/apm/xgene/xgene_enet_main.h  | 1 +
>>  drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c | 7 ++++++-
>>  drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c | 2 ++
>>  5 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
>> index 2a835e0..3697ba7 100644
>> --- a/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
>> +++ b/drivers/net/ethernet/apm/xgene/xgene_enet_hw.c
>> @@ -365,9 +365,11 @@ static void xgene_enet_rd_mcx_mac(struct xgene_enet_pdata *pdata,
>>       cmd = pdata->mcx_mac_addr + MAC_COMMAND_REG_OFFSET;
>>       cmd_done = pdata->mcx_mac_addr + MAC_COMMAND_DONE_REG_OFFSET;
>>
>> +     spin_lock(&pdata->mac_lock);
>>       if (!xgene_enet_rd_indirect(addr, rd, cmd, cmd_done, rd_addr, rd_data))
>>               netdev_err(pdata->ndev, "MCX mac read failed, addr: %04x\n",
>>                          rd_addr);
>> +     spin_unlock(&pdata->mac_lock);
>
> Why not fold the spinlock manipulation within xgenet_enet_rd_indirect()?
> That way the caller does not have to know that acquiring the spinlock is
> required and you avoid potential bugs in the future where you are
> missing the spinlock on indirect accesses?

Thanks.  We'll do that and post the next version.

> --
> Florian