netdev - Re: [PATCH net-next v2] ibmvnic: Bail from ibmvnic

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] [thread-next>] [day] [month] [year] [list]

Message-Id: <c7b3b39f-77d5-b6e8-1886-286422e72a47@linux.vnet.ibm.com>
Date:   Mon, 12 Mar 2018 14:56:51 -0500
From:   John Allen <jallen@...ux.vnet.ibm.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org,
        Thomas Falcon <tlfalcon@...ux.vnet.ibm.com>,
        Nathan Fontenot <nfont@...ux.vnet.ibm.com>
Subject: Re: [PATCH net-next v2] ibmvnic: Bail from ibmvnic_open if driver is
 already open

On 03/12/2018 02:33 PM, Andrew Lunn wrote:
> On Mon, Mar 12, 2018 at 02:19:52PM -0500, John Allen wrote:
>> If the driver is already in the "open" state, don't attempt the procedure
>> for opening the driver.
>>
>> Signed-off-by: John Allen <jallen@...ux.vnet.ibm.com>
>> ---
>> v2: Unlock reset_lock mutex before returning.
>>
>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index 7be4b06..9a5e8ac 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -1057,6 +1057,11 @@ static int ibmvnic_open(struct net_device *netdev)
>>
>>  	mutex_lock(&adapter->reset_lock);
>>
>> +	if (adapter->state == VNIC_OPEN) {
>> +		mutex_unlock(&adapter->reset_lock);
>> +		return 0;
>> +	}
> 
> Hi John
> 
> This looks better.
> 
> But how did you get ibmvnic_open() to be called twice? Is the actual
> issue that __ibmvnic_close() does not kill off the
> adapter->ibmvnic_reset workqueue, so that a reset can happen after
> close() has been called?

The problem here is that our routine to change the mtu does a full reset on
the driver meaning that in the process we go from effectively "open" to
"closed" to "open" again.

Consider the scenario where we change the mtu by running "ifdown <interface>",
editing the ifcfg file with the new mtu, and finally running "ifup <interface".
In this case, we call ibmvnic_close from the ifdown and as a result of the ifup,
we do the reset for the mtu change (which puts us back in the "open" state) and
call ibmvnic_open. After the reset, we are already in the "open" state and the
following call to open is redundant. The check in ibmvnic_open to see if we're
in the open state is really just insurance to guard against any case like this
where we're in the open state from the driver perspective and something decides
to trigger the drivers open routine.

-John

> 
> 	   Andrew
> 
>