netdev - Re: [PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

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: <20210109194146.7c8ac5ce@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Sat, 9 Jan 2021 19:41:46 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Sukadev Bhattiprolu <sukadev@...ux.ibm.com>
Cc:     netdev@...r.kernel.org, Dany Madden <drt@...ux.ibm.com>,
        Lijun Pan <ljp@...ux.ibm.com>
Subject: Re: [PATCH 5/7] ibmvnic: use a lock to serialize remove/reset

On Thu,  7 Jan 2021 23:12:34 -0800 Sukadev Bhattiprolu wrote:
> Use a separate lock to serialze ibmvnic_reset() and ibmvnic_remove()
> functions. ibmvnic_reset() schedules work for the worker thread and
> ibmvnic_remove() flushes the work before removing the adapter. We
> don't want any work to be scheduled once we start removing the
> adapter (i.e after we have already flushed the work).

Locking based on functions, not on data being accessed is questionable
IMO. If you don't want work to be scheduled isn't it enough to have a
bit / flag that you set to let other flows know not to schedule reset?

> @@ -5459,6 +5464,7 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  {
>  	struct net_device *netdev = dev_get_drvdata(&dev->dev);
>  	struct ibmvnic_adapter *adapter = netdev_priv(netdev);
> +	unsigned long rmflags;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&adapter->state_lock, flags);
> @@ -5467,7 +5473,15 @@ static int ibmvnic_remove(struct vio_dev *dev)
>  		return -EBUSY;
>  	}

> +	spin_lock_irqsave(&adapter->remove_lock, rmflags);

You can just use flags again, no need for separate variables.

>  	adapter->state = VNIC_REMOVING;
> +	spin_unlock_irqrestore(&adapter->remove_lock, rmflags);